[Webkit-unassigned] [Bug 40425] Web Inspector: provide API for content scripts to interact with the inspector

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Joseph Pecoraro <joepeck at webkit.org>  2010-07-27 10:47:01 PST ---
(From update of attachment 62568)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,107 @@
> +2010-07-26  Andrey Kosyakov  <caseq at chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Web Inspector: basic support of WebInspector extensions API.
> +        https://bugs.webkit.org/show_bug.cgi?id=40425
> +        Test: inspector/extensions.html
> +
>          ...

This is pretty bare. I think it would go a long way to add at least
a comment that this is experimental. Also, I always recommend giving
a brief description of new files in the ChangeLog.


> +            'inspector/front-end/ExtensionAPI.js',
> +            'inspector/front-end/ExtensionPanel.js',
> +            'inspector/front-end/ExtensionRegistryStub.js',
> +            'inspector/front-end/ExtensionServer.js',

New inspector files are typically added to WebCore/inspector/front-end/WebKit.qrc.
There is a helper script to generate it, but I actually have never used it:
WebKitTools/Scripts/generate-qt-inspector-resource



> Index: WebCore/inspector/InspectorController.cpp
> ===================================================================
> +    if (m_scriptsToEvaluateOnLoad.size()) {
> +        ScriptState* scriptState = mainWorldScriptState(frame);
> +        for (Vector<String>::iterator it = m_scriptsToEvaluateOnLoad.begin();
> +             it != m_scriptsToEvaluateOnLoad.end(); ++it) {
> +            m_injectedScriptHost->injectScript(*it, scriptState);
> +        }
> +    }
> +
>      if (!enabled() || !m_frontend || frame != m_inspectedPage->mainFrame())
>          return;

Seems weird to inject scripts before checking if the Web Inspector is
enabled() or not. I don't think this is correct.


> Index: WebCore/inspector/front-end/ExtensionAPI.js
> ===================================================================
> +function bind(func, thisObject)
> +{
> +    var args = Array.prototype.slice.call(arguments, 2);
> +    return function() { return func.apply(thisObject, args.concat(Array.prototype.slice.call(arguments, 0))); };
> +}

Can you use the utilities.js's Function.prototype.bind?


> Index: WebCore/inspector/front-end/ExtensionServer.js
> ===================================================================

> +    initExtensions: function(extensions)
> +    {
> +        InspectorFrontendHost.setExtensionAPI("(" + extensionAPI + ")");
> +        InspectorExtensionRegistry.getExtensions();
> +    },

This could use a comment because extensionAPI is defined in a different file,
and its actually a function. I think it would be nice to see toString() on it.
Also, the parameter, "extensions" is not used.


> +    _addExtension: function(extension)
> +    {
>          ...
> +        } catch (e) {
> +            console.log("Failed to initialize extension " + extension.startPage + ":" + e);
> +        }
> +    },

Typically we make these console.error(). They show up in red in the inspector's
inspector which makes them more noticeable.


> Index: WebCore/inspector/front-end/InjectedScript.js
> ===================================================================

> +        if (value != null && typeof value !== "string")

We strive for === as much as possible, but this might be acceptable.
"value != null" is only true when value is null or undefined. But
it still takes a bit of memorization.


This code looked pretty solid too me. Huge patch though, so I skimmed
quite a bit.

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