[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





--- Comment #6 from Jan Keromnes <janx at chromium.org>  2012-07-24 17:16:45 PST ---
(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?

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