Flash plugins are not resized correctly according to resolution

RESOLVED FIXED in Firefox 29

Status

RESOLVED FIXED
5 years ago
15 days ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

Trunk
Firefox 29
ARM
Android

Details

Attachments

(3 attachments, 3 obsolete attachments)

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.
Created attachment 8359278 [details] [diff] [review]
Add nsPresShell::GetCumulativeResolution (carry r+ from bug 947523)
Attachment #8359278 - Flags: review+
Created attachment 8359279 [details] [diff] [review]
Correctly size plugins on Android according to resolution
Attachment #8359279 - Flags: review?(bugmail.mozilla)
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.
Attachment #8359279 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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
Attachment #8359278 - Flags: approval-mozilla-beta?
Attachment #8359278 - Flags: approval-mozilla-aurora?
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
Attachment #8359279 - Flags: approval-mozilla-beta?
Attachment #8359279 - Flags: approval-mozilla-aurora?
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Flags: needinfo?(benjamin)

Comment 9

5 years ago
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.
Flags: needinfo?(benjamin)
(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?
You can also inline the GetCumulativeResolution function into the call site and make it a branch-specific patch.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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)
Flags: needinfo?(benjamin)

Comment 14

5 years ago
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.
Flags: needinfo?(benjamin)
Created attachment 8363820 [details] [diff] [review]
New patch for Aurora/Beta
Attachment #8363820 - Flags: feedback?(benjamin)

Comment 16

5 years ago
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".
Attachment #8363820 - Flags: feedback?(benjamin) → feedback+
Created attachment 8363905 [details] [diff] [review]
New patch for Aurora/Beta
Attachment #8363820 - Attachment is obsolete: true
Attachment #8363905 - Flags: review?(benjamin)
Created attachment 8363912 [details] [diff] [review]
New patch for Aurora/Beta

Oops.
Attachment #8363905 - Attachment is obsolete: true
Attachment #8363905 - Flags: review?(benjamin)
Attachment #8363912 - Flags: review?(benjamin)

Comment 19

5 years ago
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
Attachment #8363912 - Flags: review?(benjamin) → review+
bajaj, this should be ok to land on Aurora now. Not sure what you want to do about Beta.
Flags: needinfo?(bbajaj)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 ?
Flags: needinfo?(bbajaj)
(In reply to bhavana bajaj [:bajaj] from comment #21)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Created attachment 8364132 [details] [diff] [review]
New patch for Aurora/Beta

Apparently uploaded the wrong patch before.
Attachment #8363912 - Attachment is obsolete: true
Attachment #8364132 - Flags: review+
status-firefox27: --- → fixed
status-firefox28: --- → fixed
status-firefox29: --- → fixed
status-firefox26: --- → wontfix

Updated

5 years ago
Attachment #8359278 - Flags: approval-mozilla-beta?
Attachment #8359278 - Flags: approval-mozilla-beta+
Attachment #8359278 - Flags: approval-mozilla-aurora?
Attachment #8359278 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Attachment #8359279 - Flags: approval-mozilla-beta?
Attachment #8359279 - Flags: approval-mozilla-beta+
Attachment #8359279 - Flags: approval-mozilla-aurora?
Attachment #8359279 - Flags: approval-mozilla-aurora+
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29

Updated

15 days ago
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.