Bug 89087 – Web Inspector: Remove devtoolsInjectedScript hidden property

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

[Andrey Adaikin]

We use this hidden property to reference a InjectedScript object for a given ScriptState, but we also have the same reference in the InjectedScriptManager's hash maps. Just use the hash maps and remove the hidden property from the global object.

[Andrey Adaikin]

Created attachment 147539 [details]
Patch

[Andrey Adaikin]

Created attachment 147544 [details]
Patch

Fix broken mac build

[Andrey Kosyakov]

Comment on attachment 147544 [details]
Patch

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

General approach looks good, but I'm a bit concerned with consistency between two maps (m_idToInjectedScript and  m_scriptStateToId) being not obvious.
Also a nit -- Google copyright line should only include last year.

> Source/WebCore/inspector/InjectedScriptManager.cpp:180
> +        if (it1 != m_idToInjectedScript.end())

When are we supposed to hit this check?

[Andrey Kosyakov]

Comment on attachment 147544 [details]
Patch

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

>> Source/WebCore/inspector/InjectedScriptManager.cpp:180
>> +        if (it1 != m_idToInjectedScript.end())
> 
> When are we supposed to hit this check?

Ok, looks like it's normal for us to hit this. LGTM then.

[Andrey Adaikin]

Created attachment 147567 [details]
Patch to land

[Pavel Feldman]

Comment on attachment 147567 [details]
Patch to land

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

> Source/WebCore/inspector/InjectedScriptManager.cpp:194
> +ScriptObject InjectedScriptManager::wrapWebGLRenderingContextForInstrumentation(const ScriptObject&)

InjectedScriptManager should not be aware of "WebGL".

[Andrey Adaikin]

Comment on attachment 147567 [details]
Patch to land

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

>> Source/WebCore/inspector/InjectedScriptManager.cpp:194
>> +ScriptObject InjectedScriptManager::wrapWebGLRenderingContextForInstrumentation(const ScriptObject&)
> 
> InjectedScriptManager should not be aware of "WebGL".

Then it also should not be aware of the InjectedScript class.

[WebKit Review Bot]

Comment on attachment 147567 [details]
Patch to land

Clearing flags on attachment: 147567

Committed r120347: <http://trac.webkit.org/changeset/120347>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

Comment on attachment 147567 [details]
Patch to land

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

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:-98
> -        visitor.append(&thisObject->m_injectedScript);

Now that this is removed how can we make sure that the injected script is not garbage collected?

[Yury Semikhatsky]

(In reply to comment #10)
> (From update of attachment 147567 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147567&action=review
> 
> > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:-98
> > -        visitor.append(&thisObject->m_injectedScript);
> 
> Now that this is removed how can we make sure that the injected script is not garbage collected?

Nevermind. I missed that it is kept as a strong ref in ScriptObject.