Show the unregistered workers in chrome://serviceworker-internals and chrome://inspect

Show the unregistered workers in chrome://serviceworker-internals and chrome://inspect.

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

[horo, 2014-05-27 12:52:35]

falken@
Could you please review this?

[falken, 2014-05-28 08:21:40]

style nits

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
File content/browser/resources/service_worker/serviceworker_internals.js
(right):

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:6: 'use
strict';
it's late to point out now, but why does this file have 4 space indents?
shouldn't it be 2?

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:69:
live_registrations.forEach(function (registration) {
should be no space after "function"? (and throughout this file)

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:71:
registration.pending].forEach(function (version){
should be a space before "{"? (and throughout)

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:93: // Set
log for each worker versions.
This function is really filling the log for one version, right? Then it should
be "Set log for a worker version"

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:122:
stored_registrations.forEach(function (registration) {
should be no space after "function"?

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:134: });
this close paren should be aligned with its open paren line?

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

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:42: const
StatusCallback& callback);
These typedefs are duplicated in the .h file.

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:45: const
WeakPtr<ServiceWorkerInternalsUI> internals,
Chrome code doesn't seem to use const WeakPtr... I think because a WeakPtr can
be invalidated at any time. maybe drop the const

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:56: if (internals)
this if should probably have braces as the body has multiple lines

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:116: // it from
being deleted while starting the worker. It the refcount of
s/It/If/.... but why does StartWorker destroy the registration?

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:254: const
WeakPtr<ServiceWorkerInternalsUI> internals,
const WeakPtr again

[horo, 2014-05-28 09:06:39]

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
File content/browser/resources/service_worker/serviceworker_internals.js
(right):

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:6: 'use
strict';
On 2014/05/28 08:21:40, falken wrote:
> it's late to point out now, but why does this file have 4 space indents?
> shouldn't it be 2?

Done.

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:69:
live_registrations.forEach(function (registration) {
On 2014/05/28 08:21:40, falken wrote:
> should be no space after "function"? (and throughout this file)

Done.

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:71:
registration.pending].forEach(function (version){
On 2014/05/28 08:21:40, falken wrote:
> should be a space before "{"? (and throughout)

Done.

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:93: // Set
log for each worker versions.
On 2014/05/28 08:21:40, falken wrote:
> This function is really filling the log for one version, right? Then it should
> be "Set log for a worker version"

Done.

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:122:
stored_registrations.forEach(function (registration) {
On 2014/05/28 08:21:40, falken wrote:
> should be no space after "function"?

Done.

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:134: });
On 2014/05/28 08:21:40, falken wrote:
> this close paren should be aligned with its open paren line?

Done.

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

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:42: const
StatusCallback& callback);
On 2014/05/28 08:21:40, falken wrote:
> These typedefs are duplicated in the .h file.

Done.

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:45: const
WeakPtr<ServiceWorkerInternalsUI> internals,
On 2014/05/28 08:21:40, falken wrote:
> Chrome code doesn't seem to use const WeakPtr... I think because a WeakPtr can
> be invalidated at any time. maybe drop the const

Done.

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:56: if (internals)
On 2014/05/28 08:21:40, falken wrote:
> this if should probably have braces as the body has multiple lines

Done.

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:116: // it from
being deleted while starting the worker. It the refcount of
On 2014/05/28 08:21:40, falken wrote:
> s/It/If/.... but why does StartWorker destroy the registration?

Done.
If there is no ServiceWorkerHandle which has the reference of the registration,
the refcount will be 0.

https://codereview.chromium.org/304543002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_internals_ui.cc:254: const
WeakPtr<ServiceWorkerInternalsUI> internals,
On 2014/05/28 08:21:40, falken wrote:
> const WeakPtr again

Done.

[falken, 2014-05-28 16:26:06]

lgtm

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
File content/browser/resources/service_worker/serviceworker_internals.js
(right):

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:31: // All
commands are sent with the partition_path and scope, and
Is the "partition_path and scope" part of the comment now outdated?

https://codereview.chromium.org/304543002/diff/40001/content/browser/resource...
File content/browser/resources/service_worker/serviceworker_internals.html
(right):

https://codereview.chromium.org/304543002/diff/40001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.html:50:
jsvalues=".cmdArgs:{process_id:process_id,devtools_agent_route_id:devtools_agent_route_id}">Inspect</button>
html files in chrome also should adhere to 80 col limit, no? (not from your
patch so you don't have to fix this now. it's probably better to do large
style-only fixes in a separate patch for easier reviewing/diffing)

[horo, 2014-05-29 01:31:24]

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
File content/browser/resources/service_worker/serviceworker_internals.js
(right):

https://codereview.chromium.org/304543002/diff/20001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.js:31: // All
commands are sent with the partition_path and scope, and
On 2014/05/28 16:26:06, falken wrote:
> Is the "partition_path and scope" part of the comment now outdated?

Done.

https://codereview.chromium.org/304543002/diff/40001/content/browser/resource...
File content/browser/resources/service_worker/serviceworker_internals.html
(right):

https://codereview.chromium.org/304543002/diff/40001/content/browser/resource...
content/browser/resources/service_worker/serviceworker_internals.html:50:
jsvalues=".cmdArgs:{process_id:process_id,devtools_agent_route_id:devtools_agent_route_id}">Inspect</button>
On 2014/05/28 16:26:06, falken wrote:
> html files in chrome also should adhere to 80 col limit, no? (not from your
> patch so you don't have to fix this now. it's probably better to do large
> style-only fixes in a separate patch for easier reviewing/diffing)

I will do so in another cl.
http://www.chromium.org/developers/web-development-style-guide

[horo, 2014-05-29 01:34:42]

kaznacheev@
Could you please review chrome/browser/resources/inspect/inspect.js?
Thank you.

[Vladislav Kaznacheev, 2014-05-29 07:08:50]

On 2014/05/29 01:34:42, horo wrote:
> kaznacheev@
> Could you please review chrome/browser/resources/inspect/inspect.js?
> Thank you.

inspect.js LGTM

[horo, 2014-05-29 12:50:53]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-29 12:51:38]

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

[commit-bot: I haz the power, 2014-05-29 15:48:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-29 20:10:05]

Change committed as 273567