[webkit-reviews] review granted: [Bug 108368] Implement a custom appearance for the snapshotted plugin background : [Attachment 192123] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <timothy_horton at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 108368: Implement a custom appearance for the snapshotted plugin background
https://bugs.webkit.org/show_bug.cgi?id=108368

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

------- Additional Comments from Tim Horton <timothy_horton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=192123&action=review


Seems reasonable to me! Would really like more tests.

> Source/WebCore/rendering/RenderThemeMacShared.mm:1717
> +    LayoutUnit cWidth = renderBlock->contentWidth();

Leading c looks so weird.

> Source/WebCore/rendering/RenderThemeMacShared.mm:1729
> +    IntRect alignedRect = pixelSnappedIntRect(rect);

Is pixel-snapping right here?

> Source/WebCore/rendering/RenderThemeMacShared.mm:1740
> +    HTMLElement* plugInOverlay =
static_cast<HTMLElement*>(renderBlock->node());

Please use toHTMLElement.

> Source/WebCore/rendering/RenderThemeMacShared.mm:1742
> +    while (parent && !parent->isPluginElement())

Weird that this isn’t capitalized the same as everything else (but obviously
not your fault).

> Source/WebCore/rendering/RenderThemeMacShared.mm:1748
> +    HTMLPlugInElement* plugInElement =
static_cast<HTMLPlugInElement*>(parent);

Ditto (if there is one?).

> Source/WebCore/rendering/RenderThemeMacShared.mm:1752
> +    HTMLPlugInImageElement* plugInImageElement =
static_cast<HTMLPlugInImageElement*>(plugInElement);

Ditto (if there is one?).

> Source/WebCore/rendering/RenderThemeMacShared.mm:1764
> +    RenderBox* renderBox = static_cast<RenderBox*>(renderBlock);

Ditto.

> Source/WebCore/rendering/RenderThemeMacShared.mm:1770
> +	   renderBox = static_cast<RenderBox*>(renderBox->parentBox());

Ditto.