Plugin in iframe may not display

[John Bauman]

Plugin in iframe may not display

[John Bauman]

Created attachment 188462 [details]
Patch

[John Bauman]

I'm not certain this is the best way to do this, though it seems to work.

Is there a way to do a layout test with a plugin with chromium?

[James Robinson]

(In reply to comment #2)
> I'm not certain this is the best way to do this, though it seems to work.
> 
> Is there a way to do a layout test with a plugin with chromium?

Yes - you can instantiate an NPAPI plugin with mime type "application/x-webkit-test-netscape" in a layout test.  See LayoutTests/plugins/ for some examples.  That will instantiate an instance of the plugin defined in Tools/DumpRenderTree/TestNetscapePlugIn/.

[Simon Fraser (smfr)]

Comment on attachment 188462 [details]
Patch

This scares me a little. We call widgetPositionsUpdated() after scrolling (main frame if we have fixed, and overflow scrolling). Seems like this could be a significant perf hit if you have frames with lots of plugins.

[John Bauman]

Yeah, I'm not sure how much this changes the performance. We already do a similar thing with frameRectsChanged on scroll, so at least the overall time complexity isn't different. And I don't know if we would expect putting a plugin inside an iframe would improve performance during layout over keeping it outside.

I could try to add a separate method to do this that's only called after layout finishes, if you want.

[John Bauman]

Created attachment 188660 [details]
Patch

[Darin Adler]

Comment on attachment 188660 [details]
Patch

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

I’m not enough of an expert on our scrolling to know if this will cause performance problems on some platforms or not, so I can’t give this review+ but I do have a few comments.

> Source/WebCore/ChangeLog:11
> +        Changing the cliprect on a layer containing an iframe may change the
> +        cliprect of plugins inside the iframe, so recursively tell all plugins
> +        in iframes that their layout has changed after doing layout on the
> +        outer frame.

Seems obviously wrong that our comment here says “tell plug-ins that their layout has changed”, but the code says “tell plug-ins that the widget positions have changed”, which is not the same thing. This also could be the crux of the bug, that clipping changes should be included too, not just position changes. So I think a rename may be in order.

> Source/WebCore/ChangeLog:17
> +        (WebCore):

Please remove bogus lines like this from the change log. The script is there to help you write a good change log, but if it spits out nonsense like this, please delete it. And consider fixing the script or asking someone else to do that.

> Source/WebCore/ChangeLog:19
> +        (ScrollView):

Ditto.

> Source/WebCore/platform/ScrollView.h:243
> +    virtual void widgetPositionsUpdated();

Should specify OVERRIDE.

[Darin Adler]

Comment on attachment 188660 [details]
Patch

review- because apparently Simon Fraser had the same concern I did, and he’s more of a scrolling expert. We should not do this unless we’re sure it doesn’t cause scrolling problems.

[Darin Adler]

(In reply to comment #8)
> We should not do this unless we’re sure it doesn’t cause scrolling problems.

We should not do this *in this way* ...

[John Bauman]

Created attachment 188683 [details]
Patch

[Simon Fraser (smfr)]

It would be nice if you could describe an actual bug on a real web page that is fixed by this patch.

[John Bauman]

Sorry, the bug that was reported is that Flash ads in the Youtube sidebar are sometimes not visible - https://code.google.com/p/chromium/issues/detail?id=168900 (though that bug report isn't visible to the public for various reasons).

[John Bauman]

Created attachment 188688 [details]
Patch

Checking again, it seems like other ports would also have this issue, so fixing them as well

[Early Warning System Bot]

Comment on attachment 188688 [details]
Patch

Attachment 188688 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16586456

[EFL EWS Bot]

Comment on attachment 188688 [details]
Patch

Attachment 188688 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16592424

[Build Bot]

Comment on attachment 188688 [details]
Patch

Attachment 188688 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16603238

[Build Bot]

Comment on attachment 188688 [details]
Patch

Attachment 188688 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16585844

[John Bauman]

Created attachment 189323 [details]
Patch

Fix build

[Build Bot]

Comment on attachment 189323 [details]
Patch

Attachment 189323 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16647428

New failing tests:
plugins/plugin-clip-subframe.html

[Build Bot]

Comment on attachment 189323 [details]
Patch

Attachment 189323 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16698530

New failing tests:
plugins/plugin-clip-subframe.html

[John Bauman]

Created attachment 190209 [details]
Patch

Fix mac WK1 and add WK2 expectations.

[Eric Seidel (no email)]

Crazy.  Patch looks sane though.

[Simon Fraser (smfr)]

Comment on attachment 190209 [details]
Patch

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

> Source/WebCore/plugins/PluginView.cpp:164
> +void PluginView::clipRectChanged()
> +{
> +    updatePluginWidget();
> +}

Why doesn't this call the base class function?

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:227
> +    Widget::clipRectChanged();

Is this necessary? It would be nice to have a virtual function on Widget that doesn't require remembering to call the base class.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1590
> +    virtual void clipRectChanged()
> +    {
> +        // Changing the clip rect doesn't affect the view hierarchy, so the plugin must be told about the change directly.
> +        [(WebBaseNetscapePluginView *)platformWidget() updateAndSetWindow];
> +    }

Ditto.

[John Bauman]

Thanks for the review.

(In reply to comment #23)
> (From update of attachment 190209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190209&action=review
> 
> 
> > Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:227
> > +    Widget::clipRectChanged();
> 
> Is this necessary? It would be nice to have a virtual function on Widget that doesn't require remembering to call the base class.
> 
You're right, this really shouldn't be necessary. I'll remove  it.

[John Bauman]

Created attachment 190583 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190583 [details]
Patch

Clearing flags on attachment: 190583

Committed r144236: <http://trac.webkit.org/changeset/144236>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.