Add a new InjectedModule abstract class that inherits InjectedScriptBase to reuse implementation, and which javascript code is injected via InjectedScript's addModule.
Created attachment 148460 [details] Patch
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.
Created attachment 148491 [details] Patch
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.
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.
Created attachment 148589 [details] Patch
Comment on attachment 148589 [details] Patch Clearing flags on attachment: 148589 Committed r120842: <http://trac.webkit.org/changeset/120842>
All reviewed patches have been landed. Closing bug.