|
Description
Devin Rousso
2017-12-13 15:14:02 PST
Created attachment 329265 [details]
[Patch] WIP
Comment on attachment 329265 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=329265&action=review > Source/JavaScriptCore/inspector/protocol/Canvas.json:-49 > - { "name": "frameId", "$ref": "Network.FrameId", "description": "Parent frame identifier." }, If the frontend wants to associate the Canvas with a Frame it would need to use resolve this to a DOMNode and then get the node's frame? That seems okay. > Source/WebCore/inspector/InspectorCanvas.cpp:95 > +HTMLCanvasElement* InspectorCanvas::getIfHTMLCanvasElement() This is a clunky name. How about: HTMLCanvasElement* canvasElement() const; It seems clear this would return a nullptr if this is not an HTMLCanvasElement. Comment on attachment 329265 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=329265&action=review >> Source/JavaScriptCore/inspector/protocol/Canvas.json:-49 >> - { "name": "frameId", "$ref": "Network.FrameId", "description": "Parent frame identifier." }, > > If the frontend wants to associate the Canvas with a Frame it would need to use resolve this to a DOMNode and then get the node's frame? That seems okay. Yeah. I was trying to think about it, but when would we want to do that? I figure that it might be better for us to provide data relating to what context/target (document vs worker) we are dealing with, but that would be for a followup (and once worker support is done). >> Source/WebCore/inspector/InspectorCanvas.cpp:95 >> +HTMLCanvasElement* InspectorCanvas::getIfHTMLCanvasElement() > > This is a clunky name. How about: > > HTMLCanvasElement* canvasElement() const; > > It seems clear this would return a nullptr if this is not an HTMLCanvasElement. I personally don't like that naming convention, as just having an HTMLCanvasElement* doesn't mean that it might be a nullptr. Also, it would make naming a bit more annoying at a few of the call-sites (e.g. InspectorCanvas::buildObjectForCanvas) where there is already a `canvas` or `element` variable, especially since I can't reuse `canvasElement`. If you have a better name that avoids this, I'd be happy to change it. Comment on attachment 329265 [details] [Patch] WIP Attachment 329265 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5650968 New failing tests: webgl/1.0.2/conformance/context/context-release-with-workers.html inspector/canvas/create-context-webgl.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329279 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329265 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=329265&action=review >>> Source/JavaScriptCore/inspector/protocol/Canvas.json:-49 >>> - { "name": "frameId", "$ref": "Network.FrameId", "description": "Parent frame identifier." }, >> >> If the frontend wants to associate the Canvas with a Frame it would need to use resolve this to a DOMNode and then get the node's frame? That seems okay. > > Yeah. I was trying to think about it, but when would we want to do that? I figure that it might be better for us to provide data relating to what context/target (document vs worker) we are dealing with, but that would be for a followup (and once worker support is done). I'd guess that an OffscreenCanvas created by a Worker would be exposed through the Worker's InspectorController (all happening on the Worker's Thread). In which case we'd: • Enable the Canvas domain for Workers • Give WI.Canvas a `target` property and populate it. At which point we'd be able to know from `canvas.target` if it was in a Page / Worker. Comment on attachment 329265 [details] [Patch] WIP Attachment 329265 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5650987 New failing tests: webgl/1.0.2/conformance/context/context-release-with-workers.html inspector/canvas/create-context-webgl.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329282 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 329265 [details] [Patch] WIP Attachment 329265 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5651127 New failing tests: fast/canvas/webgl/context-release-upon-reload.html webgl/1.0.2/conformance/context/context-release-with-workers.html Created attachment 329287 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 329265 [details] [Patch] WIP Attachment 329265 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5651926 New failing tests: inspector/canvas/create-context-webgl.html Created attachment 329294 [details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 329306 [details]
[Patch] WIP
Rebase
Comment on attachment 329306 [details] [Patch] WIP Attachment 329306 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5654045 New failing tests: webgl/1.0.2/conformance/context/context-release-with-workers.html inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329315 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329306 [details] [Patch] WIP Attachment 329306 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5654238 New failing tests: webgl/1.0.2/conformance/context/context-release-with-workers.html inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html Created attachment 329320 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 329306 [details] [Patch] WIP Attachment 329306 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5654261 New failing tests: fast/canvas/webgl/context-release-upon-reload.html webgl/1.0.2/conformance/context/context-release-with-workers.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329323 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 329324 [details]
Patch
Comment on attachment 329324 [details] Patch Attachment 329324 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5655226 New failing tests: fast/canvas/webgl/context-release-upon-reload.html webgl/1.0.2/conformance/context/context-release-with-workers.html Created attachment 329326 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 329324 [details] Patch Attachment 329324 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5655412 New failing tests: fast/canvas/webgl/context-release-upon-reload.html inspector/canvas/create-context-bitmaprenderer.html webgl/1.0.2/conformance/context/context-release-with-workers.html inspector/canvas/create-context-webgl.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329327 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 329328 [details]
Patch
I think some of the test failures were due to the fact that the CanvasRenderingContext would be destroyed before the frame navigation event is fired, but I could be wrong
Comment on attachment 329328 [details] Patch Attachment 329328 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5656046 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html Created attachment 329330 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329328 [details] Patch Attachment 329328 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5656175 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html Created attachment 329334 [details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 329336 [details]
Patch
Based on the mac-debug results, I think we do need a timer when destroying contexts to prevent JS allocations.
Comment on attachment 329336 [details] Patch Attachment 329336 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5656962 New failing tests: fast/canvas/webgl/context-release-upon-reload.html Created attachment 329338 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329336 [details] Patch Attachment 329336 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5656975 New failing tests: webgl/1.0.2/conformance/context/context-release-with-workers.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329339 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 329336 [details] Patch Attachment 329336 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5656986 New failing tests: fast/canvas/webgl/context-release-upon-reload.html webgl/1.0.2/conformance/context/context-release-with-workers.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329340 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 329336 [details] Patch Attachment 329336 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5656987 New failing tests: webgl/1.0.2/conformance/context/context-release-with-workers.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329341 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 329353 [details]
Patch
Oops. I added back the logic inside frameNavigated.
Comment on attachment 329353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329353&action=review r=me > Source/WebCore/inspector/InspectorCanvas.cpp:96 > +HTMLCanvasElement* InspectorCanvas::getIfHTMLCanvasElement() I still don't like this name. We normally don't include `get` in a getter name either. > Source/WebCore/inspector/InspectorCanvas.cpp:313 > + auto* canvasElement = getIfHTMLCanvasElement(); Should we have a FIXME here for OffscreenCanvas? > Source/WebCore/inspector/InspectorCanvas.h:49 > +class OffscreenCanvas; Not used? > Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:153 > + errorString = ASCIILiteral("OffscreenCanvas is currently not supported."); OffscreenCanvas won't have a node. So probably a better error might be something like: errorString = ASCIILiteral("No node for this canvas"); errorString = ASCIILiteral("No node for OffscreenCanvas"); Also I know we aren't consistent, but I don't think we have periods with these errors most of the time. > Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:215 > + auto* canvasElement = inspectorCanvas->getIfHTMLCanvasElement(); > + if (!canvasElement) { > + errorString = ASCIILiteral("OffscreenCanvas is currently not supported."); > + return; > + } Would it be possible for an OffscreenCanvas to have client nodes? If not we should update this error messages. > Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:-380 > - Vector<InspectorCanvas*> inspectorCanvases; > - for (auto& inspectorCanvas : m_identifierToInspectorCanvas.values()) { > - if (inspectorCanvas->canvas().document().frame() == &frame) > - inspectorCanvases.append(inspectorCanvas.get()); > - } > - > - for (auto* inspectorCanvas : inspectorCanvases) { > - String identifier = unbindCanvas(*inspectorCanvas); > - if (m_enabled) > - m_frontendDispatcher->canvasRemoved(identifier); > - } Hmm. So this relies on the GC to eliminate canvases in subframes instead of immediately eliminating them when the frame goes away? I suppose this is fine but if I have a long running top frame and short lived sub-frames it might be annoying if the frontend updates out of sync with the frames coming and going. Created attachment 329395 [details]
Patch
Created attachment 329399 [details]
Patch
Comment on attachment 329399 [details] Patch Clearing flags on attachment: 329399 Committed r225941: <https://trac.webkit.org/changeset/225941> All reviewed patches have been landed. Closing bug. webgl/1.0.2/conformance/context/context-release-with-workers.html is failing an assertion after this change: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r225980%20(5403)/results.html SHOULD NEVER BE REACHED /Volumes/Data/slave/elcapitan-debug/build/Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp(397) : void WebCore::InspectorCanvasAgent::didCreateCanvasRenderingContext(WebCore::CanvasRenderingContext &) 1 0x10be08dfd WTFCrash 2 0x116183bc5 WebCore::InspectorCanvasAgent::didCreateCanvasRenderingContext(WebCore::CanvasRenderingContext&) 3 0x116131395 WebCore::InspectorInstrumentation::didCreateCanvasRenderingContextImpl(WebCore::InstrumentingAgents&, WebCore::CanvasRenderingContext&) 4 0x116033bc1 WebCore::InspectorInstrumentation::didCreateCanvasRenderingContext(WebCore::CanvasRenderingContext&) 5 0x11723021e WebCore::WebGLRenderingContext::create(WebCore::CanvasBase&, WTF::Ref<WebCore::GraphicsContext3D>&&, WebCore::GraphicsContext3DAttributes) 6 0x1172400ef WebCore::WebGLRenderingContextBase::create(WebCore::CanvasBase&, WebCore::GraphicsContext3DAttributes&, WTF::String const&) 7 0x115ee752b WebCore::HTMLCanvasElement::createContextWebGL(WTF::String const&, WebCore::GraphicsContext3DAttributes&&) 8 0x115ee6503 WebCore::HTMLCanvasElement::getContext(JSC::ExecState&, WTF::String const&, WTF::Vector<JSC::Strong<JSC::Unknown>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&) 9 0x114826c8d WebCore::jsHTMLCanvasElementPrototypeFunctionGetContextBody(JSC::ExecState*, WebCore::JSHTMLCanvasElement*, JSC::ThrowScope&) 10 0x11481f70e long long WebCore::IDLOperation<WebCore::JSHTMLCanvasElement>::call<&(WebCore::jsHTMLCanvasElementPrototypeFunctionGetContextBody(JSC::ExecState*, WebCore::JSHTMLCanvasElement*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) 11 0x11481f49c WebCore::jsHTMLCanvasElementPrototypeFunctionGetContext(JSC::ExecState*) 12 0x36c7aa401178 13 0x10a99721b llint_entry 14 0x10a99721b llint_entry 15 0x10a98f2f7 vmEntryToJavaScript 16 0x10b6c439e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 17 0x10b66a1e2 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) 18 0x10b8e6ee7 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 19 0x10b8e7030 JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 20 0x11578e19b WebCore::JSMainThreadExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 21 0x11577bc78 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) 22 0x11577bd9d WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ExceptionDetails*) 23 0x115d131e7 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&) 24 0x115d11882 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) 25 0x116076b00 WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&) 26 0x11607696f WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement>&&, WTF::TextPosition const&) 27 0x1160589e5 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() 28 0x116058b33 WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&) 29 0x116057c28 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) 30 0x11605783b WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) 31 0x116059a8a WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl>&&) https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r225980%20(5403)/results.html Reverted r225941 for reason: This change introduced LayoutTest crashes and assertion failures. Committed r225990: <https://trac.webkit.org/changeset/225990> Created attachment 329562 [details]
Patch
I'm not 100% sure about this, but I think that the InstrumentingAgents are wiped out whenever a Frame is removed. The failing tests checked that canvas contexts within iframes are destroyed after the iframe navigates or is removed. As a result, since the InstrumentingAgents don't exist anymore, we don't have a way to remove the existing contexts from the CanvasAgent's list, meaning that they are still listed there even though they don't exist anymore. The solution is to not delete the code inside InspectorCanvasAgent::frameNavigated that finds all the tracked contexts for the navigating frame and removes them. Hopefully :|
Comment on attachment 329562 [details] Patch Attachment 329562 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5681941 New failing tests: webgl/1.0.2/conformance/context/context-release-with-workers.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329564 [details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329562 [details] Patch Attachment 329562 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5681960 New failing tests: webgl/1.0.2/conformance/context/context-release-with-workers.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329565 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 329562 [details] Patch Attachment 329562 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5681966 New failing tests: webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html webgl/1.0.2/conformance/context/context-release-with-workers.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329567 [details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329562 [details] Patch Attachment 329562 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5681970 New failing tests: fast/canvas/webgl/context-release-upon-reload.html webgl/1.0.2/conformance/context/context-release-with-workers.html webgl/1.0.2/conformance/context/context-release-upon-reload.html Created attachment 329569 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 329588 [details]
Patch
Taking a different approach. Instead of trying to add InspectorInstrumentation hooks to ~CanvasRenderingContext, we can expand the usage of the CanvasObserver concept and rely on the destruction of the CanvasBase to know when the CanvasRenderingContext will be destroyed.
Comment on attachment 329588 [details] Patch Attachment 329588 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5689923 Number of test failures exceeded the failure limit. Created attachment 329589 [details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329588 [details] Patch Attachment 329588 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5690175 New failing tests: inspector/canvas/requestContent-webgl.html inspector/dom/highlightQuad.html Created attachment 329590 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329588 [details] Patch Attachment 329588 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5690242 New failing tests: inspector/dom/highlightRect.html Created attachment 329591 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 329594 [details]
Patch
Comment on attachment 329594 [details] Patch Attachment 329594 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5692347 New failing tests: inspector/dom/highlightSelector.html inspector/dom/highlightQuad.html Created attachment 329596 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 329597 [details]
Patch
I love it how all of these tests run fine on my machine...
Comment on attachment 329597 [details] Patch Attachment 329597 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5693325 Number of test failures exceeded the failure limit. Created attachment 329598 [details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 329599 [details]
Patch
Rebase
Comment on attachment 329599 [details] Patch Attachment 329599 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5694498 New failing tests: webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html Created attachment 329600 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 329619 [details]
Patch
It looks like it's possible for willDeleteProgram and isShaderProgramDisabled to be called on a ShaderProgram that has already been deleted via deleteProgram, so we don't want to ASSERT for an InspectorShaderProgram.
Created attachment 329686 [details]
Patch
Let's see if I can get the win EWS to be happy :P
Comment on attachment 329686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329686&action=review > Source/WebCore/ChangeLog:138 > +22017-12-18 Megan Gardner <megan_gardner@apple.com> Extra 2. > Source/WebCore/ChangeLog:-258 > -2017-12-16 Dan Bernstein <mitz@apple.com> Missing 2. Created attachment 329709 [details]
Patch
Oops.
Comment on attachment 329709 [details] Patch Attachment 329709 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5736444 New failing tests: fast/mediastream/MediaStream-MediaElement-setObject-null.html Created attachment 329720 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 329729 [details]
Patch
Rebase
Comment on attachment 329729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329729&action=review Some open questions. > Source/WebCore/html/CanvasBase.h:29 > +#include "IntSize.h" Is this needed? It is also forward declared below. > Source/WebCore/html/CanvasBase.h:82 > + HashSet<CanvasObserver*> m_observers; I'd have expected this to be private. > Source/WebCore/html/OffscreenCanvas.cpp:45 > +OffscreenCanvas::~OffscreenCanvas() Why isn't this done in ~CanvasBase? > Tools/ChangeLog:8 > + * Scripts/webkitpy/common/config/watchlist: The watchlist stuff could probably be its own unrelated change. No need to include it with a code change like this. Its also super generic given there are 260+ files many (WebGPU). Comment on attachment 329729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329729&action=review >> Source/WebCore/html/OffscreenCanvas.cpp:45 >> +OffscreenCanvas::~OffscreenCanvas() > > Why isn't this done in ~CanvasBase? This isn't done in ~CanvasBase because of what ~HTMLCanvasElement does, specifically that we manually set `m_context = nullptr;`. Due to the inheritance destruction order, having the `CanvasObserver::canvasDestroyed` call be in `~CanvasBase` would mean that the `m_context` would be `nullptr` by the time of the `CanvasObserver::canvasDestroyed` call. As a result, we keep the existing functionality of `~HTMLCanvasElement`. If we added the `CanvasObserver::canvasDestroyed` calls to `~CanvasBase`, each `CanvasObserver::canvasDestroyed` would get called multiple times for the same canvas. Instead, we don't add it to the base and have the subclasses manage the observers themselves (this is also why `m_observers` is protected). Comment on attachment 329729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329729&action=review >>> Source/WebCore/html/OffscreenCanvas.cpp:45 >>> +OffscreenCanvas::~OffscreenCanvas() >> >> Why isn't this done in ~CanvasBase? > > This isn't done in ~CanvasBase because of what ~HTMLCanvasElement does, specifically that we manually set `m_context = nullptr;`. Due to the inheritance destruction order, having the `CanvasObserver::canvasDestroyed` call be in `~CanvasBase` would mean that the `m_context` would be `nullptr` by the time of the `CanvasObserver::canvasDestroyed` call. As a result, we keep the existing functionality of `~HTMLCanvasElement`. > > If we added the `CanvasObserver::canvasDestroyed` calls to `~CanvasBase`, each `CanvasObserver::canvasDestroyed` would get called multiple times for the same canvas. Instead, we don't add it to the base and have the subclasses manage the observers themselves (this is also why `m_observers` is protected). Okay, then I think we really should make this less prone to error that someone subclasses CanvasBase and forgets to notify observers. Worst case something like: class CanvasBase { ... protected: // Subclasses are expected to notify observers in destruction. HashSet<CanvasObserver*> m_observers; void notifyObserversOfDestruction(); #ifndef NDEBUG bool m_didNotifyObserversOfDestruction { false }; #endif ... } With implementation: CanvasBase::~CanvasBase() { ASSERT(m_didNotifyObserversOfDestruction); } void CanvasBase::notifyObserversOfDestruction() { #ifndef NDEBUG ASSERT(!m_didNotifyObserversOfDestruction); m_didNotifyObserversOfDestruction = true; #endif for (auto& observer : m_observers) observer->canvasDestroyed(*this); } Would mean when the next person adds `class FutureCanvas : public <CanvasBase>` we would immediately catch if they didn't handle destruction. --- But all of that said, it would be best to just make this work as expected and make m_observers managed only by CanvasBase. Created attachment 330506 [details]
Patch
Comment on attachment 330506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330506&action=review r=me > Source/WebCore/html/CanvasBase.h:63 > + void addObserver(CanvasObserver&); > + void removeObserver(CanvasObserver&); Style: I'd put these after the list of virtual methods instead of in the middle. And maybe the notifyObserver functions could be made protected instead of public, but public is fine. Created attachment 330519 [details]
Patch
Comment on attachment 330519 [details] Patch Clearing flags on attachment: 330519 Committed r226439: <https://trac.webkit.org/changeset/226439> All reviewed patches have been landed. Closing bug. |