Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3018603002:
Add tab.StopAllServiceWorkers() (Closed)
DescriptionAdd tab.StopAllServiceWorkers()
This CL adds a method StopAllServiceWorkers() to tab.py .
In order to implement this method, I also made InspectorServiceWorker class and use websocket connections logic with devtool ServiceWorker domain in it.
ServiceWorker backend is enabled for all InspectorBackends on initializing InspectorServiceWorker (with devtools method ServiceWorker.enable).
Design doc of perf benchmark for PWA:
https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yuuo/edit?usp=sharing
BUG=chromium:736697
Review-Url: https://chromiumcodereview.appspot.com/3018603002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/639e972bf15c206a77decda169e022c41c5aae36
Patch Set 1 #Patch Set 2 : add comments #
Total comments: 3
Patch Set 3 : add inspector_serviceworker #
Total comments: 8
Patch Set 4 : add comments #
Total comments: 4
Patch Set 5 : add comments #Patch Set 6 : call ServiceWorker.enable on initializing InspectorServiceWorker #
Total comments: 2
Patch Set 7 : edit a comment #
Total comments: 2
Patch Set 8 : edit a TODO comment #
Total comments: 2
Patch Set 9 : edit comment #
Dependent Patchsets: Messages
Total messages: 30 (8 generated)
yukiy@google.com changed reviewers: + horo@chromium.org, kouhei@chromium.org, nednguyen@google.com, shimazu@chromium.org
As mentioned here (https://codereview.chromium.org/3011263002/diff/80001/telemetry/telemetry/pag...), this CL adds two methods for tab.py . Running Status of service worker is not reachable from tab_unittest.py I think, so I have no good idea for adding some tests for these new methods. horo@ shimazu@ Could you give me any advice? PTAL :D
https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/tab.py (right): https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/tab.py:298: def EnableServiceWorker(self, timeout=DEFAULT_TAB_TIMEOUT): Can you move these to inspector_backend? Then these two methods just call: inspector_backend.EnableServiceWorker & inspector_backend.StopServiceWorker
https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/tab.py (right): https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/tab.py:277: def ClearDataForOrigin(self, url, timeout=DEFAULT_TAB_TIMEOUT): I will make a CL to refactor this to inspector_backend as well
On 2017/09/22 11:11:31, nednguyen wrote: > https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... > File telemetry/telemetry/internal/browser/tab.py (right): > > https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... > telemetry/telemetry/internal/browser/tab.py:277: def ClearDataForOrigin(self, > url, timeout=DEFAULT_TAB_TIMEOUT): > I will make a CL to refactor this to inspector_backend as well https://codereview.chromium.org/3013243002/ show how it should be done. Sorry, I should have paid attention to this in earlier code review.
Description was changed from ========== Add tab.EnableServiceWorker() and tab.StopServiceWorker() This CL adds two methods EnableServiceWorker() and StopServiceWorker() to tab.py . These methods call devtools method. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add tab.EnableServiceWorker() and tab.StopAllServiceWorkers() This CL adds two methods EnableServiceWorker() and StopAllServiceWorkers() to tab.py . These methods call devtools method. Also madeInspectorServiceWorker class and use websocket connections logic with devtool ServiceWorker domain in it. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ==========
On 2017/09/22 11:43:26, nednguyen wrote: > On 2017/09/22 11:11:31, nednguyen wrote: > > > https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/browser/tab.py (right): > > > > > https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... > > telemetry/telemetry/internal/browser/tab.py:277: def ClearDataForOrigin(self, > > url, timeout=DEFAULT_TAB_TIMEOUT): > > I will make a CL to refactor this to inspector_backend as well > > https://codereview.chromium.org/3013243002/ show how it should be done. Sorry, I > should have paid attention to this in earlier code review. Thanks so much for your help, nednguyen@ ! PTAL :-) https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/tab.py (right): https://codereview.chromium.org/3018603002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/tab.py:298: def EnableServiceWorker(self, timeout=DEFAULT_TAB_TIMEOUT): On 2017/09/22 11:07:34, nednguyen wrote: > Can you move these to inspector_backend? Then these two methods just call: > inspector_backend.EnableServiceWorker & inspector_backend.StopServiceWorker Done.
lgtm with minor comments https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py (right): https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py:18: def EnableServiceWorker(self, timeout): "Enable" seems enough since this is a method owned by InspectorServiceWorker, and for making the name consistent with the devtools' methods. https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py:24: def StopAllServiceWorkers(self, timeout): ditto (StopAllWorkers)
https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/tab.py (right): https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/tab.py:290: """Enables devtools for ServiceWorker domain. Is this accurate?
https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/tab.py (right): https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/tab.py:290: """Enables devtools for ServiceWorker domain. On 2017/09/25 01:36:16, kouhei (in TOK) wrote: > Is this accurate? At least, enable should be called before calling any methods. I'm not so much familiar with the history of it, but from the blame this mechanism is to avoid unnecessary events: https://bugs.webkit.org/show_bug.cgi?id=73411
https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/tab.py (right): https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/tab.py:290: """Enables devtools for ServiceWorker domain. On 2017/09/25 01:43:34, shimazu wrote: > On 2017/09/25 01:36:16, kouhei (in TOK) wrote: > > Is this accurate? > > At least, enable should be called before calling any methods. > I'm not so much familiar with the history of it, but from the blame this > mechanism is to avoid unnecessary events: > https://bugs.webkit.org/show_bug.cgi?id=73411 Then, how about "Start listening for ServiceWorker notifications"? (I'm not familiar enough w/ the code to tell if this is correct)
PTAL:D I"m not sure if calling inspector_serviceworker.Enable() on initializing inspector_backend is accurate, so please tell me if there are better ways. https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py (right): https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py:18: def EnableServiceWorker(self, timeout): On 2017/09/25 01:30:37, shimazu wrote: > "Enable" seems enough since this is a method owned by InspectorServiceWorker, > and for making the name consistent with the devtools' methods. Done. https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py:24: def StopAllServiceWorkers(self, timeout): On 2017/09/25 01:30:37, shimazu wrote: > ditto (StopAllWorkers) Done. https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/tab.py (right): https://codereview.chromium.org/3018603002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/tab.py:290: """Enables devtools for ServiceWorker domain. On 2017/09/25 01:54:41, kouhei (in TOK) wrote: > On 2017/09/25 01:43:34, shimazu wrote: > > On 2017/09/25 01:36:16, kouhei (in TOK) wrote: > > > Is this accurate? > > > > At least, enable should be called before calling any methods. > > I'm not so much familiar with the history of it, but from the blame this > > mechanism is to avoid unnecessary events: > > https://bugs.webkit.org/show_bug.cgi?id=73411 > > Then, how about "Start listening for ServiceWorker notifications"? > (I'm not familiar enough w/ the code to tell if this is correct) I modified and moved this comment to inspector_serviceworker.py , so please check this there.
https://codereview.chromium.org/3018603002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py (right): https://codereview.chromium.org/3018603002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py:83: self._serviceworker.Enable(timeout) This enables ServiceWorker backend for all InspectorBackends. I think this is acceptable given its low overhead. Would you note this behavioral change in CL desc? https://codereview.chromium.org/3018603002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py (right): https://codereview.chromium.org/3018603002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py:11: self._websocket.RegisterDomain('ServiceWorker', self._OnNotification) Should we just enable ServiceWorker here?
Description was changed from ========== Add tab.EnableServiceWorker() and tab.StopAllServiceWorkers() This CL adds two methods EnableServiceWorker() and StopAllServiceWorkers() to tab.py . These methods call devtools method. Also madeInspectorServiceWorker class and use websocket connections logic with devtool ServiceWorker domain in it. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add tab.StopAllServiceWorkers() This CL adds two methods StopAllServiceWorkers() to tab.py . Also made InspectorServiceWorker class and use websocket connections logic with devtool ServiceWorker domain in it. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ==========
Description was changed from ========== Add tab.StopAllServiceWorkers() This CL adds two methods StopAllServiceWorkers() to tab.py . Also made InspectorServiceWorker class and use websocket connections logic with devtool ServiceWorker domain in it. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add tab.StopAllServiceWorkers() This CL adds a method StopAllServiceWorkers() to tab.py . In order to implement this method, I also made InspectorServiceWorker class and use websocket connections logic with devtool ServiceWorker domain in it. ServiceWorker backend is enabled for all InspectorBackends on initializing InspectorServiceWorker (with devtools method ServiceWorker.enable). Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ==========
PTAL https://codereview.chromium.org/3018603002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py (right): https://codereview.chromium.org/3018603002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py:83: self._serviceworker.Enable(timeout) On 2017/09/25 02:29:25, kouhei (in TOK) wrote: > This enables ServiceWorker backend for all InspectorBackends. > I think this is acceptable given its low overhead. > > Would you note this behavioral change in CL desc? Done. https://codereview.chromium.org/3018603002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py (right): https://codereview.chromium.org/3018603002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py:11: self._websocket.RegisterDomain('ServiceWorker', self._OnNotification) On 2017/09/25 02:29:25, kouhei (in TOK) wrote: > Should we just enable ServiceWorker here? Done.
https://codereview.chromium.org/3018603002/diff/100001/telemetry/telemetry/in... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py (right): https://codereview.chromium.org/3018603002/diff/100001/telemetry/telemetry/in... telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py:12: # This method must be called before calling any other methods in s/This method/ServiceWorker.enable RPC/
PTAL https://codereview.chromium.org/3018603002/diff/100001/telemetry/telemetry/in... File telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py (right): https://codereview.chromium.org/3018603002/diff/100001/telemetry/telemetry/in... telemetry/telemetry/internal/backends/chrome_inspector/inspector_serviceworker.py:12: # This method must be called before calling any other methods in On 2017/09/25 04:59:32, kouhei (in TOK) wrote: > s/This method/ServiceWorker.enable RPC/ Done.
lgtm
On 2017/09/25 06:37:35, nednguyen wrote: > lgtm Please watch out for regression. If the overhead of always enabling inspector for SW is high, we should have an explicit method to enabling it.
lgtm with a nit https://codereview.chromium.org/3018603002/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js (right): https://codereview.chromium.org/3018603002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:10: * After tracking service worker events and catching notifications are enabled Nit: I think this sentence should be in TODO comment. TODO: Move the ServiceWorker state operations to InspectorServiceWorker by implementing InspectorServiceWorker._OnNotification.
Thanks horo@ ! kouhei@ does this CL look OK? https://codereview.chromium.org/3018603002/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js (right): https://codereview.chromium.org/3018603002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:10: * After tracking service worker events and catching notifications are enabled On 2017/09/26 00:13:11, horo wrote: > Nit: > I think this sentence should be in TODO comment. > > TODO: Move the ServiceWorker state operations to InspectorServiceWorker by > implementing InspectorServiceWorker._OnNotification. Done.
lgtm % below comment https://codereview.chromium.org/3018603002/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js (right): https://codereview.chromium.org/3018603002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:10: * TODO: Move the ServiceWorker state operations to InspectorServiceWorker by Would you file a crbug about this and make this TODO(crbug.com/123)? Assign shimazu-san as the owner of the bug.
Thanks all! I will commit this CL:D https://codereview.chromium.org/3018603002/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js (right): https://codereview.chromium.org/3018603002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:10: * TODO: Move the ServiceWorker state operations to InspectorServiceWorker by On 2017/09/26 05:04:15, kouhei (in TOK) wrote: > Would you file a crbug about this and make this TODO(crbug.com/123)? > Assign shimazu-san as the owner of the bug. Done.
The CQ bit was checked by yukiy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org, nednguyen@google.com, horo@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/3018603002/#ps150001 (title: "edit comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 150001, "attempt_start_ts": 1506404281811210,
"parent_rev": "030de5515608e2730c405575bf4a7eb189634f6c", "commit_rev":
"639e972bf15c206a77decda169e022c41c5aae36"}
Message was sent while issue was closed.
Description was changed from ========== Add tab.StopAllServiceWorkers() This CL adds a method StopAllServiceWorkers() to tab.py . In order to implement this method, I also made InspectorServiceWorker class and use websocket connections logic with devtool ServiceWorker domain in it. ServiceWorker backend is enabled for all InspectorBackends on initializing InspectorServiceWorker (with devtools method ServiceWorker.enable). Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add tab.StopAllServiceWorkers() This CL adds a method StopAllServiceWorkers() to tab.py . In order to implement this method, I also made InspectorServiceWorker class and use websocket connections logic with devtool ServiceWorker domain in it. ServiceWorker backend is enabled for all InspectorBackends on initializing InspectorServiceWorker (with devtools method ServiceWorker.enable). Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 Review-Url: https://chromiumcodereview.appspot.com/3018603002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
