Add WorkerScriptLoaded message to support attaching DevTools while starting ServiceWorker.

Add WorkerScriptLoaded message to support attaching DevTools while starting ServiceWorker.

EmbeddedWorkerHostMsg_WorkerStarted is sent after the worker script has been evaluated.
But if the break point is set in the top-level script, the worker thread is blocked and the message will not be sent.

To let the browser process know that the script has been loaded and the worker thread has started, the renderer process need to send EmbeddedWorkerHostMsg_WorkerScriptLoaded.

BUG=358657
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268431

[horo, 2014-05-02 07:02:57]

kinuko@
Could you please review?

[kinuko, 2014-05-02 07:27:10]

lgtm

https://codereview.chromium.org/268753004/diff/1/content/browser/service_work...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/268753004/diff/1/content/browser/service_work...
content/browser/service_worker/embedded_worker_instance.h:128: // its
WorkerGlobalScope is actually started on |thread_id| in the
would be better to add 'and evaluated' part?

...is actually started and evaluated on |thread_id|...

https://codereview.chromium.org/268753004/diff/1/content/renderer/service_wor...
File content/renderer/service_worker/embedded_worker_context_client.cc (right):

https://codereview.chromium.org/268753004/diff/1/content/renderer/service_wor...
content/renderer/service_worker/embedded_worker_context_client.cc:135:
WorkerContextDestroyed(embedded_worker_id_);
While you're there... it might be better to send WorkerScriptLoadFailed here?
(For symmetricity, and also we'll probably want to distinguish 'just stopped'
status and 'failed to load' status in a near future. For now it can just
dispatch OnStopped in the browser side, with TODO comment to handle it better)

[kinuko, 2014-05-02 07:29:04]

https://codereview.chromium.org/268753004/diff/1/content/browser/service_work...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/268753004/diff/1/content/browser/service_work...
content/browser/service_worker/embedded_worker_instance.h:128: // its
WorkerGlobalScope is actually started on |thread_id| in the
On 2014/05/02 07:27:10, kinuko wrote:
> would be better to add 'and evaluated' part?
> 
> ...is actually started and evaluated on |thread_id|...

evaluated -> 'parsed' would be better to be consistent with the spec

[horo, 2014-05-02 08:04:56]

https://codereview.chromium.org/268753004/diff/1/content/browser/service_work...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/268753004/diff/1/content/browser/service_work...
content/browser/service_worker/embedded_worker_instance.h:128: // its
WorkerGlobalScope is actually started on |thread_id| in the
On 2014/05/02 07:29:05, kinuko wrote:
> On 2014/05/02 07:27:10, kinuko wrote:
> > would be better to add 'and evaluated' part?
> > 
> > ...is actually started and evaluated on |thread_id|...
> 
> evaluated -> 'parsed' would be better to be consistent with the spec
> 

Done.

https://codereview.chromium.org/268753004/diff/1/content/renderer/service_wor...
File content/renderer/service_worker/embedded_worker_context_client.cc (right):

https://codereview.chromium.org/268753004/diff/1/content/renderer/service_wor...
content/renderer/service_worker/embedded_worker_context_client.cc:135:
WorkerContextDestroyed(embedded_worker_id_);
On 2014/05/02 07:27:10, kinuko wrote:
> While you're there... it might be better to send WorkerScriptLoadFailed here?
> (For symmetricity, and also we'll probably want to distinguish 'just stopped'
> status and 'failed to load' status in a near future. For now it can just
> dispatch OnStopped in the browser side, with TODO comment to handle it better)

Done.

[horo, 2014-05-02 08:05:52]

nasko@
Could you please review embedded_worker_messages.h?

[nasko, 2014-05-02 16:24:46]

LGTM once the one issue I pointed out is fixed.

https://codereview.chromium.org/268753004/diff/20001/content/browser/service_...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/268753004/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:94:
DCHECK_EQ(found->second->process_id(), process_id);
What are the implications of this check failing? We shouldn't be trusting the
renderer to tell us the right thing, so this should be an actual if statement
with clear action when expectations fail.

[horo, 2014-05-06 00:26:48]

https://codereview.chromium.org/268753004/diff/20001/content/browser/service_...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/268753004/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:94:
DCHECK_EQ(found->second->process_id(), process_id);
On 2014/05/02 16:24:46, nasko wrote:
> What are the implications of this check failing? We shouldn't be trusting the
> renderer to tell us the right thing, so this should be an actual if statement
> with clear action when expectations fail.

Done.

[horo, 2014-05-06 00:26:58]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-06 00:27:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/268753004/70001

[commit-bot: I haz the power, 2014-05-06 05:13:18]

Change committed as 268431