Bug 89087 - Web Inspector: Remove devtoolsInjectedScript hidden property
Summary: Web Inspector: Remove devtoolsInjectedScript hidden property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Adaikin
URL:
Keywords:
Depends on:
Blocks: 88973
  Show dependency treegraph
 
Reported: 2012-06-14 03:00 PDT by Andrey Adaikin
Modified: 2012-06-18 03:23 PDT (History)
17 users (show)

See Also:


Attachments
Patch (17.38 KB, patch)
2012-06-14 03:04 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (17.48 KB, patch)
2012-06-14 03:26 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch to land (18.25 KB, patch)
2012-06-14 06:09 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Adaikin 2012-06-14 03:00:33 PDT
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.
Comment 1 Andrey Adaikin 2012-06-14 03:04:51 PDT
Created attachment 147539 [details]
Patch
Comment 2 Andrey Adaikin 2012-06-14 03:26:50 PDT
Created attachment 147544 [details]
Patch

Fix broken mac build
Comment 3 Andrey Kosyakov 2012-06-14 05:30:28 PDT
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?
Comment 4 Andrey Kosyakov 2012-06-14 05:48:40 PDT
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.
Comment 5 Andrey Adaikin 2012-06-14 06:09:09 PDT
Created attachment 147567 [details]
Patch to land
Comment 6 Pavel Feldman 2012-06-14 08:32:32 PDT
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".
Comment 7 Andrey Adaikin 2012-06-14 08:58:24 PDT
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.
Comment 8 WebKit Review Bot 2012-06-14 12:24:50 PDT
Comment on attachment 147567 [details]
Patch to land

Clearing flags on attachment: 147567

Committed r120347: <http://trac.webkit.org/changeset/120347>
Comment 9 WebKit Review Bot 2012-06-14 12:24:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Yury Semikhatsky 2012-06-18 03:19:19 PDT
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?
Comment 11 Yury Semikhatsky 2012-06-18 03:23:16 PDT
(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.