Bug 89530 – Web Inspector: Allow module injections into the InjectedScript

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Adaikin]

Add a new InjectedModule abstract class that inherits InjectedScriptBase to reuse implementation, and which javascript code is injected via InjectedScript's addModule.

[Andrey Adaikin]

Created attachment 148460 [details]
Patch

[Pavel Feldman]

Comment on attachment 148460 [details]
Patch

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

> Source/WebCore/inspector/InjectedModule.h:42
> +class InjectedModule : public InjectedScriptBase {

InjectedScriptModule

> Source/WebCore/inspector/InjectedModule.h:49
> +    friend void InjectedScript::injectModule(InjectedModule&, bool&);

friend class InjectedScript

> Source/WebCore/inspector/InjectedScript.cpp:242
> +    ASSERT(!hasNoValue());

I would try to hold all the module-related logic in a single location. (InjectedScriptModule). Eventually, we might want to make generic injected script a module itself.

[Andrey Adaikin]

Created attachment 148491 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 148491 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptBase.h:53
> +    virtual String name() const = 0;

Just pass it as a constructor parameter?

> Source/WebCore/inspector/InjectedScriptBase.h:70
> +    friend class InjectedScriptModule;

I'd rather introduce protected method initialize(ScriptObject, inspectedStateAccessCheck)

> Source/WebCore/inspector/InjectedScriptModule.cpp:51
> +    ASSERT(!injectedScript.hasNoValue());

If InjectedScript is required we may have injectModule method defined on InjectedScript and avoid this check and InjectedScriptManager parameter.

[Andrey Adaikin]

Comment on attachment 148491 [details]
Patch

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

>> Source/WebCore/inspector/InjectedScriptBase.h:53
>> +    virtual String name() const = 0;
> 
> Just pass it as a constructor parameter?

Done.

>> Source/WebCore/inspector/InjectedScriptBase.h:70
>> +    friend class InjectedScriptModule;
> 
> I'd rather introduce protected method initialize(ScriptObject, inspectedStateAccessCheck)

Done.

>> Source/WebCore/inspector/InjectedScriptModule.cpp:51
>> +    ASSERT(!injectedScript.hasNoValue());
> 
> If InjectedScript is required we may have injectModule method defined on InjectedScript and avoid this check and InjectedScriptManager parameter.

The previous attachment does suggest a solution with injectModule defined on the InjectedScript. But ultimately we would like the InjectedScript be a module itself (e.g. smth like GenericInjectedScriptModule), and prohibit it to inject other modules. The "injectModule" method will be in a separate entity (let it call InjectedModuleManager) that will be also shared across all modules to reuse methods like bindObject, wrapObject and etc.

In other words, now we have: InjectedScript = GenericInjectedScriptModule + InjectedModuleManager, which should be split up accordingly in a future refactoring. Thus, not having the "injectModule" method defined on InjectedScript helps the future efforts.

[Andrey Adaikin]

Created attachment 148589 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148589 [details]
Patch

Clearing flags on attachment: 148589

Committed r120842: <http://trac.webkit.org/changeset/120842>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.