Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/#399 navigation #432

Merged
merged 33 commits into from
Dec 24, 2019
Merged

Bugfix/#399 navigation #432

merged 33 commits into from
Dec 24, 2019

Conversation

ziflex
Copy link
Member

@ziflex ziflex commented Dec 21, 2019

Big changes!
Refactored the way how we deal with frames and network events:

  • new EventLoop (no hardcoded EventBroker anymore)
  • new DOM manager
  • new Network manager
  • more accurate event handling
  • faster Document update
  • redirects handling

@ziflex ziflex added area/stdlib Standard library issue area/drivers/cdp Cdp driver area/drivers HTML drivers labels Dec 21, 2019
@ziflex ziflex requested a review from 3timeslazy December 21, 2019 01:00
@codecov
Copy link

codecov bot commented Dec 21, 2019

Codecov Report

Merging #432 into master will increase coverage by 2.3%.
The diff coverage is 17.2%.

@@           Coverage Diff            @@
##           master    #432     +/-   ##
========================================
+ Coverage    39.4%   41.7%   +2.3%     
========================================
  Files         222     227      +5     
  Lines        9096    8756    -340     
========================================
+ Hits         3585    3654     +69     
+ Misses       5154    4746    -408     
+ Partials      357     356      -1

)

type (
DocumentUpdatedListener func(ctx context.Context)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment to all types below please.

}
)

// Many is a helper function that tells event loop to always call the function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Always...

}

// Many is a helper function that tells event loop to call the function only once
func Once(fn func(ctx context.Context, message interface{})) Handler {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Once...

lc.mu.Lock()
defer lc.mu.Unlock()

bucket, exists := lc.values[eventID]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go allows you to delete key from the map without any checks.
Example: https://play.golang.org/p/yHhJIsYHkm6

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but i'm not deleting it from lc.values. I'm deleting in from a nested map.

"github.com/mafredri/cdp/rpcc"
)

type (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment to all types below please.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

)

var (
Error = New("error")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a builtin event ID


oc := other.(HTTPCookies)

if len(c) > len(oc) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch-case seems to be more readable

switch {
case len(c) > len(oc):
	return 1
case len(c) < len(oc):
	return -1
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? I didn't know that you can omit switch expression.


var emptyExpires = time.Time{}

func parseAttrs(attrs []string) *values.Object {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t quite understand what is going on here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CDP returns HTML attributes as an array of strings of key-value pairs. This methods parses the array and converts it into an object.

@ziflex ziflex merged commit fe7b45d into master Dec 24, 2019
@ziflex ziflex deleted the bugfix/#399-navigation branch December 26, 2019 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/drivers/cdp Cdp driver area/drivers HTML drivers area/stdlib Standard library issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants