Closed Bug 959121 Opened 11 years ago Closed 11 years ago

Moz2Dify ImageClientSingle::UpdateImage

Categories

(Core :: Graphics: Layers, defect)

28 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: ali, Assigned: ali)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 947194
Depends on: 958489
Assignee: nobody → ali
Attachment #8359690 - Flags: review?(nical.bugzilla)
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?
Attachment #8359690 - Flags: review?(nical.bugzilla) → review+
(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!
(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.
(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.
Attachment #8359690 - Attachment is obsolete: true
Attachment #8360934 - Attachment is obsolete: true
Blocks: 960524
No longer blocks: 947194
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8858624d813a
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: