DevTools: move the force update SW on reload checkbox to the Resources / Service Workers view.

DevTools: move the force update SW on reload checkbox to the Resources / Service Workers view.
Remove the "Debug on start" option from Resources / Service Workers.

Committed: https://crrev.com/63bdc4d2b130967783f25a770a95c561471b0e5f
Cr-Commit-Position: refs/heads/master@{#384347}

[pfeldman, 2016-03-30 00:51:29]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org, horo@chromium.org

[horo, 2016-03-30 05:17:35]

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(left):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:24:
var debugOnStartCheckboxLabel = createCheckboxLabel(WebInspector.UIString("Open
DevTools window and pause JavaScript execution on Service Worker startup for
debugging."));
Why do you remove this debugOnStartCheckbox?

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js
(right):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:58: }
If "force update on reload" flag is global, ServiceWorkerManager should send
"force update on reload updated" event to update the checkoboxes in multiple
DevTools windows.

[pfeldman, 2016-03-30 17:11:44]

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(left):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:24:
var debugOnStartCheckboxLabel = createCheckboxLabel(WebInspector.UIString("Open
DevTools window and pause JavaScript execution on Service Worker startup for
debugging."));
On 2016/03/30 05:17:35, horo wrote:
> Why do you remove this debugOnStartCheckbox?

It does not currently work in combination with the update on reload. What it
does is it spins off a new devtools window that tries to attach to a new SW, but
then existing DevTools window takes over. Furthermore, with the source of the SW
available, you can set a breakpoint and simply do update - this results in a
much cleaner behavior.

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js
(right):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:58: }
On 2016/03/30 05:17:35, horo wrote:
> If "force update on reload" flag is global, ServiceWorkerManager should send
> "force update on reload updated" event to update the checkoboxes in multiple
> DevTools windows.

I don't think so. This is a user setting, stored in our preferences. It should
not be controlled externally. If we want to synchronize its value, we should do
that by the front-end means. This adds quite a bit of complexity though, so I
would leave it as is and let the latter win. We are talking about edge case
here.

[dgozman, 2016-03-30 21:09:54]

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(left):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:181:
this._manager.setDebugOnStart(this._debugOnStartCheckbox.checked);
Remove it from manager, protocol and backend.

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js
(right):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:47:
this._forceUpdateSetting =
WebInspector.settings.moduleSetting("serviceWorkerUpdateOnReload");
Just createSetting(), don't declare it in module.json.

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:894:
WebInspector.ToolbarCheckbox = function(text, title, setting, updateCallback)
I don't see where you use this.

[dgozman, 2016-03-30 21:28:57]

lgtm modulo previous comments

[horo, 2016-03-31 02:39:45]

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(left):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:24:
var debugOnStartCheckboxLabel = createCheckboxLabel(WebInspector.UIString("Open
DevTools window and pause JavaScript execution on Service Worker startup for
debugging."));
On 2016/03/30 17:11:44, pfeldman wrote:
> On 2016/03/30 05:17:35, horo wrote:
> > Why do you remove this debugOnStartCheckbox?
> 
> It does not currently work in combination with the update on reload. What it
> does is it spins off a new devtools window that tries to attach to a new SW,
but
> then existing DevTools window takes over. Furthermore, with the source of the
SW
> available, you can set a breakpoint and simply do update - this results in a
> much cleaner behavior.

OK. Sounds reasonable.
Please write about this in the CL description.

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js
(right):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:58: }
On 2016/03/30 17:11:44, pfeldman wrote:
> On 2016/03/30 05:17:35, horo wrote:
> > If "force update on reload" flag is global, ServiceWorkerManager should send
> > "force update on reload updated" event to update the checkoboxes in multiple
> > DevTools windows.
> 
> I don't think so. This is a user setting, stored in our preferences. It should
> not be controlled externally. If we want to synchronize its value, we should
do
> that by the front-end means. This adds quite a bit of complexity though, so I
> would leave it as is and let the latter win. We are talking about edge case
> here.

OK.
Then please remove "DebugOnStartUpdated" from here.

[pfeldman, 2016-03-31 05:02:32]

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(left):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:181:
this._manager.setDebugOnStart(this._debugOnStartCheckbox.checked);
On 2016/03/30 21:09:54, dgozman wrote:
> Remove it from manager, protocol and backend.

Done.

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js
(right):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:47:
this._forceUpdateSetting =
WebInspector.settings.moduleSetting("serviceWorkerUpdateOnReload");
On 2016/03/30 21:09:54, dgozman wrote:
> Just createSetting(), don't declare it in module.json.

Done.

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:58: }
On 2016/03/31 02:39:45, horo wrote:
> On 2016/03/30 17:11:44, pfeldman wrote:
> > On 2016/03/30 05:17:35, horo wrote:
> > > If "force update on reload" flag is global, ServiceWorkerManager should
send
> > > "force update on reload updated" event to update the checkoboxes in
multiple
> > > DevTools windows.
> > 
> > I don't think so. This is a user setting, stored in our preferences. It
should
> > not be controlled externally. If we want to synchronize its value, we should
> do
> > that by the front-end means. This adds quite a bit of complexity though, so
I
> > would leave it as is and let the latter win. We are talking about edge case
> > here.
> 
> OK.
> Then please remove "DebugOnStartUpdated" from here.

Done.

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/1841863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:894:
WebInspector.ToolbarCheckbox = function(text, title, setting, updateCallback)
I no longer am. Done.

[pfeldman, 2016-03-31 05:59:37]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-03-31 05:59:38]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1841863003/#ps40001
(title: "for landing")

[commit-bot: I haz the power, 2016-03-31 06:00:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841863003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841863003/40001

[horo, 2016-03-31 06:17:03]

Please write about the removal of DebugOnStart checkbox in the CL description.

lgtm.

[pfeldman, 2016-03-31 06:36:31]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2016-03-31 06:36:57]

Description was changed from

==========
DevTools: move the force update SW on reload checkbox to the Resources / Service
Workers view.
==========

to

==========
DevTools: move the force update SW on reload checkbox to the Resources / Service
Workers view.
Remote the "Debug on start" option from Resources / Service Workers.
==========

[pfeldman, 2016-03-31 06:37:05]

Description was changed from

==========
DevTools: move the force update SW on reload checkbox to the Resources / Service
Workers view.
Remote the "Debug on start" option from Resources / Service Workers.
==========

to

==========
DevTools: move the force update SW on reload checkbox to the Resources / Service
Workers view.
Remove the "Debug on start" option from Resources / Service Workers.
==========

[pfeldman, 2016-03-31 06:37:12]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2016-03-31 06:37:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841863003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841863003/40001

[commit-bot: I haz the power, 2016-03-31 07:09:56]

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

[commit-bot: I haz the power, 2016-03-31 07:09:57]

Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[pfeldman, 2016-03-31 14:56:36]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-03-31 14:56:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/1841863003/#ps60001
(title: "test fixed")

[commit-bot: I haz the power, 2016-03-31 14:56:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841863003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841863003/60001

[commit-bot: I haz the power, 2016-03-31 14:58:51]

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

[commit-bot: I haz the power, 2016-03-31 14:58:53]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[pfeldman, 2016-03-31 18:16:03]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2016-03-31 18:16:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841863003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841863003/60001

[commit-bot: I haz the power, 2016-03-31 18:51:01]

Description was changed from

==========
DevTools: move the force update SW on reload checkbox to the Resources / Service
Workers view.
Remove the "Debug on start" option from Resources / Service Workers.
==========

to

==========
DevTools: move the force update SW on reload checkbox to the Resources / Service
Workers view.
Remove the "Debug on start" option from Resources / Service Workers.
==========

[commit-bot: I haz the power, 2016-03-31 18:51:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-31 18:52:28]

Description was changed from

==========
DevTools: move the force update SW on reload checkbox to the Resources / Service
Workers view.
Remove the "Debug on start" option from Resources / Service Workers.
==========

to

==========
DevTools: move the force update SW on reload checkbox to the Resources / Service
Workers view.
Remove the "Debug on start" option from Resources / Service Workers.

Committed: https://crrev.com/63bdc4d2b130967783f25a770a95c561471b0e5f
Cr-Commit-Position: refs/heads/master@{#384347}
==========

[commit-bot: I haz the power, 2016-03-31 18:52:29]

Patchset 4 (id:??) landed as
https://crrev.com/63bdc4d2b130967783f25a770a95c561471b0e5f
Cr-Commit-Position: refs/heads/master@{#384347}