[webkit-reviews] review canceled: [Bug 91362] Web Inspector: Implement settings API for extensions : [Attachment 152618] Patch

[bugzilla-daemon at webkit.org]

Andrey Kosyakov <caseq at chromium.org> has canceled Jan Keromnes
<janx at chromium.org>'s request for review:
Bug 91362: Web Inspector: Implement settings API for extensions
https://bugs.webkit.org/show_bug.cgi?id=91362

Attachment 152618: Patch
https://bugs.webkit.org/attachment.cgi?id=152618&action=review

------- Additional Comments from Andrey Kosyakov <caseq at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152618&action=review


Some general comments: this does not seem to cover needs of some of the
existing extensions (e.g. chrome-devtools-autosave or devtools-save), exposes
somewhat rich interface that still does not address many use cases, including
those in use by our existing settings, such as disabling/enabling controls
based on state of other controls. Another option you may consider is having an
iframe with settings page.

> Source/WebCore/ChangeLog:8
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

Please describe the change.

> Source/WebCore/ChangeLog:10
> +	   No new tests (OOPS!).

We need the tests.

> Source/WebCore/inspector/front-end/ExtensionAPI.js:737
> +    createTab: function(name, callback)

I would suggest one tab per extension, titled with an extension name -- this
way it would be more predictable for the user.

> Source/WebCore/inspector/front-end/ExtensionAPI.js:764
> +	       onChange = new EventSink(events.SettingChanged + id);
> +
> +	   function changeListener(value) {
> +	       if (callback)
> +		   callback(value);
> +	   }
> +	   onChange.addListener(changeListener);

There is difference in semantics between callback and event handler -- we
expect callback to be invoked at most once, and the event listener may be
invoked multiple times. For event listeners, we allow subscription and
unsubscription. Quietly converting callback to event listener may be confusing.


> Source/WebCore/inspector/front-end/ExtensionAPI.js:769
> +	   extensionServer.sendRequest(request, function() {});

no need to pass empty function explicitly.

> Source/WebCore/inspector/front-end/ExtensionServer.js:93
> +    notifySettingChanged: function(identifier, value) {

style: { should be on a line of its own.

> Source/WebCore/inspector/front-end/SettingsScreen.js:745
> +	       setting = this._createSetting(section, name, checked),
> +	       label = WebInspector.UIString(name),
> +	       p = this._getOrCreateSection(section);

We use multiple vat statements for this elsewhere.

> Source/WebCore/inspector/front-end/SettingsScreen.js:758
> +	       choices.push([ WebInspector.UIString(options[i]), options[i] ]);


Do we expect user to add the values that are within localizedStrings?

> Source/WebCore/inspector/front-end/SettingsScreen.js:809
> +	       for (var i = 0; i < this._tabsToAppend.length; i++) {
> +		  
this._settingsScreen._tabbedPane.appendTab.apply(this._settingsScreen._tabbedPa
ne, this._tabsToAppend[i]);
> +	       }

{ } are redundant