1150246 - Make nsPluginFrame::GetPaintedRect explicitly use mozilla::gfx::IntSize rather than the ambiguous nsIntSize.

Status: RESOLVED WONTFIX

(Core :: Layout, defect, P4)

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Yay, finally a call site that used nsIntSize correctly ;-)

Nevertheless, I think explicitly using gfx::IntSize is preferred
in these cases.  Alternatively, we could use 'gfxIntSize' in
layout/DOM, which is a typedef to the same type.  It's slightly
shorter and already used in some places.  I would hate to see
people adding "using mozilla::gfx" in layout code and then use
IntSize directly just because typing gfx:: is too inconvenient.
I think that would lead us back to ambiguity b/c it's unitless.
And it's easy to misread without the "gfx" qualifier.

Comment 2

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fad4f2aa6ba

Comment 3

[Daniel Holbert [:dholbert]]

I'm unclear on why "GetCurrentImageSize()" in this code should return gfx::IntSize, whereas HTMLVideoElement::GetVideoSize should return CSSIntSize (in bug 1150144).  What's the difference?

Comment 4

[Mats Palmgren (inactive)]

In this case GetCurrentImageSize() returns a value that we actually get
from GFX -- gfxASurface::GetSize().  In the GetVideoSize case the size comes
from the embedded meta data in the media, and if it says "this is 300x400
pixels" we always interpret that as CSS pixels AFAICT from the code.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #4)
> In this case GetCurrentImageSize() returns a value
> that we actually get from GFX -- gfxASurface::GetSize()

(True, though that call is buried under several additional levels of non-GFX abstraction. GetCurrentImageSize() calls nsNPAPIPluginInstance::GetImageSize, which calls PluginLibrary::GetImageSize(), which (as-implemented in PluginModuleParent) does call gfxASurface::GetSize().)

> In the GetVideoSize case [...] if it says "this is 300x400
> pixels" we always interpret that as CSS pixels AFAICT from the code.

OK -- but, that same logic applies here (for dev pixels instead of CSS pixels): when we call GetCurrentImageSize(), we only ever interpret the returned size as dev pixels. (The only call is in nsPluginFrame.cpp, and we immediately convert the return value using "DevPixelsToAppUnits".)

So perhaps nsPluginInstanceOwner::GetCurrentImageSize() should actually return LayoutDeviceSize, not a dimensionless IntSize?

Or: maybe both nsPluginInstanceOwner::GetCurrentImageSize and HTMLVideoElement::GetVideoSize should return dimensionless gfx::IntSize, and the callers can choose what sort of pixels they want to use for the returned sizes? That seems possibly saner than having all these layers of plugin code have to worry about LayoutDeviceSize.

In any case -- it seems like we should try to establish a rule about when we convert into a unitted size and apply it as consistently as possible, and I'm still not seeing the fundamental difference between this API and HTMLVideoElement::GetVideoSize().

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8587057 [details] [diff] [review]
fix

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

If we do take the atttached patch (or something like it), though, I think it needs a bit more to be complete:

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +316,2 @@
>    if (mInstance) {
>      mInstance->GetImageSize(&size);

mInstance has type 'nsRefPtr<nsNPAPIPluginInstance>' -- so I think think we need to change the nsNPAPIPluginInstance::GetImageSize function signature as well, right?

(Technically we can still compile without doing that, because nsIntSize is just a typedef for gfx::IntSize, per nical's recent dev.platform post.  But as long as we're making the distinction here, we should make the same distinction on the other side of this function-call as well.)

Similarly: nsNPAPIPluginInstance::GetImageSize calls PluginLibrary::GetImageSize, so we'd need to make the same change to that function and its implementations as well, to keep the types matching.)

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix, v2

(In reply to Daniel Holbert [:dholbert] from comment #5)
> OK -- but, that same logic applies here (for dev pixels instead of CSS
> pixels): when we call GetCurrentImageSize(), we only ever interpret the
> returned size as dev pixels.

Fair enough.


(In reply to Daniel Holbert [:dholbert] from comment #6)
> mInstance has type 'nsRefPtr<nsNPAPIPluginInstance>' -- so I think think we
> need to change the nsNPAPIPluginInstance::GetImageSize function signature as
> well, right?

Well, I'm trying to make the minimal changes necessary in other modules.
So, I'll make the patch smaller instead!

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #7)
> Well, I'm trying to make the minimal changes necessary in other modules.
> So, I'll make the patch smaller instead!

nsPluginInstanceOwner is already *in* another module, though (dom/plugins).  And nsNPAPIPluginInstance is alongside it. So -- if we change nsPluginInstanceOwner::GetCurrentImageSize, I think we'd also need to change nsNPAPIPluginInstance::GetImageSize (the function that it's wrapping).  Otherwise, we end up with two functions which essentially do the same thing (one being an "owner" abstraction for the other), but return different types (and the "owner" doing some boilerplate conversion). And that feels messy & confusing.

Basically -- my thinking here (implied at the end of comment 5) is: I don't really feel comfortable r+'ing an outside-of-layout return-type change like this, unless:

 (1) it's based on a clear rule that delineates which sorts of functions should return which types (css vs. dev vs. unitless), so that the change is clearly a step in the right direction.

 (2) when changing a function, we apply that rule consistently to the function's data-sources where it makes sense (nsNPAPIPluginInstance etc. in this case), so that the change doesn't take us to a confusing place where we have two functions that do the same thing but return different types for no clear reason.

(In (2), the data-source functions could be changed in followup patches, of course. I'm not sure that's part of the plan here, though, or that it should be.)

My concern is that, without (1) & (2), we may end up making a type-conversion like this partway into an API, and we just stop at some arbitrary point, after we've gotten a few steps from layout. Then, the type-conversion at that point (a few steps from layout) seems arbitrary & meaningless.


One possible "rule" we could be using here is: keep abstract sizes unitless & abstract, until we get to the part of the (probably-layout) code where we are actually dealing with dev pixels or css pixels or whatever.  (This is closer to what your original patch did, I think, though my concern there was that we were applying the unit-change inconsistently, and it seemed to contradict our GetVideoSize change. I think the original patch would make sense if we made GetVideoSize consistent [returning unitless "pixels of media data"] and if we also had a followup on converting the rest of the stack here to gfx::IntSize for consistency.)

Comment 9

[Mats Palmgren (inactive)]

OK.  I think I'll leave the code here as is for now until someone decides what
type of pixels dom/plugins will use (at which point they'll hopefully fix this
line for us).

Comment 10

[Daniel Holbert [:dholbert]]

Cool, that sounds good.

(Though -- if you like, I'd be on board with instantiating a LayoutDeviceSize or LayoutDeviceRect in nsPluginFrame::GetPaintedRect(), and then calling ToAppUnits() on that, to replace our existing DevPixelsToAppUnits() calls there.  At that point, it's clear that we're dealing with dev pixels, so creating a local LayoutDevice[Size|Rect] variable clearly makes sense.)