616884 - plugins shown when we don't want them to be on Linux

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

This causes some ugly artifacts during page transition. I have a fix.

Comment 1

[Timothy Nikkel (:tnikkel)]

nsObjectFrame::CreateWidget creates the plugin widget with bounds (0,0,0,0). Then in nsPluginInstanceOwner::CreateWidget we initially place the plugin widget at 0,0 with the correct size and empty clip region (nothing shown, everything clipped out). But in nsWindow::SetWindowClipRegion we exit early because the existing clip region appears to be the same as the new region, because the existing clip region is zero width and height because it is the whole widget rect which hasn't been updated yet since nsObjectFrame::CreateWidget created it as (0,0,0,0).

When going back to a page with a plugin (maybe not all plugins) this creates a short period of time where a rect at (0,0,plugin width,plugin height) has the old page contents but the rest is the new page.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Set the clip properly on Linux.

If we haven't set a clip yet on the window we should not exit early if the existing and new regions are equal.

Comment 3

[Timothy Nikkel (:tnikkel)]

Part 1 alone fixes the bug I was seeing.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Use the position of the object frame instead of 0,0.

In the case I was debugging we created the plugin widget from an action triggered by nsObjectFrame::DidReflow. So in nsPluginInstanceOwner::CreateWidget we should try to use the position of the object frame instead of just putting the widget at 0,0.

Without part 1 this improves the bug I was seeing by making the artifact be where the plugin is. But with part 1 this doesn't fix any problems that I've seen.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Always store the clip rects on Windows.

While looking at this code I noticed something on Windows.

nsWindow::SetWindowClipRegion doesn't store the new rects if it is asked to intersect with the existing clip. Since we only use this function by calling it with intersect = true following by intersect = false in ConfigureChildren I think that this cannot cause anything to go wrong. So if we don't take this and there isn't a good reason for the existing behaviour we should probably add a comment mentioning this in the code.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 496016 [details] [diff] [review]
Part 3. Always store the clip rects on Windows.

I think we should just add the comment.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Add a comment noting slightly wrong code.

Ok, I will land this comment addition instead of part 3.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should add a comment saying if you call it with TRUE for the intersect parameter then you should call it with FALSE soon afterwards because it doesn't updated the stored region.

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: Add a comment noting the clip region isn't updated.

Ok.

Comment 10

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/291782dcd3f2
http://hg.mozilla.org/mozilla-central/rev/45239c06759c
http://hg.mozilla.org/mozilla-central/rev/5eaab51cd937