|
Description
Timothy Hatcher
2014-04-15 01:22:25 PDT
Created attachment 229362 [details]
Patch
Comment on attachment 229362 [details] Patch Attachment 229362 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5227490024882176 New failing tests: fast/dom/gc-attribute-node.html Created attachment 229364 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 229362 [details] Patch Attachment 229362 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4866719214469120 New failing tests: fast/dom/gc-attribute-node.html Created attachment 229365 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Test failure is unrelated to my patch. Does not fail on my machine either. Created attachment 229372 [details]
Patch
That should fix the windows build. The new Windows failures seem unrelated. Comment on attachment 229372 [details] Patch Attachment 229372 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4670146077523968 New failing tests: fast/dom/gc-attribute-node.html Created attachment 229376 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 229372 [details] Patch Attachment 229372 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6229872041000960 New failing tests: fast/dom/gc-attribute-node.html Created attachment 229387 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 229372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229372&action=review Looks good to me! r=me > Source/JavaScriptCore/inspector/agents/InspectorProfilerAgent.cpp:224 > +void InspectorProfilerAgent::didCreateFrontendAndBackend(InspectorFrontendChannel* frontendChannel, InspectorBackendDispatcher* backendDispatcher) > +{ > + m_frontendDispatcher = std::make_unique<InspectorProfilerFrontendDispatcher>(frontendChannel); > + m_backendDispatcher = InspectorProfilerBackendDispatcher::create(backendDispatcher, this); > +} > + > +void InspectorProfilerAgent::willDestroyFrontendAndBackend(InspectorDisconnectReason reason) > +{ > + m_frontendDispatcher = nullptr; > + m_backendDispatcher.clear(); > + > + reset(); > + > + disable(reason == InspectorDisconnectReason::InspectedTargetDestroyed ? SkipRecompile : Recompile); > +} Nit: I like to have these methods up near the constructor so they are easy to find. > Source/JavaScriptCore/inspector/agents/JSGlobalObjectProfilerAgent.cpp:37 > +JSGlobalObjectProfilerAgent::JSGlobalObjectProfilerAgent(JSC::JSGlobalObject& globalObject) Nit: You can drop the "JSC::" here. > Source/JavaScriptCore/inspector/agents/JSGlobalObjectProfilerAgent.h:32 > +#include <wtf/PassOwnPtr.h> Nit: I do not think this include is needed. > Source/JavaScriptCore/inspector/agents/JSGlobalObjectProfilerAgent.h:45 > + virtual JSC::ExecState* profilingGlobalExecState() const override final; Nit: Final is already on the class, we can remove it from here. > Source/WebCore/inspector/InspectorController.cpp:76 > +#include <inspector/agents/InspectorProfilerAgent.h> Nit: unnecessary include. > Source/WebCore/inspector/InspectorInstrumentation.cpp:962 > + return nullptr; Maybe this should return "String()" instead of nullptr. > Source/WebCore/inspector/PageProfilerAgent.h:45 > + virtual JSC::ExecState* profilingGlobalExecState() const override final; Nit: Lets put final on the class instead of this method. > Source/WebCore/inspector/PageProfilerAgent.h:47 > + Page* m_inspectedPage; Nit: Page&? > Source/WebCore/inspector/WebProfilerAgent.h:36 > +typedef String ErrorString; Nit: Unnecessary forward declaration. > Source/WebCore/inspector/WorkerInspectorController.cpp:99 > + profilerAgent->setScriptDebugServer(&debuggerAgent->scriptDebugServer()); Oops. Though I'm not sure if we actually need this in a worker agent. Better to have it then not. > Source/WebCore/inspector/WorkerProfilerAgent.h:45 > + virtual JSC::ExecState* profilingGlobalExecState() const override final; Nit: final on class instead of method. > Source/WebCore/page/PageConsole.cpp:170 > if (!InspectorInstrumentation::profilerEnabled(&m_page)) > return; > This seems unnecessary. Since the InspectorInstrumentation::startProfiling won't do anything unless the profiler is enabled. > Source/WebCore/page/PageConsole.cpp:179 > + RefPtr<JSC::Profile> profile = InspectorInstrumentation::stopProfiling(&m_page, exec, title); Likewise. Created attachment 229677 [details]
Patch
Comment on attachment 229677 [details] Patch Attachment 229677 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5359042188804096 New failing tests: fast/dom/gc-attribute-node.html Created attachment 229690 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 229694 [details]
Patch
Created attachment 229697 [details]
Patch
Comment on attachment 229697 [details] Patch Attachment 229697 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6506334841208832 New failing tests: fast/dom/gc-attribute-node.html Created attachment 229702 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 229697 [details] Patch Attachment 229697 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4743044607770624 New failing tests: fast/dom/gc-attribute-node.html Created attachment 229703 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 229697 [details] Patch Attachment 229697 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5995484938240000 New failing tests: fast/dom/gc-attribute-node.html Created attachment 229708 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 229697 [details] Patch Attachment 229697 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5764422190497792 New failing tests: fast/dom/gc-attribute-node.html Created attachment 229712 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Any idea why fast/dom/gc-attribute-node.html would fail with this patch? It seem flaky. It fails sometimes and not others. Comment on attachment 229697 [details] Patch Clearing flags on attachment: 229697 Committed r167530: <http://trac.webkit.org/changeset/167530> All reviewed patches have been landed. Closing bug. (In reply to comment #31) > (From update of attachment 229697 [details]) > Clearing flags on attachment: 229697 > > Committed r167530: <http://trac.webkit.org/changeset/167530> It broke the build on the Apple Windows debug bot. Comment on attachment 229697 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229697&action=review > Source/WebCore/inspector/PageDebuggerAgent.cpp:-71 > -void PageDebuggerAgent::enable() > -{ > - WebDebuggerAgent::enable(); > - m_instrumentingAgents->setPageDebuggerAgent(this); > -} > - > -void PageDebuggerAgent::disable(bool isBeingDestroyed) > -{ > - WebDebuggerAgent::disable(isBeingDestroyed); > - m_instrumentingAgents->setPageDebuggerAgent(nullptr); > -} Why did these get removed? Now nothing sets the instrumenting agents pageDebuggerAgent. We rely on that so that on page changes we do handling for clearing the global object. Comment on attachment 229697 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229697&action=review >> Source/WebCore/inspector/PageDebuggerAgent.cpp:-71 >> -} > > Why did these get removed? Now nothing sets the instrumenting agents pageDebuggerAgent. We rely on that so that on page changes we do handling for clearing the global object. I thought it wasn't needed because I saw WebDebuggerAgent doing "the same thing". But WebDebuggerAgent calls setInspectorDebuggerAgent and this calls setPageDebuggerAgent. As far as I can tell, pageDebuggerAgent is needed to avoid a cast or other type check. I'll restore this code. Comment on attachment 229697 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229697&action=review >>> Source/WebCore/inspector/PageDebuggerAgent.cpp:-71 >>> -} >> >> Why did these get removed? Now nothing sets the instrumenting agents pageDebuggerAgent. We rely on that so that on page changes we do handling for clearing the global object. > > I thought it wasn't needed because I saw WebDebuggerAgent doing "the same thing". But WebDebuggerAgent calls setInspectorDebuggerAgent and this calls setPageDebuggerAgent. As far as I can tell, pageDebuggerAgent is needed to avoid a cast or other type check. I'll restore this code. Patch to add them back: https://bugs.webkit.org/attachment.cgi?id=230121&action=review |