959121 - Moz2Dify ImageClientSingle::UpdateImage

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ali Ak]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Steps to reproduce:

Moz2Dify ImageClientSingle::UpdateImage. Change usages of gfxASurface to SourceSurface.

Comment 1

[Ali Ak]

Attachment: Moz2Dify ImageClientSingle::UpdateImage

Comment 2

[Ali Ak]

try: https://tbpl.mozilla.org/?tree=Try&rev=84e411f839ad

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8359690 [details] [diff] [review]
Moz2Dify ImageClientSingle::UpdateImage

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

Be sure to land this after all Image classes have their GetAsSurface ported to GetAsSourceSurface.

::: gfx/layers/client/ImageClient.cpp
@@ +225,3 @@
>      MOZ_ASSERT(surface);
>  
>      gfx::IntSize size = gfx::IntSize(image->GetSize().width, image->GetSize().height);

It's not part of this changeset but I notice that Image::GetSize now already returns a gfx::IntSize, can you also simplify this line into

gfx::IntSize size = image->GetSize();

in a followup patch?

Comment 4

[Ali Ak]

(In reply to Nicolas Silva [:nical] from comment #3)
> Comment on attachment 8359690 [details] [diff] [review]
> Moz2Dify ImageClientSingle::UpdateImage
> 
> Review of attachment 8359690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Be sure to land this after all Image classes have their GetAsSurface ported
> to GetAsSourceSurface.

Should I assume same condition for bug 959120 then? (since I assume it's to avoid regressions we won't know about until after all Image subclasses implement GetAsSourceSurface)

> 
> ::: gfx/layers/client/ImageClient.cpp
> @@ +225,3 @@
> >      MOZ_ASSERT(surface);
> >  
> >      gfx::IntSize size = gfx::IntSize(image->GetSize().width, image->GetSize().height);
> 
> It's not part of this changeset but I notice that Image::GetSize now already
> returns a gfx::IntSize, can you also simplify this line into
> 
> gfx::IntSize size = image->GetSize();
> 
> in a followup patch?

For sure!

Comment 5

[Nicolas Silva [:nical]]

(In reply to Ali Ak from comment #4)
> (In reply to Nicolas Silva [:nical] from comment #3)
> > Comment on attachment 8359690 [details] [diff] [review]
> > Moz2Dify ImageClientSingle::UpdateImage
> > 
> > Review of attachment 8359690 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Be sure to land this after all Image classes have their GetAsSurface ported
> > to GetAsSourceSurface.
> 
> Should I assume same condition for bug 959120 then? (since I assume it's to
> avoid regressions we won't know about until after all Image subclasses
> implement GetAsSourceSurface)
> 

I might be confused about what's landing and in what order, but I meant that since this code uses GetAsSurface now, replacing it by GetAsSourceSurface must happen when GetAsSourceSurface covers all the use cases of GetAsSurface. Or does GetAsSourceSurface fallback to converting the result of GetAsSurface using gfxPlatform magic? I don't remember and GetAsSourceSurface has not landed yet so I am not sure. Well anyway try servers will catch it if we run into a case where GetAsSourceSurface returns nullptr and GetAsSurface would have returned something. Ignore me :) my brain is a bit fried right now.

Comment 6

[Ali Ak]

(In reply to Nicolas Silva [:nical] from comment #5)
> Or does GetAsSourceSurface fallback to converting the result
> of GetAsSurface using gfxPlatform magic? I don't remember and
> GetAsSourceSurface has not landed yet so I am not sure. Well anyway try
> servers will catch it if we run into a case where GetAsSourceSurface returns
> nullptr and GetAsSurface would have returned something. Ignore me :) my
> brain is a bit fried right now.

Hehe no problem, but yes, GetAsSourceSurface wraps GetAsSurface (well DeprecatedGetAsSurface now) and it's landed now. So the idea is to implement all the subclasses GetAsSourceSurface properly now (meta bug 960053), then remove the DeprecatedGetAsSurface calls slowly.

Comment 7

[Ali Ak]

Attachment: Moz2Dify ImageClientSingle::UpdateImage. r=nical.

Comment 8

[Ali Ak]

Attachment: Moz2Dify ImageClientSingle::UpdateImage. r=nical.

Comment 9

[Ali Ak]

https://tbpl.mozilla.org/?tree=Try&rev=c64e017a48e8

Comment 10

[Ali Ak]

try: https://tbpl.mozilla.org/?tree=Try&rev=c64e017a48e8

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8858624d813a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8858624d813a