Add option to open the DevTools window for ServiceWorker on start in serviceworker-internals.

Add option to open the DevTools window for ServiceWorker on start in serviceworker-internals.

BUG=358657
R=yurys@chromium.org
TBR=pfeldman@chromium.org

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

[horo, 2014-05-20 15:06:09]

kinuko@
Could you please review this?

[pfeldman, 2014-05-20 19:20:41]

not lgtm

https://codereview.chromium.org/299693002/diff/1/chrome/browser/devtools/devt...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/299693002/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_window.cc:636: bool worker_paused,
we should not pass state or capabilities-related parameters as query parameters.
Protocol should be used for that. DevTools will support several targets
(including page, its oop iframes, shared and dedicated workers and service
workers) in a single window. We can't pass any of these into the front-end url
params.

https://codereview.chromium.org/299693002/diff/1/content/public/browser/devto...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/299693002/diff/1/content/public/browser/devto...
content/public/browser/devtools_agent_host.h:73: virtual bool IsWorkerPaused()
const = 0;
This is a very strange addition to the public content API. I don't think it
belongs here.

[horo, 2014-05-21 09:05:50]

On 2014/05/20 19:20:41, pfeldman wrote:
> not lgtm
> 
>
https://codereview.chromium.org/299693002/diff/1/chrome/browser/devtools/devt...
> File chrome/browser/devtools/devtools_window.cc (right):
> 
>
https://codereview.chromium.org/299693002/diff/1/chrome/browser/devtools/devt...
> chrome/browser/devtools/devtools_window.cc:636: bool worker_paused,
> we should not pass state or capabilities-related parameters as query
parameters.
> Protocol should be used for that. DevTools will support several targets
> (including page, its oop iframes, shared and dedicated workers and service
> workers) in a single window. We can't pass any of these into the front-end url
> params.
> 
>
https://codereview.chromium.org/299693002/diff/1/content/public/browser/devto...
> File content/public/browser/devtools_agent_host.h (right):
> 
>
https://codereview.chromium.org/299693002/diff/1/content/public/browser/devto...
> content/public/browser/devtools_agent_host.h:73: virtual bool IsWorkerPaused()
> const = 0;
> This is a very strange addition to the public content API. I don't think it
> belongs here.

pfeldman@
Thank you for the review.
I changed this cl no to make change in devtools_window.* and
devtools_agent_host.*.
And I made another cl to add isPausedWorker command to Runtime's protocol.
Could you please review this? https://codereview.chromium.org/299803003/

[horo, 2014-05-23 10:24:34]

yurys@
Could you please review this?
Thank you.

[horo, 2014-05-27 10:47:08]

yurys@
ping?

[yurys, 2014-05-27 12:02:30]

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:295: if
(!info->debug_on_start())
Why is it not enough to check debug_service_worker_on_start_ here instead of
storing its value on WorkerInfo?

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.h (right):

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.h:92: explicit
WorkerInfo(const ServiceWorkerIdentifier& service_worker_id,
style nit: explicit is not needed anymore.

[horo, 2014-05-27 12:29:27]

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:295: if
(!info->debug_on_start())
On 2014/05/27 12:02:31, yurys wrote:
> Why is it not enough to check debug_service_worker_on_start_ here instead of
> storing its value on WorkerInfo?

It is because debug_service_worker_on_start_ could be changed between
ServiceWorkerCreated and WorkerContextStarted are called.

If we only check debug_service_worker_on_start_ here, in the following
situations the ServiceWorker will remain paused forever.
- The user enables debug_service_worker_on_start_.
- Start ServiceWorker.
- ServiceWorkerCreated returns true -> ServiceWorker will be started in paused
state.
- The user disables debug_service_worker_on_start_.
- ServiceWorker finishes loading script.
- WorkerContextStarted is called. -> Don't start DevTools because
debug_service_worker_on_start_ is false.

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.h (right):

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.h:92: explicit
WorkerInfo(const ServiceWorkerIdentifier& service_worker_id,
On 2014/05/27 12:02:31, yurys wrote:
> style nit: explicit is not needed anymore.

Done.

[yurys, 2014-05-27 14:22:47]

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:295: if
(!info->debug_on_start())
On 2014/05/27 12:29:27, horo wrote:
> On 2014/05/27 12:02:31, yurys wrote:
> > Why is it not enough to check debug_service_worker_on_start_ here instead of
> > storing its value on WorkerInfo?
> 
> It is because debug_service_worker_on_start_ could be changed between
> ServiceWorkerCreated and WorkerContextStarted are called.
> 
> If we only check debug_service_worker_on_start_ here, in the following
> situations the ServiceWorker will remain paused forever.
> - The user enables debug_service_worker_on_start_.
> - Start ServiceWorker.
> - ServiceWorkerCreated returns true -> ServiceWorker will be started in paused
> state.
> - The user disables debug_service_worker_on_start_.
> - ServiceWorker finishes loading script.
> - WorkerContextStarted is called. -> Don't start DevTools because
> debug_service_worker_on_start_ is false.

I see. That makes sense. I'd rather we used a new state
(WORKER_PAUSED_FOR_DEBUG_ON_START?) instead of introducing a new field on
WorkerInfo, wdyt? Otherwise LGTM.

[horo, 2014-05-28 01:02:07]

On 2014/05/27 14:22:47, yurys wrote:
>
https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
> File content/browser/devtools/embedded_worker_devtools_manager.cc (right):
> 
>
https://codereview.chromium.org/299693002/diff/30001/content/browser/devtools...
> content/browser/devtools/embedded_worker_devtools_manager.cc:295: if
> (!info->debug_on_start())
> On 2014/05/27 12:29:27, horo wrote:
> > On 2014/05/27 12:02:31, yurys wrote:
> > > Why is it not enough to check debug_service_worker_on_start_ here instead
of
> > > storing its value on WorkerInfo?
> > 
> > It is because debug_service_worker_on_start_ could be changed between
> > ServiceWorkerCreated and WorkerContextStarted are called.
> > 
> > If we only check debug_service_worker_on_start_ here, in the following
> > situations the ServiceWorker will remain paused forever.
> > - The user enables debug_service_worker_on_start_.
> > - Start ServiceWorker.
> > - ServiceWorkerCreated returns true -> ServiceWorker will be started in
paused
> > state.
> > - The user disables debug_service_worker_on_start_.
> > - ServiceWorker finishes loading script.
> > - WorkerContextStarted is called. -> Don't start DevTools because
> > debug_service_worker_on_start_ is false.
> 
> I see. That makes sense. I'd rather we used a new state
> (WORKER_PAUSED_FOR_DEBUG_ON_START?) instead of introducing a new field on
> WorkerInfo, wdyt? Otherwise LGTM.

WORKER_PAUSED_FOR_DEBUG_ON_START state sounds good.
I changed this cl to use WORKER_PAUSED_FOR_DEBUG_ON_START.

[yurys, 2014-05-28 10:54:53]

lgtm

https://codereview.chromium.org/299693002/diff/110001/content/browser/devtool...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/299693002/diff/110001/content/browser/devtool...
content/browser/devtools/embedded_worker_devtools_manager.cc:275: case
WORKER_PAUSED_FOR_DEBUG_ON_START:
case WORKER_PAUSED_FOR_DEBUG_ON_START: can be placed after case
WORKER_UNINSPECTED: and using fall through you would have workers_.erase(it); in
a single place.

[horo, 2014-05-28 11:04:17]

https://codereview.chromium.org/299693002/diff/110001/content/browser/devtool...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/299693002/diff/110001/content/browser/devtool...
content/browser/devtools/embedded_worker_devtools_manager.cc:275: case
WORKER_PAUSED_FOR_DEBUG_ON_START:
On 2014/05/28 10:54:53, yurys wrote:
> case WORKER_PAUSED_FOR_DEBUG_ON_START: can be placed after case
> WORKER_UNINSPECTED: and using fall through you would have workers_.erase(it);
in
> a single place.

Done.

[horo, 2014-05-28 11:05:57]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-28 11:06:20]

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

[commit-bot: I haz the power, 2014-05-28 11:06:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-28 11:06:22]

A disapproval has been posted by following reviewers: pfeldman@chromium.org.
Please make sure to get positive review before another CQ attempt.

[horo, 2014-05-28 11:09:25]

pfeldman@
Could you please review this again?
Thank you.

[horo, 2014-05-29 09:37:25]

The CQ bit was checked by horo@chromium.org

[horo, 2014-05-29 09:39:10]

pfeldman@ seems to be ooo.
I added TBR line to the description.

[commit-bot: I haz the power, 2014-05-29 09:39:12]

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

[commit-bot: I haz the power, 2014-05-29 09:39:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-29 09:39:15]

A disapproval has been posted by following reviewers: pfeldman@chromium.org.
Please make sure to get positive review before another CQ attempt.

[horo, 2014-05-29 09:40:53]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-29 09:43:32]

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

[commit-bot: I haz the power, 2014-05-29 09:43:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-29 09:43:35]

A disapproval has been posted by following reviewers: pfeldman@chromium.org.
Please make sure to get positive review before another CQ attempt.

[horo, 2014-05-30 08:10:36]

Committed patchset #9 manually as r273761 (presubmit successful).