[webkit-reviews] review denied: [Bug 52594] [Qt] iframe shims don't work : [Attachment 80559] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 52594: [Qt] iframe shims don't work
https://bugs.webkit.org/show_bug.cgi?id=52594

Attachment 80559: Patch
https://bugs.webkit.org/attachment.cgi?id=80559&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80559&action=review

> Source/WebCore/plugins/PluginView.cpp:1531
> +static void getObjectStack(const RenderObject* ro,
> +			      Vector<const RenderObject*>* roStack)

Keep this one line

> Source/WebCore/plugins/PluginView.cpp:1543
> +static bool checkStackOnTop(
> +	   const Vector<const RenderObject*>& iframeZstack,
> +	   const Vector<const RenderObject*>& pluginZstack)

Also here.

I do not like the name very much, what about isIframeAbovePlugin or so? that
seems more webkitish

> Source/WebCore/plugins/PluginView.cpp:1547
> +    for (size_t i1 = 0, i2 = 0;
> +	    i1 < iframeZstack.size() && i2 < pluginZstack.size();
> +	    i1++, i2++) {

Are i1 and i2 ever different?

> Source/WebCore/plugins/PluginView.cpp:1584
> +	       // Inspect the document order.  Later order means higher
> +	       // stacking.

One line

> Source/WebCore/plugins/PluginView.cpp:1609
> +void PluginView::windowCutOutRects(const IntRect& frameRect,
> +						  Vector<IntRect>& cutOutRects)


One line. I'm not sure cutOut rects is the most descriptive name

> Source/WebCore/plugins/PluginView.cpp:1619
> +    // Get the parent widget

I find the comment a bit redundant

> Source/WebCore/plugins/PluginView.cpp:1624
> +    FrameView* parentFrameView = static_cast<FrameView*>(parentWidget);

There is no toFrameView method?

> Source/WebCore/plugins/PluginView.cpp:1633
> +	   const FrameView* frameView =
> +	       static_cast<const FrameView*>((*it).get());

one line :-)

> Source/WebCore/plugins/PluginView.cpp:1649
> +		   IntPoint point =
> +		       roundedIntPoint(iframeRenderer->localToAbsolute());

I would keep this on one line