Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Replaced CaptureScreenshot with
document.activeElement.href
#3base: master
Are you sure you want to change the base?
Replaced CaptureScreenshot with
document.activeElement.href
#3Changes from 3 commits
3c668ae
c4e30db
59f6603
a7f9a45
07de945
0369c1f
2b17b21
c224e2f
e4a0f89
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just realized; doesn't this actually do the job of navToLast (except the actual navigation itself)? So couldn't we move that code to inside navToLast, so that it replaces there the loop that relies on the location change? And then we would only have to do one "\n" key event right after?
Alternatively, we could keep it here, and remove most of the the code in navToLast (it would just do the one "\n" event basically, but I don't like this solution as much as it does not separate as well each task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could structure it that way.
but I also prefer keeping the tasks separate.
In my other experiment, I am trying to abstract the following:
sleeps
. There is also an event fired for an initiated download, which obviates the need for the directory polling.When I'm done finding the right abstractions I can port it back to Go/cdp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure we understood each other, so just to be clear:
If I'm not making sense, I can add myself a commit on top of this PR so you can see what I mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying.
No, I don't agree that most of what navToLast is doing is redundant
We still need
navToLast
to work the way it does, because the selected element (309-310), is not guaranteed to be the actual last element, it is just close to the end.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, understood, let's leave L309 and navToLast as they are for now.
Do you know why L309-L310 isn't enough to get the very last element though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, then I don't think we should have previousHref in this logging message, as it might lead one to believe that previousHref is the very last element of the page (which it might not be, as you've just explained).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
href isn't used by anything, so you could save yourself one variable, and just
return lastElement["href"], nil