Add very basic image control rendering. This sets up the mechanism that will be expanded upon later, including layouttest support.
Created attachment 224707 [details] Patch v1
Two style errors will be caught - fixed locally.
Attachment 224707 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/RenderImage.h:68: The parameter name "shouldLayoutControls" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/html/shadow/ImageControls.h:38: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 224716 [details] Patch v1
Missed some feature guards in v1, trying again in the second v1 (which I meant to be v2, of course)
Comment on attachment 224716 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=224716&action=review > LayoutTests/fast/images/image-controls-basic.html:2 > +if (window.testRunner) { no braces > LayoutTests/fast/images/image-controls-basic.html:8 > +<img src="resources/green-256x256.jpg" webkitexperimentalimagemenu> is this right? all scrunched up? > Source/WebCore/html/HTMLImageElement.cpp:465 > + if (!imageControls) > + return; When is this ever going to happen?
(In reply to comment #6) > (From update of attachment 224716 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224716&action=review > > > LayoutTests/fast/images/image-controls-basic.html:2 > > +if (window.testRunner) { > > no braces K. > > > LayoutTests/fast/images/image-controls-basic.html:8 > > +<img src="resources/green-256x256.jpg" webkitexperimentalimagemenu> > > is this right? all scrunched up? It is what I intended. Is it right? I don't know. > > > Source/WebCore/html/HTMLImageElement.cpp:465 > > + if (!imageControls) > > + return; > > When is this ever going to happen? If the document isn't in a page, or if there's an exception when building up the shadow dom. Are these likely? No. Are they possible? Yes. Maybe it should be "maybeCreate()"
Created attachment 224755 [details] Patch v3 - More build fixes, address review comments
Comment on attachment 224755 [details] Patch v3 - More build fixes, address review comments Attachment 224755 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4793928880488448 New failing tests: fullscreen/video-controls-timeline.html http/tests/misc/acid2-pixel.html http/tests/misc/acid2.html media/audio-controls-rendering.html fullscreen/video-controls-drag.html fast/events/media-element-focus-tab.html media/track/track-kind.html media/track/track-cue-rendering-horizontal.html fast/css/acid2-pixel.html media/track/track-cue-rendering-snap-to-lines-not-set.html plugins/object-embed-plugin-scripting.html media/controls-without-preload.html fast/repaint/obscured-background-no-repaint.html media/audio-delete-while-slider-thumb-clicked.html fast/css/acid2.html fast/css/contentDivWithChildren.html media/track/track-cue-nothing-to-render.html accessibility/media-element.html media/media-controls-clone.html fast/layers/video-layer.html media/track/track-word-breaking.html css2.1/20110323/inline-block-replaced-height-008.htm media/track/track-cue-rendering-rtl.html media/controls-strict.html media/track/track-cue-container-rendering-position.html media/controls-drag-timebar.html fast/hidpi/video-controls-in-hidpi.html css2.1/t0801-c412-hz-box-00-b-a.html fullscreen/video-controls-override.html fast/hidpi/image-set-in-content-dynamic.html
Created attachment 224768 [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
(In reply to comment #9) > (From update of attachment 224755 [details]) > Attachment 224755 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/4793928880488448 > > New failing tests: > fullscreen/video-controls-timeline.html > http/tests/misc/acid2-pixel.html > http/tests/misc/acid2.html > media/audio-controls-rendering.html > fullscreen/video-controls-drag.html > fast/events/media-element-focus-tab.html > media/track/track-kind.html > media/track/track-cue-rendering-horizontal.html > fast/css/acid2-pixel.html > media/track/track-cue-rendering-snap-to-lines-not-set.html > plugins/object-embed-plugin-scripting.html > media/controls-without-preload.html > fast/repaint/obscured-background-no-repaint.html > media/audio-delete-while-slider-thumb-clicked.html > fast/css/acid2.html > fast/css/contentDivWithChildren.html > media/track/track-cue-nothing-to-render.html > accessibility/media-element.html > media/media-controls-clone.html > fast/layers/video-layer.html > media/track/track-word-breaking.html > css2.1/20110323/inline-block-replaced-height-008.htm > media/track/track-cue-rendering-rtl.html > media/controls-strict.html > media/track/track-cue-container-rendering-position.html > media/controls-drag-timebar.html > fast/hidpi/video-controls-in-hidpi.html > css2.1/t0801-c412-hz-box-00-b-a.html > fullscreen/video-controls-override.html > fast/hidpi/image-set-in-content-dynamic.html Only 12 of these 30 fail for me... *great* Exploring those 12.
Comment on attachment 224755 [details] Patch v3 - More build fixes, address review comments Attachment 224755 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6576718265450496 New failing tests: fullscreen/video-controls-timeline.html http/tests/misc/acid2-pixel.html http/tests/misc/acid2.html media/audio-controls-rendering.html fullscreen/video-controls-drag.html fast/events/media-element-focus-tab.html media/track/track-kind.html media/track/track-cue-rendering-horizontal.html fast/spatial-navigation/snav-media-elements.html fast/css/acid2-pixel.html media/track/track-cue-rendering-snap-to-lines-not-set.html plugins/object-embed-plugin-scripting.html media/audio-delete-while-slider-thumb-clicked.html fast/css/acid2.html media/controls-after-reload.html fast/css/contentDivWithChildren.html media/track/track-cue-nothing-to-render.html accessibility/media-element.html fast/layers/video-layer.html media/track/track-word-breaking.html fast/events/tabindex-focus-blur-all.html css2.1/20110323/inline-block-replaced-height-008.htm media/track/track-cue-rendering-rtl.html media/controls-strict.html media/track/track-cue-container-rendering-position.html media/controls-drag-timebar.html fullscreen/video-controls-override.html css2.1/t0801-c412-hz-box-00-b-a.html fast/hidpi/image-set-in-content-dynamic.html fast/hidpi/video-controls-in-hidpi.html
Created attachment 224773 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
A fair number of the failures are RenderImages getting children that they shouldn't be getting.
Created attachment 224785 [details] Patch v4 - More protection against the existence of the mechanism in the common case.
Created attachment 224787 [details] Patch v5 - More building
Comment on attachment 224787 [details] Patch v5 - More building View in context: https://bugs.webkit.org/attachment.cgi?id=224787&action=review > Source/WebCore/html/shadow/ImageControls.h:37 > +class ImageControls : public HTMLDivElement { I think this should be called ImageControlsElement or ImageControlsRootElement
Comment on attachment 224787 [details] Patch v5 - More building View in context: https://bugs.webkit.org/attachment.cgi?id=224787&action=review > Source/WebCore/html/HTMLImageElement.cpp:455 > + toRenderImage(renderObject)->setHasShadowControls(m_experimentalImageMenuEnabled); Won't this have been called already if we made controls in a previous call? > Source/WebCore/html/HTMLImageElement.cpp:479 > + Node* node = shadowRoot->firstChild(); > + ASSERT_WITH_SECURITY_IMPLICATION(!node || node->isImageControls()); > + if (node) > + shadowRoot->removeChild(node); if (Node* node=) {...} ? > Source/WebCore/html/shadow/mac/ImageControlsMac.cpp:55 > + controls->setInnerHTML(" ", ec); Very gross to make a round trip through the parser from this code, even if it's temporary. > Source/WebCore/html/shadow/mac/ImageControlsMac.cpp:58 > + controls->setAttribute(HTMLNames::styleAttr, "position: relative; background-color: red; width: 20px; height: 20px;"); Are we doing to hardcode all the style for the controls, or can we style them through some injected bundle magic? > Source/WebCore/rendering/RenderImage.cpp:133 > + if (isHTMLImageElement(element)) > + m_hasShadowControls = toHTMLImageElement(element).hasShadowControls(); How does m_hasShadowControls get updated if the image element gains controls dynamically? > Source/WebCore/rendering/RenderImage.cpp:717 > + RenderBox* controlsRenderer = toRenderBox(firstChild()); > + if (!controlsRenderer) > + return; > + > + bool controlsNeedLayout = controlsRenderer->needsLayout(); > + // If the region chain has changed we also need to relayout the controls to update the region box info. > + // FIXME: We can do better once we compute region box info for RenderReplaced, not only for RenderBlock. > + const RenderFlowThread* flowThread = flowThreadContainingBlock(); > + if (flowThread && !controlsNeedLayout) { > + if (flowThread->pageLogicalSizeChanged()) > + controlsNeedLayout = true; > + } > + > + LayoutSize newSize = contentBoxRect().size(); > + if (newSize == oldSize && !controlsNeedLayout) > + return; > + > + // When calling layout() on a child node, a parent must either push a LayoutStateMaintainter, or > + // instantiate LayoutStateDisabler. Since using a LayoutStateMaintainer is slightly more efficient, > + // and this method might be called many times per second during video playback, use a LayoutStateMaintainer: > + LayoutStateMaintainer statePusher(view(), *this, locationOffset(), hasTransform() || hasReflection() || style().isFlippedBlocksWritingMode()); > + > + if (needsCustomLayoutMetrics()) { > + controlsRenderer->setLocation(LayoutPoint(borderLeft(), borderTop()) + LayoutSize(paddingLeft(), paddingTop())); > + controlsRenderer->style().setHeight(Length(newSize.height(), Fixed)); > + controlsRenderer->style().setWidth(Length(newSize.width(), Fixed)); > + } > + > + controlsRenderer->setNeedsLayout(MarkOnlyThis); > + controlsRenderer->layout(); > + clearChildNeedsLayout(); > + > + statePusher.pop(); Please move all this into a separate function: layoutShadowControls or something. > Source/WebCore/rendering/RenderImage.h:67 > + void setHasShadowControls(bool hasShadowControls) { m_hasShadowControls = hasShadowControls; } Odd that this is public if the renderer gets the state from the element. > Source/WebKit/mac/WebView/WebPreferences.mm:599 > + [NSNumber numberWithBool:YES], WebKitImageControlsEnabledPreferenceKey, Shouldn't this default to NO? > Source/WebKit2/ChangeLog:10 > + * WebProcess/InjectedBundle/InjectedBundle.cpp: > + (WebKit::InjectedBundle::overrideBoolPreferenceForTestRunner): Expose the > + imageControlsEnabled setting to WKTR. Why no changes to WK2 preferences code?
(In reply to comment #18) > (From update of attachment 224787 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224787&action=review > > > Source/WebCore/html/HTMLImageElement.cpp:455 > > + toRenderImage(renderObject)->setHasShadowControls(m_experimentalImageMenuEnabled); > > Won't this have been called already if we made controls in a previous call? This is leftover from an earlier version of the patch before the RenderImage preemptively grabbed the setting itself. I can probably move it into the controls create/destroy methods now. I will try that and verify. > > > Source/WebCore/html/HTMLImageElement.cpp:479 > > + Node* node = shadowRoot->firstChild(); > > + ASSERT_WITH_SECURITY_IMPLICATION(!node || node->isImageControls()); > > + if (node) > > + shadowRoot->removeChild(node); > > if (Node* node=) {...} ? Sure! > > Source/WebCore/html/shadow/mac/ImageControlsMac.cpp:55 > > + controls->setInnerHTML(" ", ec); > > Very gross to make a round trip through the parser from this code, even if it's temporary. > > > Source/WebCore/html/shadow/mac/ImageControlsMac.cpp:58 > > + controls->setAttribute(HTMLNames::styleAttr, "position: relative; background-color: red; width: 20px; height: 20px;"); > > Are we doing to hardcode all the style for the controls, or can we style them through some injected bundle magic? I'm sure we'll get to using some external resource magic, much like MediaControls does. Seems like a great separate step from this patch, though. > > > Source/WebCore/rendering/RenderImage.cpp:133 > > + if (isHTMLImageElement(element)) > > + m_hasShadowControls = toHTMLImageElement(element).hasShadowControls(); > > How does m_hasShadowControls get updated if the image element gains controls dynamically? In the code you made your very first comment about; Where HTMLImageElement does it directly. > > Source/WebCore/rendering/RenderImage.cpp:717 > > + RenderBox* controlsRenderer = toRenderBox(firstChild()); > > + if (!controlsRenderer) > > + return; > > + > > + bool controlsNeedLayout = controlsRenderer->needsLayout(); > > + // If the region chain has changed we also need to relayout the controls to update the region box info. > > + // FIXME: We can do better once we compute region box info for RenderReplaced, not only for RenderBlock. > > + const RenderFlowThread* flowThread = flowThreadContainingBlock(); > > + if (flowThread && !controlsNeedLayout) { > > + if (flowThread->pageLogicalSizeChanged()) > > + controlsNeedLayout = true; > > + } > > + > > + LayoutSize newSize = contentBoxRect().size(); > > + if (newSize == oldSize && !controlsNeedLayout) > > + return; > > + > > + // When calling layout() on a child node, a parent must either push a LayoutStateMaintainter, or > > + // instantiate LayoutStateDisabler. Since using a LayoutStateMaintainer is slightly more efficient, > > + // and this method might be called many times per second during video playback, use a LayoutStateMaintainer: > > + LayoutStateMaintainer statePusher(view(), *this, locationOffset(), hasTransform() || hasReflection() || style().isFlippedBlocksWritingMode()); > > + > > + if (needsCustomLayoutMetrics()) { > > + controlsRenderer->setLocation(LayoutPoint(borderLeft(), borderTop()) + LayoutSize(paddingLeft(), paddingTop())); > > + controlsRenderer->style().setHeight(Length(newSize.height(), Fixed)); > > + controlsRenderer->style().setWidth(Length(newSize.width(), Fixed)); > > + } > > + > > + controlsRenderer->setNeedsLayout(MarkOnlyThis); > > + controlsRenderer->layout(); > > + clearChildNeedsLayout(); > > + > > + statePusher.pop(); > > Please move all this into a separate function: layoutShadowControls or something. Sounds reasonable. > > Source/WebCore/rendering/RenderImage.h:67 > > + void setHasShadowControls(bool hasShadowControls) { m_hasShadowControls = hasShadowControls; } > > Odd that this is public if the renderer gets the state from the element. The element also sets this on the renderer, too (see the code you made your first comment about) > > Source/WebKit/mac/WebView/WebPreferences.mm:599 > > + [NSNumber numberWithBool:YES], WebKitImageControlsEnabledPreferenceKey, > > Shouldn't this default to NO? YES!! (It should be NO!!) > > Source/WebKit2/ChangeLog:10 > > + * WebProcess/InjectedBundle/InjectedBundle.cpp: > > + (WebKit::InjectedBundle::overrideBoolPreferenceForTestRunner): Expose the > > + imageControlsEnabled setting to WKTR. > > Why no changes to WK2 preferences code? Landed separately in https://trac.webkit.org/changeset/164364 Thanks for the review! Next patch coming up.
Created attachment 224800 [details] Patch v6 - Address review feedback
Comment on attachment 224800 [details] Patch v6 - Address review feedback View in context: https://bugs.webkit.org/attachment.cgi?id=224800&action=review > Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:24 > + */ space below this line > Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:51 > + return 0; nullptr. > Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:57 > + controls->appendChild(Text::create(document, ""), ec); Early return? ASSERT_NO_EXCEPTION? No idea.
(In reply to comment #21) > (From update of attachment 224800 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224800&action=review > > > Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:24 > > + */ > > space below this line Done > > > Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:51 > > + return 0; > Done. > > > Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:57 > > + controls->appendChild(Text::create(document, ""), ec); > > Early return? ASSERT_NO_EXCEPTION? No idea. The very next line does an early return...!
https://trac.webkit.org/changeset/164457