631388 - Investigate not centering plugin images in their frame if the frame/image sizes mismatch

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Spun off from bug 626602.  It's not clear how this helps anything, and Rob alluded to it being a problem, but I didn't get the details there.

If the goal is to minimize the visual difference between sizes-match rendering and a subsequent sizes-mismatch rendering, then there are a few things to consider first.  If we assume that the common case is the <object> top-left staying fixed and width/height changing, then (obviously) the minimum visual difference before/after resize is to leave the image at <0,0>.  I don't know if that's a good assumption.  That's the simplest implementation.

Comment 1

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

For example, go to http://superpokepets.com/spp, create an account and log in. There's Flash object at the top of the page with a kind of menubar. Hover over the menubar and the Flash object will jump around because we've changed the size of the object and the async rendering hasn't caught up.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I housed my monkey in the sky palace.

Yeah, that's pretty jarring.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Stop trying to center plugin images in their frames

Seems to fix superpokepets.

I don't see any risk in taking this patch.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/c3a4a1970f6c

Comment 5

[Marco Bonardo [:mak]]

I backed out this because unfortunately it was conflicting with the backout of bug 626602 and I didn't want to merge a code that I don't know.

Comment 6

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

So I think here we should get rid of all the centering logic, and furthermore we should avoid changing nsDisplayPlugin::GetBounds in nsObjectFrame::BuildLayer, by a) never changing the result of GetBounds based on the imagecontainer, and b) modifying nsDisplayPlugin::GetOpaqueRegion to return false if the mImageSurface dimensions don't match the display item bounds.

Comment 7

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

Attachment: fix v2

More comprehensive fix. Needed for patch in bug 630835 to pass tests reliably.

I'm happy to land this after bug 626602; there will be some merging, but it's probably easier to merge this on top of 626602.

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 512760 [details] [diff] [review]
fix v2

cjones asked if I wanted to look this over. Seems fine to me.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 512760 [details] [diff] [review]
fix v2

> I'm happy to land this after bug 626602; there will be some merging, but it's
> probably easier to merge this on top of 626602.

Bless you, kind sir.

Comment 10

[Benjamin Smedberg]

We should get this landed for beta feedback on the whole set.

Comment 11

[Raul Nicolae Malea]

Attachment: menu and flash windows not stay fixed

Comment 12

[Raul Nicolae Malea]

Confirm this bug.

Comment 13

[(no longer active)]

I tried to land this, but the patch didn't apply cleanly, and it was not easy to merge at all.

Comment 14

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

Sorry yes, this needs more work post-626602. However, I don't think this needs beta coverage.

Comment 15

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

Attachment: Part 1: Add nsIPluginInstance::GetImageSize

GetImageSize will be useful in bug 634844 as well.

Comment 16

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

Attachment: Part 2: make nsDisplayPlugin bounds independent of async rendering state

nsDisplayPlugin::GetOpaqueRegion is necessarily dependent on async rendering state, but this patch makes it depend on nsIPluginInstance::GetImageSize, which shouldn't change during painting.

Comment 17

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

Passed reftests on try.

Comment 18

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

http://hg.mozilla.org/mozilla-central/rev/8b22f4933c31
http://hg.mozilla.org/mozilla-central/rev/9ad79fc34fee

Comment 19

[Raul Nicolae Malea]

Verified. Mozilla/5.0 (Windows NT 6.1; rv:2.0b13pre) Gecko/20110223 Firefox/4.0b13pre ID:20110223030416
Thanks ! Very good job!

Comment 20

[orangezilla]

This has been reported to have fixed the Nike sites on bug 634380, now marked as resolved

thanks to Keldorn on mozillazine
http://forums.mozillazine.org/viewtopic.php?f=23&t=2116361&p=10474673#p10474673

Comment 21

[Maniac Vlad Florin (:vladmaniac)]

(In reply to comment #19)
> Verified. Mozilla/5.0 (Windows NT 6.1; rv:2.0b13pre) Gecko/20110223
> Firefox/4.0b13pre ID:20110223030416
> Thanks ! Very good job!

Confirming as verified