Bug 108368 - Implement a custom appearance for the snapshotted plugin background
Summary: Implement a custom appearance for the snapshotted plugin background
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-01-30 12:28 PST by Dean Jackson
Modified: 2013-03-08 02:08 PST (History)
10 users (show)

See Also:


Attachments
Patch (17.39 KB, patch)
2013-03-07 15:02 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (20.15 KB, patch)
2013-03-07 18:32 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch for landing (22.43 KB, patch)
2013-03-07 19:15 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Another Patch for Landing (22.44 KB, patch)
2013-03-07 19:32 PST, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2013-01-30 12:28:48 PST
Turn the blur behind the label background back on.
Comment 1 Radar WebKit Bug Importer 2013-01-30 12:29:13 PST
<rdar://problem/13117802>
Comment 2 Simon Fraser (smfr) 2013-02-01 10:58:41 PST
Why was it removed?
Comment 3 Dean Jackson 2013-02-01 11:40:08 PST
(In reply to comment #2)
> Why was it removed?

Moving to a shadow tree solution meant I needed to query the dimensions/location of the label element in order to know where to blur. Previously we always drew the label in the same place. I figured it was better to do this in a separate patch.
Comment 4 Dean Jackson 2013-03-07 14:44:57 PST
Retitling from "Reinstate blur for snapshotted plugin label background" to "Implement a custom renderer for the snapshotted plugin background".

Why? Because we shouldn't hardcode an effect into WebCore. By having a custom element in the shadow tree, we can allow ports to do anything they want to the element via their injected CSS. This could be a filter effect, like blur, or an animation, etc.

The custom renderer should paint the same as the snapshotted plugin, allowing the content to position the label where ever they like, and have it seamlessly blend into the element content.
Comment 5 Dean Jackson 2013-03-07 15:02:12 PST
Created attachment 192089 [details]
Patch
Comment 6 Dean Jackson 2013-03-07 15:35:42 PST
Comment on attachment 192089 [details]
Patch

temporarily removing r? to see if i can avoid having a custom element
Comment 7 Dean Jackson 2013-03-07 18:32:53 PST
Created attachment 192123 [details]
Patch
Comment 8 Tim Horton 2013-03-07 18:46:49 PST
Comment on attachment 192123 [details]
Patch

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.
Comment 9 Dean Jackson 2013-03-07 18:56:54 PST
(In reply to comment #8)
> (From update of attachment 192123 [details])
> 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.

Fixed.

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

Fixed.

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

Yes :(

> 
> > Source/WebCore/rendering/RenderThemeMacShared.mm:1748
> > +    HTMLPlugInElement* plugInElement = static_cast<HTMLPlugInElement*>(parent);
> 
> Ditto (if there is one?).

I made one.

> 
> > Source/WebCore/rendering/RenderThemeMacShared.mm:1752
> > +    HTMLPlugInImageElement* plugInImageElement = static_cast<HTMLPlugInImageElement*>(plugInElement);
> 
> Ditto (if there is one?).

I made one.

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

Done.

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

In fact, that one isn't even necessary.

> 
> Ditto.
Comment 10 Dean Jackson 2013-03-07 19:15:01 PST
Created attachment 192130 [details]
Patch for landing
Comment 11 Dean Jackson 2013-03-07 19:18:24 PST
Eric, I hear you are looking at static_casts and toBlah() methods. I added some to HTMLPlugInElement and HTMLPlugInImageElement here.
Comment 12 Early Warning System Bot 2013-03-07 19:21:17 PST
Comment on attachment 192130 [details]
Patch for landing

Attachment 192130 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17054303
Comment 13 Early Warning System Bot 2013-03-07 19:21:32 PST
Comment on attachment 192130 [details]
Patch for landing

Attachment 192130 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16990308
Comment 14 Dean Jackson 2013-03-07 19:32:44 PST
Created attachment 192132 [details]
Another Patch for Landing
Comment 15 Dean Jackson 2013-03-08 02:08:51 PST
Committed r145196: <http://trac.webkit.org/changeset/145196>