Bug 91362 – Web Inspector: Implement settings API for extensions

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

[Jan Keromnes]

It would be great if Web Inspector extensions could declare settings and expose them in the Settings Screen.

EXAMPLE:
Imagine an extension called "Orion" featuring an Orion-like code editor inside the DevTools. This extension could add a tab called "Orion" to the Settings Screen with a few settings like "Theme", "Keymap", "Tab width", ... Those settings could then be modified by the user, changing the behavior of the extension accordingly.

[Jan Keromnes]

Created attachment 152618 [details]
Patch

[Jan Keromnes]

I uploaded my first take on this API, without layout test coverage for now. +caseq can you tell me what you think of it?

[WebKit Review Bot]

Comment on attachment 152618 [details]
Patch

Attachment 152618 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13248707

New failing tests:
inspector/extensions/extensions-api.html

[WebKit Review Bot]

Created attachment 152650 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Andrey Kosyakov]

Comment on attachment 152618 [details]
Patch

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

[Jan Keromnes]

(In reply to comment #5)
> 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.

For now only 2 types of simple controls are supported, and more will be added once the API is clearly defined. I agree that it should address complex use cases like the ones you mentioned, and it's worth looking into supporting custom settings components (e.g. iframes).

> We need the tests.

I agree and will write tests for this API once we agreed on how it should work.

> > 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.

I mirrored the behavior of extension panels (an extension can create several panels with arbitrary labels), but having one settings panel per extension is also a good idea. How can I get the extension name (not the id)?

> > 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.

A callback for the creation of a setting is not really useful, the only thing extension writers care about is when a setting's value is modified. The idea of passing a listener directly at creation is nicely simple and removes unnecessary lines of code. I don't think it will be a problem if the documentation is well written, but if you feel that I should revert to explicit callback and listener for the sake of consistency with the rest of the code, I can do it.

> > 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?

I copied the behavior for extension panel labels. I think it's a good idea to support localized strings for both extension panels and settings panels, but I don't know how this works in the Web Inspector -- do we have a translation table?

[Jan Keromnes]

Created attachment 155873 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 155873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155873&action=review

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

Can we have some tests that create settings on the extension side, open and change them on the front-end and assure extension receives notifications?

> Source/WebCore/inspector/front-end/ExtensionAPI.js:736
> +    createCheckbox: function(section, name, checked, callback) {

So how are extension authors supposed to modify the properties of the checkbox that they created? E.g., disable it or change the value?

> Source/WebCore/inspector/front-end/ExtensionServer.js:570
> +    _extensionName: function(port) {

style: { needs to be on a line by its own.

> Source/WebCore/inspector/front-end/SettingsScreen.js:836
> +    _getOrCreateSection: function(section) {

{ => new line
also, please annotate.

> Source/WebCore/inspector/front-end/SettingsScreen.js:842
> +    _createSetting: function(id, name, value) {

ditto

> Source/WebCore/inspector/front-end/SettingsScreen.js:852
> +    createCheckbox: function(id, section, name, checked) {

ditto

> Source/WebCore/inspector/front-end/SettingsScreen.js:855
> +        var label = WebInspector.UIString(name);

WebInspector.UIString performs a lookup in localizedStrings map. Using it on a user-provided strings is not appropriate.

> Source/WebCore/inspector/front-end/SettingsScreen.js:861
> +    createSelect: function(id, section, name, options, callback) {

ditto

[Jan Keromnes]

Created attachment 158202 [details]
Patch

[Jan Keromnes]

Thanks for your feedback, Andrey!

> Can we have some tests that create settings on the extension side, open and change them on the front-end and assure extension receives notifications?

Yes we can!

> So how are extension authors supposed to modify the properties of the checkbox that they created? E.g., disable it or change the value?

I don't really see why an extension would need to change the value of one of its settings or disable it:
- Extensions declare settings with their default values, and then wait for the user to change them. An extension changing its own settings behind the back of the user is unexpected behavior.
- Disabled settings are in my opinion one of the worst UI ideas ever: a user might want to change the setting, but he'll not be able to. This is source of frustration.

However, I am open to discuss future changes to the behavior of these settings, including ideas like more complex or interdependent settings. Once agreed upon, those ideas will become separate bugs and patches.

> style: { needs to be on a line by its own.

Fixed.

> also, please annotate.

Annotating WebInspector.ExtensionSettingsTab in a follow-up patch -- hang tight.

> WebInspector.UIString performs a lookup in localizedStrings map. Using it on a user-provided strings is not appropriate.

Fixed by not using WebInspector.UIString().

[Jan Keromnes]

Created attachment 158216 [details]
Patch

[Pavel Feldman]

A high level question: what is the priority of this for you? Do we have extensions screaming for such support?

[Jan Keromnes]

(In reply to comment #12)
> A high level question: what is the priority of this for you? Do we have extensions screaming for such support?

Not really a top priority but I think it would be really nice for devtools extensions to be able to augment the devtools settings, without the need to reinvent UI and do weird custom things to expose settings to users. It's part of my effort to increase extensibility in the devtools, and will work really well with the future extension API for editors.

[Pavel Feldman]

Comment on attachment 158216 [details]
Patch

Parking it for now - will resume when we have actual clients /request for this API.

[Brian Burg]

Closing as invalid, as this bug pertains to the old inspector UI and/or its tests.
Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.