Summary: | Web Inspector: Remove devtoolsInjectedScript hidden property | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Adaikin <aandrey> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Adaikin <aandrey> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, apavlov, bweinstein, caseq, eustas.bug, haraken, japhet, jochen, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 88973 | ||||||||||
Attachments: |
|
Description
Andrey Adaikin
2012-06-14 03:00:33 PDT
Created attachment 147539 [details]
Patch
Created attachment 147544 [details]
Patch
Fix broken mac build
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 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. Created attachment 147567 [details]
Patch to land
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 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 on attachment 147567 [details] Patch to land Clearing flags on attachment: 147567 Committed r120347: <http://trac.webkit.org/changeset/120347> All reviewed patches have been landed. Closing bug. 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? (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. |