DevTools: simplify Service Workers panel UI, remove non-actionable data.

DevTools: simplify Service Workers panel UI, remove non-actionable data.

BUG=608997
Committed: https://crrev.com/f60030d12c0738084bac0a563cfc6edab2bf5b40
Cr-Commit-Position: refs/heads/master@{#391774}

[pfeldman, 2016-05-04 03:14:09]

Description was changed from

==========
DevTools: simplify Service Workers panel UI, remove non-actionable data.
==========

to

==========
DevTools: simplify Service Workers panel UI, remove non-actionable data.

BUG=608997
==========

[pfeldman, 2016-05-04 03:14:54]

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

[pfeldman, 2016-05-04 03:14:55]

This a preliminary version. Check out the mock - we trimmed a lot of non-active
version details.

[pfeldman, 2016-05-04 03:15:49]

wait, a more recent patch set 5 is coming.

[pfeldman, 2016-05-04 23:18:24]

pfeldman@chromium.org changed reviewers:
+ caseq@chromium.org, lushnikov@chromium.org

[lushnikov, 2016-05-04 23:28:07]

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/common/Throttler.js:58:
asSoonAsPossible = !!asSoonAsPossible || (!hasScheduledTasks && Date.now() -
this._lastCompleteTime > this._timeout);
This change feels risky by itself - can we please land it in a separate change?

[caseq, 2016-05-05 01:09:23]

https://codereview.chromium.org/1940993002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/1940993002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:78: if
(registration->waiting_version()) {
nit:
if (status != SERVICE_WORKER_ON || !registration->waiting_version())
    return;

https://codereview.chromium.org/1940993002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:316:
context_core_->storage()->FindRegistrationForPattern(
Let's extract something like dispatchOnRegistrationForPattern(pattern, callback)
-- the amount of boilerplate here is getting too big.

https://codereview.chromium.org/1940993002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1940993002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:175: // Skips waiting
and forces this version to become activated.
s/forces/force/

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/common/Throttler.js:58:
asSoonAsPossible = !!asSoonAsPossible || (!hasScheduledTasks && Date.now() -
this._lastCompleteTime > this._timeout);
On 2016/05/04 23:28:07, lushnikov wrote:
> This change feels risky by itself - can we please land it in a separate
change?

+1 to extract this

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css
(right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css:43:
z-index: 0;
unrelated?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js
(right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js:104:
var title = (icon["sizes"] || "") + "\n" + (icon["type"] || "");
can we expect these to be unique?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js:107:
imageElement.style.maxWidth = "200px";
please extract a style or make sure this one works?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:19:
// /** @type {!Map.<!WebInspector.ServiceWorkerRegistration,
!WebInspector.ServiceWorkersView.Section>} */
nuke //?
Also, drop the dot after Map.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:20:
this._sections = new Map();
... or put as Symbol() on the registration and you've got one less map to clean
up.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:23:
this._securityOrigins = new Set();
Drop this altogether!

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:76:
var registration = section._registration;
inline?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:140:
_addOrigin: function(securityOrigin)
remove, call _updateSectionVisibility() directly.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:149:
_securityOriginRemoved: function(event)
ditto.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:185:
this._pushButton = new WebInspector.ToolbarButton(WebInspector.UIString("Emulate
push event"), undefined, WebInspector.UIString("Push"));
let's keep toolbar items together.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:197:
WebInspector.console.log("S: " + this._registration.scopeURL);
remove this?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:224:
var title = WebInspector.UIString(this._registration.isDeleted ? "%s - deleted"
: "%s", this._registration.scopeURL);
nit:
var title = this._registration.isDeleted ?
WebInspector.UIString(this._registration.isDeleted ? "%s - deleted",
this._registration.scopeURL) : this._registration.scopeURL;

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:236:
if (active) {
This is getting long, let's extract updating active/waiting/installing into
separate methods?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:252:
var inspectButton = activeEntry.createChild("span", "link");
Let's extract button creation into a method, we do this a lot here.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:278:
skipButton.textContent = WebInspector.UIString("skipWaiting");
Skip Waiting

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:308:
var location = error.sourceURL ? "(" + error.sourceURL + ":" + error.lineNumber
+ ")" : "";
WebInspector.UIString("(%s:%d)")

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:314:
message.appendChild(createLabel("#" + error.versionId + ": " +
error.errorMessage, "error-icon"));
WebInspector.UIString()?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:377:
this._manager.skipWaiting(this._registration.scopeURL);
Let's extract this into a follow-up patch along with the protocol/backend
change.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js
(right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:495:
context.setLabel(label + " #" + this._versionId + " (" + this._status + ")");
Looks like WebInspector.UIString().. Wait.. A UIString in SDK?  😱

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:672:
this.controlledClients = []
; while you're here

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:684:
this._fingerprint = JSON.stringify(this);
Please don't. Perhaps, keep payload around and stringify it?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:802: *
@extends {WebInspector.Object}
Why?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:853:
return result;
return new Map(this.versions.values().map(version => [version.mode(), version])

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/ReportView.js (right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ReportView.js:42: createToolbar:
function()
nit: perhaps rename to toolbar() and create only once?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ReportView.js:81: /** @type
{!Map.<string, !Element>} */
Map<string, !Element>

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ReportView.js:112: var row =
this._fieldMap.get(title);
using title as a key looks a bit unfortunate...

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/inspectorCommon.css
(right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/inspectorCommon.css:145: color: 
rgb(17, 85, 204);
revert?

[pfeldman, 2016-05-05 02:21:42]

https://codereview.chromium.org/1940993002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/1940993002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:78: if
(registration->waiting_version()) {
On 2016/05/05 01:09:21, caseq wrote:
> nit:
> if (status != SERVICE_WORKER_ON || !registration->waiting_version())
>     return;

Done.

https://codereview.chromium.org/1940993002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:316:
context_core_->storage()->FindRegistrationForPattern(
On 2016/05/05 01:09:21, caseq wrote:
> Let's extract something like dispatchOnRegistrationForPattern(pattern,
callback)
> -- the amount of boilerplate here is getting too big.

It is a common pattern that SW uses to hop between threads. I agree it is
somewhat suboptimal

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:19:
// /** @type {!Map.<!WebInspector.ServiceWorkerRegistration,
!WebInspector.ServiceWorkersView.Section>} */
On 2016/05/05 01:09:22, caseq wrote:
> nuke //?
> Also, drop the dot after Map.

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:20:
this._sections = new Map();
On 2016/05/05 01:09:22, caseq wrote:
> ... or put as Symbol() on the registration and you've got one less map to
clean
> up.

Acknowledged.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:23:
this._securityOrigins = new Set();
On 2016/05/05 01:09:22, caseq wrote:
> Drop this altogether!

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:76:
var registration = section._registration;
On 2016/05/05 01:09:22, caseq wrote:
> inline?

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:140:
_addOrigin: function(securityOrigin)
On 2016/05/05 01:09:22, caseq wrote:
> remove, call _updateSectionVisibility() directly.

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:149:
_securityOriginRemoved: function(event)
On 2016/05/05 01:09:22, caseq wrote:
> ditto.

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:185:
this._pushButton = new WebInspector.ToolbarButton(WebInspector.UIString("Emulate
push event"), undefined, WebInspector.UIString("Push"));
On 2016/05/05 01:09:22, caseq wrote:
> let's keep toolbar items together.

What do you mean?

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:197:
WebInspector.console.log("S: " + this._registration.scopeURL);
On 2016/05/05 01:09:21, caseq wrote:
> remove this?

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:224:
var title = WebInspector.UIString(this._registration.isDeleted ? "%s - deleted"
: "%s", this._registration.scopeURL);
On 2016/05/05 01:09:22, caseq wrote:
> nit:
> var title = this._registration.isDeleted ?
> WebInspector.UIString(this._registration.isDeleted ? "%s - deleted",
> this._registration.scopeURL) : this._registration.scopeURL;

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:236:
if (active) {
On 2016/05/05 01:09:22, caseq wrote:
> This is getting long, let's extract updating active/waiting/installing into
> separate methods?

I'd rather not for now. It is still manageable.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:252:
var inspectButton = activeEntry.createChild("span", "link");
On 2016/05/05 01:09:21, caseq wrote:
> Let's extract button creation into a method, we do this a lot here.

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:278:
skipButton.textContent = WebInspector.UIString("skipWaiting");
On 2016/05/05 01:09:21, caseq wrote:
> Skip Waiting

This is a term.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:308:
var location = error.sourceURL ? "(" + error.sourceURL + ":" + error.lineNumber
+ ")" : "";
On 2016/05/05 01:09:21, caseq wrote:
> WebInspector.UIString("(%s:%d)")

It is unused :)

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:314:
message.appendChild(createLabel("#" + error.versionId + ": " +
error.errorMessage, "error-icon"));
On 2016/05/05 01:09:22, caseq wrote:
> WebInspector.UIString()?

This is from the server, we don't wrap those.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:377:
this._manager.skipWaiting(this._registration.scopeURL);
I did the other way around.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js
(right):

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:495:
context.setLabel(label + " #" + this._versionId + " (" + this._status + ")");
On 2016/05/05 01:09:22, caseq wrote:
> Looks like WebInspector.UIString().. Wait.. A UIString in SDK?  😱

Nope

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:672:
this.controlledClients = []
On 2016/05/05 01:09:22, caseq wrote:
> ; while you're here

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:684:
this._fingerprint = JSON.stringify(this);
On 2016/05/05 01:09:22, caseq wrote:
> Please don't. Perhaps, keep payload around and stringify it?

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:802: *
@extends {WebInspector.Object}
On 2016/05/05 01:09:22, caseq wrote:
> Why?

Done.

https://codereview.chromium.org/1940993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerManager.js:853:
return result;
this.versions.values(...).map is not a function

[caseq, 2016-05-05 04:04:06]

lgtm

https://codereview.chromium.org/1940993002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(right):

https://codereview.chromium.org/1940993002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:145:
// Preserve the order.
move the entire section after toolbar construction.

https://codereview.chromium.org/1940993002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:165:
this._throttler = new WebInspector.Throttler(500);
Wouldn't this defeat throttling considering we may have multiple sections?

[pfeldman, 2016-05-05 04:28:31]

https://codereview.chromium.org/1940993002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(right):

https://codereview.chromium.org/1940993002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:145:
// Preserve the order.
On 2016/05/05 04:04:06, caseq wrote:
> move the entire section after toolbar construction.

Done.

https://codereview.chromium.org/1940993002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:165:
this._throttler = new WebInspector.Throttler(500);
On 2016/05/05 04:04:06, caseq wrote:
> Wouldn't this defeat throttling considering we may have multiple sections?

Nope, this throttling is against the rapid changes to the registration itself
(when installing it goes through numerous statuses before it settles).

[pfeldman, 2016-05-05 05:41:35]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-05-05 05:41:36]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/1940993002/#ps180001
(title: "review comment addressed.")

[commit-bot: I haz the power, 2016-05-05 05:41:47]

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

[commit-bot: I haz the power, 2016-05-05 06:54:57]

Description was changed from

==========
DevTools: simplify Service Workers panel UI, remove non-actionable data.

BUG=608997
==========

to

==========
DevTools: simplify Service Workers panel UI, remove non-actionable data.

BUG=608997
==========

[commit-bot: I haz the power, 2016-05-05 06:54:58]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-05-05 06:56:49]

Description was changed from

==========
DevTools: simplify Service Workers panel UI, remove non-actionable data.

BUG=608997
==========

to

==========
DevTools: simplify Service Workers panel UI, remove non-actionable data.

BUG=608997

Committed: https://crrev.com/f60030d12c0738084bac0a563cfc6edab2bf5b40
Cr-Commit-Position: refs/heads/master@{#391774}
==========

[commit-bot: I haz the power, 2016-05-05 06:56:50]

Patchset 10 (id:??) landed as
https://crrev.com/f60030d12c0738084bac0a563cfc6edab2bf5b40
Cr-Commit-Position: refs/heads/master@{#391774}