959223 - Flash plugins are not resized correctly according to resolution

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Plugins, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

When you zoom in on a page with a Flash plugin, the content is scaled accordingly but never resized to draw at the new resolution.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add nsPresShell::GetCumulativeResolution (carry r+ from bug 947523)

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Correctly size plugins on Android according to resolution

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8359278 [details] [diff] [review]
Add nsPresShell::GetCumulativeResolution (carry r+ from bug 947523)

Review of attachment 8359278 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsIPresShell.h
@@ +1254,5 @@
>    virtual nsresult SetResolution(float aXResolution, float aYResolution) = 0;
>    gfxSize GetResolution() { return gfxSize(mXResolution, mYResolution); }
>    float GetXResolution() { return mXResolution; }
>    float GetYResolution() { return mYResolution; }
> +  virtual gfxSize GetCumulativeResolution() = 0;

Make sure you bump the IID in this file.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Comment on attachment 8359278 [details] [diff] [review]
> Add nsPresShell::GetCumulativeResolution (carry r+ from bug 947523)
> 
> Review of attachment 8359278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsIPresShell.h
> @@ +1254,5 @@
> >    virtual nsresult SetResolution(float aXResolution, float aYResolution) = 0;
> >    gfxSize GetResolution() { return gfxSize(mXResolution, mYResolution); }
> >    float GetXResolution() { return mXResolution; }
> >    float GetYResolution() { return mYResolution; }
> > +  virtual gfxSize GetCumulativeResolution() = 0;
> 
> Make sure you bump the IID in this file.

Weird, I thought I did that, but don't see it in the patch. Nice catch, thanks.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7bd965a6c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/afb247393486

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8359278 [details] [diff] [review]
Add nsPresShell::GetCumulativeResolution (carry r+ from bug 947523)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: blurry Flash content
Testing completed (on m-c, etc.): m-c for a day
Risk to taking this patch (and alternatives if risky): None, only adds API
String or IDL/UUID changes made by this patch: nsIPresShell UUID changed

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8359279 [details] [diff] [review]
Correctly size plugins on Android according to resolution

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: blurry Flash content
Testing completed (on m-c, etc.): m-c for a day
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 8

[bhavana bajaj [:bajaj]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #6)
> Comment on attachment 8359278 [details] [diff] [review]
> Add nsPresShell::GetCumulativeResolution (carry r+ from bug 947523)
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): unknown
> User impact if declined: blurry Flash content
> Testing completed (on m-c, etc.): m-c for a day
> Risk to taking this patch (and alternatives if risky): None, only adds API
> String or IDL/UUID changes made by this patch: nsIPresShell UUID changed

NI on :bsmedberg to see if we have any alternatives here or if this patch would is ok as the general rule is we do not allow UUID changes on beta.

Comment 9

[Benjamin Smedberg]

I don't think we can take this on beta as-is. It's clear from all of the MOZILLA_INTERNAL_API #ifdefs in nsIPresShell.h that there are consumers who use this from outside libxul, so it's possible (and even likely) that extensions are using this and the changes here would break them.

The "obvious" next step is to have a temporary nsIPresShell_MOZILLA27 interface with it's own IID and do a little QIing in the one place you need to call it. Since nsPresShell is the only implementer of nsIPresShell, I don't think that woul be a huge burden.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> I don't think we can take this on beta as-is. It's clear from all of the
> MOZILLA_INTERNAL_API #ifdefs in nsIPresShell.h that there are consumers who
> use this from outside libxul, so it's possible (and even likely) that
> extensions are using this and the changes here would break them.
> 
> The "obvious" next step is to have a temporary nsIPresShell_MOZILLA27
> interface with it's own IID and do a little QIing in the one place you need
> to call it. Since nsPresShell is the only implementer of nsIPresShell, I
> don't think that woul be a huge burden.

I don't actually don't need to query it by the ID. I already have a pointer instance. In that case maybe it's best to just leave the ID alone?

Comment 11

[Kartikaya Gupta (email:[e-mail])]

You can also inline the GetCumulativeResolution function into the call site and make it a branch-specific patch.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> You can also inline the GetCumulativeResolution function into the call site
> and make it a branch-specific patch.

Yeah I think this may be the best way to go actually.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ugh, it's apparently not really kosher to include nsPresShell.h from DOM. We need that to get the parent PresShell.

NI on bsmedberg for just nuking the UUID change (comment #10)

Comment 14

[Benjamin Smedberg]

If you add the virtual method on nsIPresShell you need to rev the IID. If you add it just to nsPresShell.h or to a new class nsIPresShell_MOZILLA27 then you can leave it alone.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: New patch for Aurora/Beta

Comment 16

[Benjamin Smedberg]

Comment on attachment 8363820 [details] [diff] [review]
New patch for Aurora/Beta

I'm going to mark r+ with changes. 

>diff --git a/dom/plugins/base/nsPluginInstanceOwner.cpp b/dom/plugins/base/nsPluginInstanceOwner.cpp
>   // NotifySize() causes Flash to do a bunch of stuff like ask for surfaces to render
>   // into, set y-flip flags, etc, so we do this at the beginning.
>-  float xResolution = mObjectFrame->PresContext()->GetRootPresContext()->PresShell()->GetXResolution();
>-  float yResolution = mObjectFrame->PresContext()->GetRootPresContext()->PresShell()->GetYResolution();
>-  ScreenSize screenSize = (r * LayoutDeviceToScreenScale(xResolution, yResolution)).Size();
>+  gfxSize resolution = reinterpret_cast<nsIPresShell_MOZILLA27*>(mObjectFrame->PresContext()->PresShell())->GetCumulativeResolution();

We need to replace the reinterpret_cast with a static_cast. reinterpret_cast is making assumptions about vtable layouts which aren't safe in this context.

>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h

>+class nsIPresShell_MOZILLA27 {

So this should inherit from nsIPresShell, e.g.

class nsIPresShell_MOZILLA27 : public nsIPresShell


> class PresShell : public nsIPresShell,
>+                  public nsIPresShell_MOZILLA27,

And then this can just be "class PresShell : public nsIPresShell_MOZILLA27".

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: New patch for Aurora/Beta

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: New patch for Aurora/Beta

Oops.

Comment 19

[Benjamin Smedberg]

Comment on attachment 8363912 [details] [diff] [review]
New patch for Aurora/Beta

There's still a reinterpret_cast in nsPluginInstanceOwner.cpp which needs to be fixed. r=me with that

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

bajaj, this should be ok to land on Aurora now. Not sure what you want to do about Beta.

Comment 21

[bhavana bajaj [:bajaj]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #20)
> bajaj, this should be ok to land on Aurora now. Not sure what you want to do
> about Beta.

Are you suggesting that the updated patch to accommodate acceptable UUID change as suggested in comment #10 do not apply to beta ?

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to bhavana bajaj [:bajaj] from comment #21)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #20)
> > bajaj, this should be ok to land on Aurora now. Not sure what you want to do
> > about Beta.
> 
> Are you suggesting that the updated patch to accommodate acceptable UUID
> change as suggested in comment #10 do not apply to beta ?

No, it's fine for beta too, but wasn't sure if it was too late now. We can land it.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: New patch for Aurora/Beta

Apparently uploaded the wrong patch before.

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8f6bdf8db3d
https://hg.mozilla.org/releases/mozilla-beta/rev/85dea29cbde9