Bug 129080 – Add very basic image control rendering

• Component: Images
• Status: RESOLVED FIXED

[Brady Eidson]

Add very basic image control rendering.

This sets up the mechanism that will be expanded upon later, including layouttest support.

[Brady Eidson]

Created attachment 224707 [details]
Patch v1

[Brady Eidson]

Two style errors will be caught - fixed locally.

[WebKit Commit Bot]

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.

[Brady Eidson]

Created attachment 224716 [details]
Patch v1

[Brady Eidson]

Missed some feature guards in v1, trying again in the second v1 (which I meant to be v2, of course)

[Tim Horton]

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?

[Brady Eidson]

(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()"

[Brady Eidson]

Created attachment 224755 [details]
Patch v3 - More build fixes, address review comments

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

(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.

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

A fair number of the failures are RenderImages getting children that they shouldn't be getting.

[Brady Eidson]

Created attachment 224785 [details]
Patch v4 - More protection against the existence of the mechanism in the common case.

[Brady Eidson]

Created attachment 224787 [details]
Patch v5 - More building

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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("&nbsp", 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?

[Brady Eidson]

(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("&nbsp", 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.

[Brady Eidson]

Created attachment 224800 [details]
Patch v6 - Address review feedback

[Tim Horton]

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.

[Brady Eidson]

(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...!

[Brady Eidson]

https://trac.webkit.org/changeset/164457