[webkit-reviews] review granted: [Bug 72388] Web Inspector: [Extensions API] drop ExtensionSidebarPane.onUpdated, use callbacks instead : [Attachment 115167] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has granted Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 72388: Web Inspector: [Extensions API] drop ExtensionSidebarPane.onUpdated,
use callbacks instead
https://bugs.webkit.org/show_bug.cgi?id=72388

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115167&action=review


> Source/WebCore/inspector/front-end/ExtensionPanel.js:84
> +WebInspector.ExtensionNotifierView = function(id)

You should probably make this a superclass of the ExtensionView.

> Source/WebCore/inspector/front-end/ExtensionPanel.js:183
> +    setObject: function(object, title, callback)

Drive-by: you should start annotating your code.

> Source/WebCore/inspector/front-end/ExtensionPanel.js:215
> +    _makeObjectPropertiesView: function()

_createObjectPropertiesView

> Source/WebCore/inspector/front-end/ExtensionPanel.js:231
> +	       callback("operation cancelled");

What does "operation cancelled" mean for the client? Annotating this callback
would give me a clue.

> Source/WebCore/inspector/front-end/ExtensionServer.js:234
> +	       var result = error ? this._status.E_FAILED(error) :
this._status.OK();

aha. so non-empty is error string.

> Source/WebCore/inspector/front-end/View.js:173
> +    detach: function(force)

force -> overrideHideOnDetach