New issue
Advanced search Search tips

Issue 855952 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 11
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Revamp start breakdown UMA and IPCs

Project Member Reported by falken@chromium.org, Jun 25 2018

Issue description

Service worker startup has multiple IPCs intended to help log breakdown UMA and other things. We can mostly simplify these and record milestones in the renderer, then pass the milestone information in a single callback  IPC.

More detailed doc:
https://docs.google.com/document/d/18LzAAO7YFTMc2v6Nrr1EPypXd9CDzSTQTUyMGUPZieQ/edit?usp=sharing
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e5bca9a7ce3d474445f1b4fbdd8993a7349c918c

commit e5bca9a7ce3d474445f1b4fbdd8993a7349c918c
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jun 25 23:32:50 2018

service worker: Deprecate some UMAs about script load timing.

This removes:
(a) EmbeddedWorkerInstance.Start.TimeToURLJob
    This was recorded only for installed workers. But since script
    streaming launched in M64, a URL job is not created for installed
    workers.
(b) EmbeddedWorkerInstance.Start.TimeToLoad.InstalledScript
    We already haven't been recording this since M64, due to script
    streaming.
(c) EmbeddedWorkerInstance.Start.TimeToLoad.HttpCache
(d) EmbeddedWorkerInstance.Start.TimeToLoad.Network
    These were recorded for new workers. But they rely on URLRequestJob
    to be recorded, and the path isn't taken in NetS13nServiceWorker or
    NetworkService. If we want these, we can re-add them later. It'd be
    more general to record them directly in Blink using Resource Timing.

Note that this removed a call to UpdateStepTime() which would appear
to change UMA timings. However it's OK because:
* for installed service workers: we weren't updating step time anyway
* for non-installed service workers: the only UMA logged using step
time was RecordTimeToLoad, which is being removed in this CL.

Bug:  855952 
Change-Id: Ia7e610565e2c50fc220212dda8aa23f2f15dc23b
Reviewed-on: https://chromium-review.googlesource.com/1113080
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570238}
[modify] https://crrev.com/e5bca9a7ce3d474445f1b4fbdd8993a7349c918c/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/e5bca9a7ce3d474445f1b4fbdd8993a7349c918c/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/e5bca9a7ce3d474445f1b4fbdd8993a7349c918c/content/browser/service_worker/service_worker_context_request_handler.cc
[modify] https://crrev.com/e5bca9a7ce3d474445f1b4fbdd8993a7349c918c/content/browser/service_worker/service_worker_metrics.cc
[modify] https://crrev.com/e5bca9a7ce3d474445f1b4fbdd8993a7349c918c/content/browser/service_worker/service_worker_metrics.h
[modify] https://crrev.com/e5bca9a7ce3d474445f1b4fbdd8993a7349c918c/tools/metrics/histograms/histograms.xml

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f87b17fa9e07b0b294845479d9023628b489524d

commit f87b17fa9e07b0b294845479d9023628b489524d
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jun 26 03:10:35 2018

service worker: Partially remove OnScriptLoaded/Failed IPC and UMA.

This CL:
- Removes ServiceWorker.ScriptLoadSuccess UMA. It was only recorded
  on success, since script streaming launched in M64. We were
  recording it in DidLoadInstalledScript (the success case for
  installed script loads). There was a callsite of script load failed in
  ServiceWorkerContextClient::WorkerContextFailedToStart, but we seem
  to abort start in the browser process before reaching that point,
  when script streaming fails to load from storage.
- Removes OnScriptLoadFailed() IPC. It was only used for the UMA,
  removed above.
- Removes OnScriptLoaded() IPC for installed worker scripts. For
  installed worker scripts, it was only used for the UMA removed above.
  We still need it for non-installed worker scripts for
  ResumeAfterDownload() after the byte-to-byte update check.

Bug:  855952 
Change-Id: I51c9da88eb8265fd1e6ce89972578408ec74d364
Reviewed-on: https://chromium-review.googlesource.com/1113089
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570319}
[modify] https://crrev.com/f87b17fa9e07b0b294845479d9023628b489524d/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/f87b17fa9e07b0b294845479d9023628b489524d/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/f87b17fa9e07b0b294845479d9023628b489524d/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/f87b17fa9e07b0b294845479d9023628b489524d/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/f87b17fa9e07b0b294845479d9023628b489524d/content/common/service_worker/embedded_worker.mojom
[modify] https://crrev.com/f87b17fa9e07b0b294845479d9023628b489524d/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/f87b17fa9e07b0b294845479d9023628b489524d/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/f87b17fa9e07b0b294845479d9023628b489524d/third_party/blink/renderer/modules/exported/web_embedded_worker_impl.cc
[modify] https://crrev.com/f87b17fa9e07b0b294845479d9023628b489524d/tools/metrics/histograms/histograms.xml

Comment 3 by falken@chromium.org, Jun 26 2018

I'm planning to remove the WaitedForRendererSetup UMA as it hasn't been actionable and the start timings will be changed anyway.

For reference, the current metrics are:

Android Stable 67
WaitedForRendererSetup: ~50% waited
WaitedForRendererSetup.Time: 350/1400/2700 (50p/95p/99p)
StartMessageLatency: 130/1000/2200 (50p/95p/99p)

Desktop Stable 67
WaitedForRendererSetup: ~15% waited
WaitedForRendererSetup.Time: 200/2300/6000 (50p/95p/99p)
StartMessageLatency: 10/550/2700 (50p/95p/99p)

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/caedf1b27e4463bc11b4c9b83baac63ef9601b5f

commit caedf1b27e4463bc11b4c9b83baac63ef9601b5f
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Jun 27 00:54:32 2018

service worker: Remove WaitedForRendererSetup UMA.

We didn't get much value out of this UMA. The UMA for start worker
timings will also be changing so it's easiest to just remove this now.

It was intended to distinguish between time spent waiting for Blink to
setup and time spent waiting for the StartWorker IPC to arrive on the
main thread. I've updated the bug with the latest data.

Bug:  855952 
Change-Id: Id8524271f0919c7c6a004145afc068c2ae477067
Reviewed-on: https://chromium-review.googlesource.com/1114534
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570598}
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/browser/service_worker/service_worker_metrics.cc
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/browser/service_worker/service_worker_metrics.h
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/browser/service_worker/service_worker_metrics_unittest.cc
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/common/service_worker/embedded_worker.mojom
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/renderer/render_thread_impl.h
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/renderer/service_worker/embedded_worker_instance_client_impl.cc
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/renderer/service_worker/embedded_worker_instance_client_impl.h
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/caedf1b27e4463bc11b4c9b83baac63ef9601b5f/tools/metrics/histograms/histograms.xml

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/947a26dc85a8538d9e34b327a248ec2ad0f63061

commit 947a26dc85a8538d9e34b327a248ec2ad0f63061
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Jun 27 07:25:43 2018

service worker: Remove unused function ServiceWorkerMetrics::TimeToURLJob

Follow-up to r570238, https://crrev.com/e5bca9a7ce3d474

Bug:  855952 
Change-Id: Id75e3e16a28fe53234d18b9ee04bb08c633d175a
TBR: shimazu
Reviewed-on: https://chromium-review.googlesource.com/1116416
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570685}
[modify] https://crrev.com/947a26dc85a8538d9e34b327a248ec2ad0f63061/content/browser/service_worker/service_worker_metrics.cc
[modify] https://crrev.com/947a26dc85a8538d9e34b327a248ec2ad0f63061/content/browser/service_worker/service_worker_metrics.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ef911d1b35976dca11de3be58cd473cbdd19dc0

commit 2ef911d1b35976dca11de3be58cd473cbdd19dc0
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri Jun 29 06:56:34 2018

service worker: Add more start worker timings from the renderer.

This adds UMA for start worker breakdown recorded from the renderer.
The plan is to replace the UMA that's recorded on the browser based
on IPCs from the renderer. Some motivations include:
* Avoid skewing the metrics when the IPCs get delayed due to busy
browser IO thread.
* Avoid delaying end-to-end start time. Each IPC is a chance to get
delayed, which delays end-to-end start time.
* Simplify the code.

The timing milestones are:
* start
* browser sent start worker IPC
* renderer received start worker IPC
* JavaScript evaluation on the worker thread started
* JavaScript evaluation on the worker thread ended
* end

The current implementation does more things, e.g., after
JS evaluation end it posts another task to itself and then sends
the worker started IPC, but this is expected to be removed
so it's not added as a milestone here.

The UMA added includes:
* Total startup time:
  ServiceWorker.StartTiming.Duration
* Start to milestone:
  ServiceWorker.StartTiming.StartToReceivedStartWorker
  ServiceWorker.StartTiming.StartToScriptEvaluationEnd
  ServiceWorker.StartTiming.StartToScriptEvaluationStart
  ServiceWorker.StartTiming.StartToSentStartWorker
* Each milestone:
  ServiceWorker.StartTiming.SentStartWorkerToReceivedStartWorker
  ServiceWorker.StartTiming.ReceivedStartWorkerToScriptEvaluationStart
  ServiceWorker.StartTiming.ScriptEvaluationStartToScriptEvaluationEnd
  ServiceWorker.StartTiming.ScriptEvaluationEndToEnd

See also:
https://docs.google.com/document/d/18LzAAO7YFTMc2v6Nrr1EPypXd9CDzSTQTUyMGUPZieQ/edit?usp=sharing

Change-Id: Ic6d0de634ca160518bcd6600ec3b22aa067ab086
Bug:  855952 
Reviewed-on: https://chromium-review.googlesource.com/1114565
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571415}
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/browser/service_worker/service_worker_metrics.cc
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/browser/service_worker/service_worker_metrics.h
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/browser/service_worker/service_worker_metrics_unittest.cc
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/common/service_worker/embedded_worker.mojom
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/renderer/service_worker/embedded_worker_instance_client_impl.cc
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/content/renderer/service_worker/service_worker_context_client_unittest.cc
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/third_party/blink/public/web/modules/serviceworker/web_service_worker_context_client.h
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope_proxy.cc
[modify] https://crrev.com/2ef911d1b35976dca11de3be58cd473cbdd19dc0/tools/metrics/histograms/histograms.xml

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ce9ab3a95813fd78246ac37a6548ed32b1cb2c16

commit ce9ab3a95813fd78246ac37a6548ed32b1cb2c16
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jul 02 10:49:47 2018

service worker: Skip recording timing when wait for debugger is on.

We were skipping recording for the legacy EmbeddedWorkerInstance
metrics but not the ServiceWorker.StartTiming.* metrics.

It doesn't look like there's been a big effect regardless.

Bug:  855952 
Change-Id: I4f2804e45e75b6fa4f7bded732e7e472e2033bdf
Reviewed-on: https://chromium-review.googlesource.com/1122060
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571862}
[modify] https://crrev.com/ce9ab3a95813fd78246ac37a6548ed32b1cb2c16/content/browser/service_worker/embedded_worker_instance.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c8598b62e3c4523bf6582281f72a976138b62313

commit c8598b62e3c4523bf6582281f72a976138b62313
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jul 03 04:58:39 2018

service worker: Remove some EmbeddedWorker* histograms.

These are obsolete by the ServiceWorker.StartTiming.* ones.

The motivation is to make further code changes easier, like removing
some of the IPC messages.

Bug:  855952 
Change-Id: I8ed8266fdef1641f74011c2fa6889908c889fa0f
Reviewed-on: https://chromium-review.googlesource.com/1122676
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572112}
[modify] https://crrev.com/c8598b62e3c4523bf6582281f72a976138b62313/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/c8598b62e3c4523bf6582281f72a976138b62313/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/c8598b62e3c4523bf6582281f72a976138b62313/content/browser/service_worker/service_worker_metrics.cc
[modify] https://crrev.com/c8598b62e3c4523bf6582281f72a976138b62313/content/browser/service_worker/service_worker_metrics.h
[modify] https://crrev.com/c8598b62e3c4523bf6582281f72a976138b62313/tools/metrics/histograms/histograms.xml

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c487f84c42de2f41e6d2ed0c13106d98f219e70d

commit c487f84c42de2f41e6d2ed0c13106d98f219e70d
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jul 09 04:20:07 2018

service worker: Remove OnScriptEvaluated.

Merge the IPC into OnStarted to simplify the Mojo interface and
implementation.

Bug:  855952 
Change-Id: I0e11d66b9473115da637f900f619bafc7ef4f82a
Reviewed-on: https://chromium-review.googlesource.com/1127900
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573214}
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/browser/service_worker/embedded_worker_test_helper.h
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/browser/service_worker/service_worker_job_unittest.cc
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/common/service_worker/embedded_worker.mojom
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/third_party/blink/common/service_worker/service_worker_type_converters.cc
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/third_party/blink/public/common/service_worker/service_worker_type_converters.h
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/third_party/blink/public/mojom/service_worker/service_worker_event_status.mojom
[modify] https://crrev.com/c487f84c42de2f41e6d2ed0c13106d98f219e70d/tools/metrics/histograms/enums.xml

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/49bddca6d321e80ed065152f7ef06ee9dc11ac2f

commit 49bddca6d321e80ed065152f7ef06ee9dc11ac2f
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jul 09 09:12:32 2018

service worker: Remove unused thread_id and process_id from observer callbacks.

Bug:  855952 
Change-Id: Iff014cd1c5f280285cc2cf07e1dda27bf424801a
Reviewed-on: https://chromium-review.googlesource.com/1128680
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573249}
[modify] https://crrev.com/49bddca6d321e80ed065152f7ef06ee9dc11ac2f/content/browser/resources/service_worker/serviceworker_internals.js
[modify] https://crrev.com/49bddca6d321e80ed065152f7ef06ee9dc11ac2f/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/49bddca6d321e80ed065152f7ef06ee9dc11ac2f/content/browser/service_worker/service_worker_context_core.cc
[modify] https://crrev.com/49bddca6d321e80ed065152f7ef06ee9dc11ac2f/content/browser/service_worker/service_worker_context_core_observer.h
[modify] https://crrev.com/49bddca6d321e80ed065152f7ef06ee9dc11ac2f/content/browser/service_worker/service_worker_context_watcher.cc
[modify] https://crrev.com/49bddca6d321e80ed065152f7ef06ee9dc11ac2f/content/browser/service_worker/service_worker_context_watcher.h
[modify] https://crrev.com/49bddca6d321e80ed065152f7ef06ee9dc11ac2f/content/browser/service_worker/service_worker_context_watcher_unittest.cc
[modify] https://crrev.com/49bddca6d321e80ed065152f7ef06ee9dc11ac2f/content/browser/service_worker/service_worker_internals_ui.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb

commit c97713c410a8094e2e9ea05fb09cb2770d7c7ddb
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jul 10 14:32:56 2018

service worker: Change OnThreadStarted to OnScriptEvaluationStart.

The OnThreadStarted IPC was used to start the ping/pong protocol
to stop service worker from running synchronous script for
too long. However it wasn't necessarily being called when script
evaluation starts, since script streaming may still be happening.

This CL replaces OnThreadStarted with OnScriptEvaluationStart, called
at script evaluation start.

Bug:  855952 
Change-Id: I6db8d56629a7f71a02025d1c6f9eced7e08affff
Reviewed-on: https://chromium-review.googlesource.com/1128790
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573717}
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/BUILD.gn
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/embedded_worker_test_helper.h
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/service_worker_job_unittest.cc
[add] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/service_worker_ping_controller.cc
[add] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/service_worker_ping_controller.h
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/browser/service_worker/service_worker_version_unittest.cc
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/common/service_worker/embedded_worker.mojom
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/c97713c410a8094e2e9ea05fb09cb2770d7c7ddb/tools/metrics/histograms/enums.xml

Status: Fixed (was: Started)
I think this is the most I intend to do with this. Future ideas:
* Remove OnReadyForInspection when the shadow page is gone.
* Remove OnStarted and make it a callback on Start().
* Remove OnStopped in favor of Mojo binding disconnection.
* Unify all the "SW worker host" interfaces/classes into a single one: there is EmbeddedWorkerInstance, ServiceWorkerVersion, and ServiceWorkerProviderHost. I'd prefer ServiceWorkerProviderHost (or some new class carved out of it) be used as the SWHost, and SWVersion be used for persistent storage/lifetime concerns, and EWI could hopefully be merged somewhere else.
Labels: M-69

Sign in to add a comment