-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
[WIP] Service worker support #391
base: main
Are you sure you want to change the base?
Conversation
@@ -71,11 +71,12 @@ def reset | |||
# @return [Cookies] | |||
attr_reader :cookies | |||
|
|||
def initialize(target_id, browser, proxy: nil) | |||
def initialize(target_id, browser, proxy: nil, type: "page") |
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.
Do you want me to use Page
or create another class for workers?
@@ -355,13 +356,21 @@ def subscribe | |||
end | |||
|
|||
def prepare_page |
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.
Should I keep everything in this method or split out the worker preparation into a different method?
end | ||
|
||
def build(**options) | ||
connection(**options) | ||
end | ||
|
||
def build_page(**options) |
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.
Should I just alias_method
this to build
?
def maybe_sleep_if_new_window | ||
# Dirty hack because new window doesn't have events at all | ||
sleep(NEW_WINDOW_WAIT) if window? | ||
end | ||
|
||
def connection(**options) |
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.
Not sure what the proper naming would be for this if it can be both a page and a worker.
@@ -23,12 +24,19 @@ def attached? | |||
end | |||
|
|||
def page | |||
@page ||= build_page | |||
connection if page? |
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.
Initially I had this return nil if it was a worker because other parts of the code were causing failures otherwise, but not sure what the impact of this is.
spec/support/public/one.png
Outdated
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.
Just an image I came up with on Figma, no particular attachment to it if you'd like something else. I tried a SVG but it didn't work with the createImageBitmap
code.
let canvas = document.getElementById('canvas'); | ||
let context = canvas.getContext('2d'); | ||
let imageBitmap = event.data.data; | ||
context.drawImage(imageBitmap, 0, 0); |
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.
Drawn to the canvas so the page isn't blank.
page.go_to("/ferrum/service_worker") | ||
|
||
browser.network.wait_for_idle | ||
traffic = browser.targets.values.map { _1.network.traffic }.flatten |
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.
Should the worker bubble up its traffic to its parent page?
I'm now on the road but I'll check your implementation this weekend, I hope by that time I'll be home. |
Okay, sounds great! |
@johncoates-st let me first finish new headless mode for Ferrum, as there are architecture changes for the gem since frames don't work like they were. So we might need some changes here as well. |
@route Okay, sounds good. What kind of timeline are you thinking for that new headless mode getting merged? |
@johncoates-st it's merged, so I'm fully yours ;) I'll start playing with it today. |
@route Great! Looking forward to your feedback |
I think first of all we should address |
The support for flatten mode and auto-attach landed to the main branch. Could you please rebase and now we can continue on service workers. To answer your questions I think it makes more sense not to derive from the Page class. |
@route That's great! Okay sounds good, I'll rebase. |
thanks for your work @johncoates-st |
Implementation of:
Hey @route, this is a follow up to our conversation as far as service worker support. I based this PR on your comment:
This is a first draft. I could use a bit more guidance, so I'll leave some notes across the changed files.