[webkit-reviews] review denied: [Bug 129080] Add very basic image control rendering : [Attachment 224787] Patch v5 - More building

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 129080: Add very basic image control rendering
https://bugs.webkit.org/show_bug.cgi?id=129080

Attachment 224787: Patch v5 - More building
https://bugs.webkit.org/attachment.cgi?id=224787&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=224787&action=review


> Source/WebCore/html/HTMLImageElement.cpp:455
> +   
toRenderImage(renderObject)->setHasShadowControls(m_experimentalImageMenuEnable
d);

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?