Bug 111242 – Plugins that are appropriately large w.r.t page size should autostart

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Dean Jackson]

A "full-page" flash site should never snapshot. The trick is getting the definition of full-page correct. I suggest something like the following:

- plugin is in the main frame (not an iframe)
- plugin is sized with %, and those are bigger than say 95
- the final size of the plugin is again about 95% of the viewport width and height

[Dean Jackson]

<rdar://problem/13328176>

[Dean Jackson]

Created attachment 191092 [details]
Patch

[Dean Jackson]

Created attachment 191093 [details]
Patch

[Tim Horton]

Comment on attachment 191093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191093&action=review

> Source/WebCore/ChangeLog:15
> +        - The plug-in is in the main frame (not an iframe).
> +        - The plug-in is sized using %, and both width and height are above 96%.
> +        - The final width and height of the plug-in is more than 95% of the viewport
> +          width and height respectively.

This seems strange. What's the downside to something like thresholding area(intersection(plugin rect, frame visible rect)) / area(frame visible rect)?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:430
> +    RenderEmbeddedObject* renderEmbeddedObject = toRenderEmbeddedObject(renderer());

Technically you could go to renderbox instead if you wanted, I think... not that that changes anything.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:441
> +    if (inMainFrame && styleWidth.isPercent() && (styleWidth.percent() > sizingFullPageThresholdPercentage)
> +        && styleHeight.isPercent() && (styleHeight.percent() > sizingFullPageThresholdPercentage)
> +        && (static_cast<float>(contentWidth) / visibleViewSize.width() * 100 > sizingFullPageThresholdPercentage)
> +        && (static_cast<float>(contentHeight) / visibleViewSize.height() * 100 > sizingFullPageThresholdPercentage)) {

That's quite the condition. Is there anything we can do to make this less insane?

[Jon Lee]

Comment on attachment 191093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191093&action=review

> Source/WebCore/ChangeLog:13
> +        - The plug-in is sized using %, and both width and height are above 96%.

Given that I think all the sites that use percentages use 100%, we should just go for that instead.

>> Source/WebCore/ChangeLog:15
>> +          width and height respectively.
> 
> This seems strange. What's the downside to something like thresholding area(intersection(plugin rect, frame visible rect)) / area(frame visible rect)?

Checking the height and width separately is more restrictive, which I think is good, but I think we do need to clip the width and height against the visible frame rect. Consider a full-page size plugin that's initially placed mostly along the bottom border of the page.

> Source/WebCore/ChangeLog:20
> +        the tests above.

How about considering the border box of the plugin element instead?

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:441
>> +        && (static_cast<float>(contentHeight) / visibleViewSize.height() * 100 > sizingFullPageThresholdPercentage)) {
> 
> That's quite the condition. Is there anything we can do to make this less insane?

We might have to extend this to clip the width and height against the frame.

[Maciej Stachowiak]

(In reply to comment #4)
> (From update of attachment 191093 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191093&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        - The plug-in is in the main frame (not an iframe).
> > +        - The plug-in is sized using %, and both width and height are above 96%.
> > +        - The final width and height of the plug-in is more than 95% of the viewport
> > +          width and height respectively.
> 
> This seems strange. What's the downside to something like thresholding area(intersection(plugin rect, frame visible rect)) / area(frame visible rect)?

Tim's rule seems more logical to me too, but maybe there is a reason for Dean's?

[Dean Jackson]

Comment on attachment 191093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191093&action=review

>>>> Source/WebCore/ChangeLog:15
>>>> +          width and height respectively.
>>> 
>>> This seems strange. What's the downside to something like thresholding area(intersection(plugin rect, frame visible rect)) / area(frame visible rect)?
>> 
>> Checking the height and width separately is more restrictive, which I think is good, but I think we do need to clip the width and height against the visible frame rect. Consider a full-page size plugin that's initially placed mostly along the bottom border of the page.
> 
> Tim's rule seems more logical to me too, but maybe there is a reason for Dean's?

I have no attachment to the logic I posted. I'll upload a patch that compares area ratios.

>> Source/WebCore/ChangeLog:20
>> +        the tests above.
> 
> How about considering the border box of the plugin element instead?

I should have been a little more clear. zombo.com is an extremely simple page. The "border" is just the default 8px margin on the <body>. The plugin takes up all the available space it can, but if the window size is, say 200x200, then the plugin can only get 186x186.

Most full-page plugin sites do margin:0;padding;0 then object 100% * 100%, so I feel like this will be a fairly rarely hit rule.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:430
>> +    RenderEmbeddedObject* renderEmbeddedObject = toRenderEmbeddedObject(renderer());
> 
> Technically you could go to renderbox instead if you wanted, I think... not that that changes anything.

That's true! I just moved the code around, but I'll swap to RenderBox as well.

>>> Source/WebCore/html/HTMLPlugInImageElement.cpp:441
>>> +        && (static_cast<float>(contentHeight) / visibleViewSize.height() * 100 > sizingFullPageThresholdPercentage)) {
>> 
>> That's quite the condition. Is there anything we can do to make this less insane?
> 
> We might have to extend this to clip the width and height against the frame.

It does look over-complex, but it isn't really too bad. How about I separate it into some nested tests? (unfortunately we're testing FOR something here, so I can't return early).

We need the logic to make sure we don't miss plugins that are 100% by 100% but contained in an element that provides the sizing (that isn't the body).

[Dean Jackson]

Created attachment 191126 [details]
Patch

[Dean Jackson]

New patch uses area calculation, which makes the logic look a little more friendly. And RenderBox rather than RenderEmbeddedObject.

[Dean Jackson]

It seems this comment in the review didn't get posted here:

I was going to add the intersection, but I'm not sure there would be many cases where it is different from just the plugin rect. Remember that we're already testing for 100% w/h, so it would be unusual to have a bunch of content on the page and then a 100% plugin.

[Jon Lee]

(In reply to comment #10)
> It seems this comment in the review didn't get posted here:
> 
> I was going to add the intersection, but I'm not sure there would be many cases where it is different from just the plugin rect. Remember that we're already testing for 100% w/h, so it would be unusual to have a bunch of content on the page and then a 100% plugin.

Ok! Unofficial r=me.

[Brady Eidson]

Comment on attachment 191126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191126&action=review

> Source/WebCore/ChangeLog:19
> +        This is definitely not foolproof. For example, zombo.com has a slight
> +        border around its plug-in. As the window size gets smaller, the body margin
> +        takes up more than 5% of the width or height, and the plug-in doesn't pass
> +        the tests above.

How could you not fix zombo com???  You are supposed to be able to do anything there!!!

[Dean Jackson]

Committed r144577: <http://trac.webkit.org/changeset/144577>