Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3012573002:
Add tab.IsServiceWorkerReadyOrNotRegist() which tells whether serviceworker registration is finished (Closed)
DescriptionAdd tab.IsServiceWorkerReadyOrNotRegistered() which tells whether serviceworker registration is finished
This CL add tab.IsServiceWorkerReadyOrNotRegistered() to check if ServiceWorker registration are completed.
After this CL submitted, IsServiceWorkerReadyOrNotRegistered() will be used in PageCyclerStory.RunPageInteractions().
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/3012573002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/71a6fd36ee38adf53de1293ebc04099dac6b4911
Patch Set 1 #
Total comments: 5
Patch Set 2 : added IsServiceWorkerReady #
Total comments: 6
Patch Set 3 : separate js file #Patch Set 4 : add test #
Total comments: 4
Patch Set 5 : add QueryServiceWorkerState() and IsServiceWorkerReadyOrNotRegistered() #Patch Set 6 : modify method name and edit comments #
Total comments: 2
Patch Set 7 : add comments #Patch Set 8 : fix presubmit error #
Total comments: 14
Messages
Total messages: 44 (23 generated)
The CQ bit was checked by yukiy@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
yukiy@google.com changed reviewers: + kouhei@google.com, ksakamoto@google.com, nednguyen@google.com
This CL is for perf benchmark for PWAs. PTAL
kouhei@chromium.org changed reviewers: + kouhei@chromium.org - kouhei@google.com
https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/browser/network_quiescence.js (right): https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/browser/network_quiescence.js:98: if (isServiceworkerRegistered && !isServiceworkerReady) { I think this isn't part of networkQuiescence thus I believe we should have a separate function to query this. window.__telemetry_isNewServiceWorkerReady = () => isServiceworkerRegistered && !isServiceWorkerReady; or something like that. https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/browser/web_contents.py (right): https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/browser/web_contents.py:232: self._quiescence_js + '; window.__telemetry_prepareForNavigate(); ' please create a self._prepare_for_navigate_js which contains the " window.__telemetry_prepareForNavigate(); " string. and don't inline this here.
On 2017/08/30 07:16:13, kouhei (in TOK) wrote: > https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... > File telemetry/telemetry/internal/browser/network_quiescence.js (right): > > https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/browser/network_quiescence.js:98: if > (isServiceworkerRegistered && !isServiceworkerReady) { > I think this isn't part of networkQuiescence thus I believe we should have a > separate function to query this. > window.__telemetry_isNewServiceWorkerReady = () => isServiceworkerRegistered && > !isServiceWorkerReady; > or something like that. > > https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... > File telemetry/telemetry/internal/browser/web_contents.py (right): > > https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/browser/web_contents.py:232: self._quiescence_js + > '; window.__telemetry_prepareForNavigate(); ' > please create a self._prepare_for_navigate_js which contains the " > window.__telemetry_prepareForNavigate(); " string. and don't inline this here. +1 to separate methods here as well. Probably WaitForServiceWorkerReady
Thank you for your advice! PTAL https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/browser/network_quiescence.js (right): https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/browser/network_quiescence.js:98: if (isServiceworkerRegistered && !isServiceworkerReady) { On 2017/08/30 07:16:12, kouhei (in TOK) wrote: > I think this isn't part of networkQuiescence thus I believe we should have a > separate function to query this. > window.__telemetry_isNewServiceWorkerReady = () => isServiceworkerRegistered && > !isServiceWorkerReady; > or something like that. Done. Should I divide js file? https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/browser/web_contents.py (right): https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/browser/web_contents.py:232: self._quiescence_js + '; window.__telemetry_prepareForNavigate(); ' On 2017/08/30 07:16:13, kouhei (in TOK) wrote: > please create a self._prepare_for_navigate_js which contains the " > window.__telemetry_prepareForNavigate(); " string. and don't inline this here. Done.
Description was changed from ========== Wait ServiceWorker registration before making hasReachedQuiesence true This CL make __telemetry_testHasReachedNetworkQuiescence() wait for ServiceWorker registration to be completed before making hasReachedQuiesence true. PageCyclerStory.RunPageInteractions() waits until hasReachedQuiesence to be true, so after this CL submitted, it will waits ServiceWorker too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=catapult:736697 ========== to ========== Wait ServiceWorker registration before making hasReachedQuiesence true This CL make __telemetry_testHasReachedNetworkQuiescence() wait for ServiceWorker registration to be completed before making hasReachedQuiesence true. PageCyclerStory.RunPageInteractions() waits until hasReachedQuiesence to be true, so after this CL submitted, it will waits ServiceWorker too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ==========
ksakamoto@chromium.org changed reviewers: + ksakamoto@chromium.org
https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/browser/network_quiescence.js (right): https://codereview.chromium.org/3012573002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/browser/network_quiescence.js:98: if (isServiceworkerRegistered && !isServiceworkerReady) { On 2017/08/31 02:23:04, yukiy wrote: > On 2017/08/30 07:16:12, kouhei (in TOK) wrote: > > I think this isn't part of networkQuiescence thus I believe we should have a > > separate function to query this. > > window.__telemetry_isNewServiceWorkerReady = () => isServiceworkerRegistered > && > > !isServiceWorkerReady; > > or something like that. > > Done. > > Should I divide js file? +1 for separate js file. https://codereview.chromium.org/3012573002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/network_quiescence.js (right): https://codereview.chromium.org/3012573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/network_quiescence.js:17: window.__telemetry_isNewServiceWorkerReady) { This is just making sure the following code will not run more than once, so you don't need to test them all. https://codereview.chromium.org/3012573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/network_quiescence.js:65: window.__telemetry_prepareForNavigate = function() { Does this need to be a function? We can just patch navigator.serviceWorker here, like we're doing for window.performance above. https://codereview.chromium.org/3012573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/network_quiescence.js:130: (isServiceworkerRegistered && isServiceWorkerReady)); Check for isServiceworkerRegistered is redundant. !A || (A && B) === !A || B
I separated js file. PTAL https://codereview.chromium.org/3012573002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/network_quiescence.js (right): https://codereview.chromium.org/3012573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/network_quiescence.js:17: window.__telemetry_isNewServiceWorkerReady) { On 2017/08/31 04:53:03, Kunihiko Sakamoto wrote: > This is just making sure the following code will not run more than once, so you > don't need to test them all. Done. https://codereview.chromium.org/3012573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/network_quiescence.js:65: window.__telemetry_prepareForNavigate = function() { On 2017/08/31 04:53:03, Kunihiko Sakamoto wrote: > Does this need to be a function? > We can just patch navigator.serviceWorker here, like we're doing for > window.performance above. Done. https://codereview.chromium.org/3012573002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/network_quiescence.js:130: (isServiceworkerRegistered && isServiceWorkerReady)); On 2017/08/31 04:53:03, Kunihiko Sakamoto wrote: > Check for isServiceworkerRegistered is redundant. > !A || (A && B) === !A || B Done.
lgtm
On 2017/08/31 10:18:35, Kunihiko Sakamoto wrote: > lgtm Can you write test for this? You can reference existing tests in tab_unittest.py
Added ServiceWorkerTabTest. PTAL
Description was changed from ========== Wait ServiceWorker registration before making hasReachedQuiesence true This CL make __telemetry_testHasReachedNetworkQuiescence() wait for ServiceWorker registration to be completed before making hasReachedQuiesence true. PageCyclerStory.RunPageInteractions() waits until hasReachedQuiesence to be true, so after this CL submitted, it will waits ServiceWorker too. Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add tab.IsServiceWorkerReady() which tells whether serviceworker registration is finished This CL add tab.IsServiceWorkerReady() to check if ServiceWorker registration are completed. After this CL submitted, IsServiceWorkerReady() will be used in PageCyclerStory.RunPageInteractions(). Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ==========
lgtm to avoid back n forth. I defer to kouhei@ for the last stamp to approve the test. https://codereview.chromium.org/3012573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/tab_unittest.py (right): https://codereview.chromium.org/3012573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/tab_unittest.py:259: py_utils.WaitFor(self._tab.IsServiceWorkerReady, timeout=5) Can you add any assertion about service worker is ready?
https://codereview.chromium.org/3012573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js (right): https://codereview.chromium.org/3012573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:25: navigator.serviceWorker.register = function(name, options = {}) { Nit: use arrow syntax here too to be consistent.
Description was changed from ========== Add tab.IsServiceWorkerReady() which tells whether serviceworker registration is finished This CL add tab.IsServiceWorkerReady() to check if ServiceWorker registration are completed. After this CL submitted, IsServiceWorkerReady() will be used in PageCyclerStory.RunPageInteractions(). Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add tab.IsServiceWorkerReadyOrNotRegistered() which tells whether serviceworker registration is finished This CL add tab.IsServiceWorkerReadyOrNotRegistered() to check if ServiceWorker registration are completed. After this CL submitted, IsServiceWorkerReadyOrNotRegistered() will be used in PageCyclerStory.RunPageInteractions(). Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ==========
yukiy@google.com changed reviewers: - ksakamoto@chromium.org
Thank you for your lgtm, @nednguyen ! I modified js and test. PTAL https://codereview.chromium.org/3012573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/tab_unittest.py (right): https://codereview.chromium.org/3012573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/tab_unittest.py:259: py_utils.WaitFor(self._tab.IsServiceWorkerReady, timeout=5) On 2017/09/01 10:49:59, nednguyen wrote: > Can you add any assertion about service worker is ready? Done. https://codereview.chromium.org/3012573002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js (right): https://codereview.chromium.org/3012573002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:25: navigator.serviceWorker.register = function(name, options = {}) { On 2017/09/03 21:20:04, kouhei (in TOK) wrote: > Nit: use arrow syntax here too to be consistent. Done.
The CQ bit was checked by yukiy@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % comment https://codereview.chromium.org/3012573002/diff/100001/telemetry/telemetry/in... File telemetry/telemetry/internal/browser/web_contents.py (right): https://codereview.chromium.org/3012573002/diff/100001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/web_contents.py:13: class ServiceWorkerState(object): Add a comment that these strings should exactly match strings in wait_for_serviceworker_registration.js
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
Thank you! I'll submit this. https://codereview.chromium.org/3012573002/diff/100001/telemetry/telemetry/in... File telemetry/telemetry/internal/browser/web_contents.py (right): https://codereview.chromium.org/3012573002/diff/100001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/web_contents.py:13: class ServiceWorkerState(object): On 2017/09/04 05:51:50, kouhei (in TOK) wrote: > Add a comment that these strings should exactly match strings in > wait_for_serviceworker_registration.js Done.
The CQ bit was checked by yukiy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ksakamoto@chromium.org, nednguyen@google.com, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/3012573002/#ps120001 (title: "add comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
The CQ bit was checked by yukiy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, ksakamoto@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/3012573002/#ps140001 (title: "fix presubmit error")
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": 140001, "attempt_start_ts": 1504509400604870,
"parent_rev": "d4179a057d4b2410cbe1d0eef9773558faec6a7c", "commit_rev":
"71a6fd36ee38adf53de1293ebc04099dac6b4911"}
Message was sent while issue was closed.
Description was changed from ========== Add tab.IsServiceWorkerReadyOrNotRegistered() which tells whether serviceworker registration is finished This CL add tab.IsServiceWorkerReadyOrNotRegistered() to check if ServiceWorker registration are completed. After this CL submitted, IsServiceWorkerReadyOrNotRegistered() will be used in PageCyclerStory.RunPageInteractions(). Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 ========== to ========== Add tab.IsServiceWorkerReadyOrNotRegistered() which tells whether serviceworker registration is finished This CL add tab.IsServiceWorkerReadyOrNotRegistered() to check if ServiceWorker registration are completed. After this CL submitted, IsServiceWorkerReadyOrNotRegistered() will be used in PageCyclerStory.RunPageInteractions(). Design doc of perf benchmark for PWA: https://docs.google.com/document/d/1Nf97CVp1X7aSqvAspyJ7yOCDyr1osUNrnfrGwZ_Yu... BUG=chromium:736697 Review-Url: https://chromiumcodereview.appspot.com/3012573002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
falken@chromium.org changed reviewers: + falken@chromium.org
Message was sent while issue was closed.
Sorry, I have some post-commit comments. (Feel free to send me any of your CLs.) https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/browser/tab_unittest.py (right): https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/tab_unittest.py:258: 'if ("serviceWorker" in navigator) { \ Just curious: do we run telemetry tests on environments that don't support service worker? All Chrome browsers except for iOS support service worker. https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/tab_unittest.py:259: navigator.serviceWorker.register("{{ @scriptURL }}") };', Looks like the semi-colon should go after the register() call, not after the if {} brace. https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js (right): https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:17: // These variable is used for detecting and waiting service worker "variable is" => "variables are", "waiting" => "waiting for" https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:22: // Patch navigator.serviceWorker.register I'm not familiar with telemetry style, but probably need a '.' at the end of the sentence. https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:34: // ServiceWorkerState, in web_contents.py ditto with '.' https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:36: return isServiceWorkerReady ? 'unknown state' : 'not registered'; maybe 'unknown state' => 'error: ready but register() was not called' I guess we're assuming this won't happen in our test cases. But maybe it'd be clearer with a string like this. https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/wait_for_serviceworker_registration.js:38: return isServiceWorkerReady ? 'activated' : 'installing'; 'activated' is slightly misleading since when .ready resolve, the service worker is the "active" worker but it's not necessarily "activated" yet (yes, very confusing). It reaches "activated" after it finishes the 'activate' event handler, and .ready resolves right before that gets dispatched. I'd prefer: isServiceWorkerReady ? 'active' : 'installing' This is more consistent with the properties on ServiceWorkerRegistration: https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration To be really precise, we could say 'installing_or_waiting', but as long as there's not an 'active' worker, any 'waiting' worker becomes 'active' immediately. And it seems we don't care about ensuring that the service worker installed by *this* register() call became active, as long as there's an active worker we're happy. https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/browser/web_contents.py (right): https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/web_contents.py:15: # wait_for_serviceworker_registration.js Add some docu: # The page did not call register(). https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/web_contents.py:16: NOT_REGISTERED = "not registered" # The page called register(), and there is not yet an active service worker. https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/web_contents.py:17: INSTALLING = "installing" # The page called register(), and has an active service worker. https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/web_contents.py:18: ACTIVATED = "activated" if you take up my suggestion, can change this to ACTIVE https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/web_contents.py:157: "Unexpected service worker registration state.") maybe we can output 'state' as part of the exception https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/web_contents.py:159: def IsServiceWorkerActivatedOrNotRegistered(self): IsServiceWorkerActiveOrNotRegistered https://codereview.chromium.org/3012573002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/browser/web_contents.py:164: ACTIVATED state. Also returns true if the page does not register service It's a bit nitty but we're not really checking that the service worker registered from the page reached active. Suggest: "True if the page registered a service worker and has an active service worker. Also returns true if...."
Message was sent while issue was closed.
... after writing all that out, I'm wondering if we really want to wait for the
service worker to reach 'activated' instead of just become .active. The SW won't
get fetch events until it reaches 'activated'. So we might want to do:
serviceWorker.ready.then(reg => {
wait_for_state(reg.active, 'activated').then(() => {
isServiceWorkerReady = true;
});
Then you can keep the 'activated' terminology.
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://chromiumcodereview.appspot.com/3008163002/ by perezju@chromium.org. The reason for reverting is: This CL appears to be failing and blocking catapult rolls. See https://catapult-roll.skia.org/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
