[Webkit-unassigned] [Bug 91362] Web Inspector: Implement settings API for extensions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=91362


Andrey Kosyakov <caseq at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #152618|review?                     |
               Flag|                            |




--- Comment #5 from Andrey Kosyakov <caseq at chromium.org>  2012-07-17 06:40:21 PST ---
(From update of attachment 152618)
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._tabbedPane, this._tabsToAppend[i]);
> +            }

{ } are redundant

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.