592037 - Remove mSize from gfx***Surface implementations and use cairo functionality directly

Status: NEW

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Joe proposed to remove stored mSize at all from gfx***Surface implemetations 
https://bugzilla.mozilla.org/show_bug.cgi?id=556487#c103

Comment 1

[tom brinkman]

I removed msize from gfxImageSurface and changed gfxASurface::CairoSurface to be a const method.

Note: the gfxImageSurface constructor takes a pointer to an image buffer and constructs a cairo surface which takes this image buffer as a parameter and at the same time gfxImageSurface keeps that buffer as a member variable.  This should not necessary, as the buffer can always be retrieved from the cairo surface object.  Would be a simple matter to clean this up.

Comment 2

[tom brinkman]

Attachment: This patch removes msize from gfxImageSurface

This patch removes msize from gfxImageSurface.  That information is available from the cairo_surface.  No need to store it again.

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 473705 [details] [diff] [review]
This patch removes msize from gfxImageSurface


> gfxSharedImageSurface::gfxSharedImageSurface(const mozilla::ipc::Shmem &aShmem)

>-    if (!InitSurface(PR_FALSE))
>+	gfxIntSize size(shmInfo->width, shmInfo->height);
>+    if (!InitSurface(size, PR_FALSE))
>         NS_RUNTIMEABORT("Shared memory is bad");

Indentation's a little wonky here.

Looks wonderful otherwise. r=joe

Comment 4

[tom brinkman]

Attachment: Fixed tab/spacing Push ME

Small formatting change (push)

Comment 5

[Oleg Romashin (:romaxa)]

Comment on attachment 474263 [details] [diff] [review]
Fixed tab/spacing Push ME

Only indentation fixed... moving r+

Comment 6

[Dão Gottwald [:dao]]

Still needs approval2.0.

Comment 7

[Oleg Romashin (:romaxa)]

Comment on attachment 474263 [details] [diff] [review]
Fixed tab/spacing Push ME

I think it is enough safe for 2.0.
Joe any help with this?

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 474263 [details] [diff] [review]
Fixed tab/spacing Push ME

low risk.

Comment 9

[Oleg Romashin (:romaxa)]

Comment on attachment 474263 [details] [diff] [review]
Fixed tab/spacing Push ME

Patch is not applying on latest m-c.

Also I found some style issues:
>+PRInt32 gfxImageSurface::GetDataSize() const 
>+{ 
>+	return mStride*Height(); 
                      ^^ spaces needed 
>+}
>+
>+gfxIntSize gfxImageSurface::GetSize() const 
                                             ^ whitespace
>+{ 
>+	return gfxIntSize(Width(),Height()); 
  ^ here is the tab.. use spaces  ^ space missing here
Also I think this can be moved into gfxImageSurface header.


>+}
>+
>+PRInt32 gfxImageSurface::Width() const 
                                        ^ white space

>-    const gfxIntSize& GetSize() const { return mSize; }
>-    PRInt32 Width() const { return mSize.width; }
>-    PRInt32 Height() const { return mSize.height; }
>+    gfxIntSize GetSize() const;
>+    PRInt32 Width() const;
>+    PRInt32 Height() const;
This is fail to apply on latest m-c.... because const gfxIntSize& GetSize() const is virtual now...


Could you update patch plz?

Comment 10

[tom brinkman]

Attachment: Fixed some bitrot and formatting issues

This patch is a roll up of the previous two patches.  Fixed bitrot and formatting issues.

Comment 11

[Joe Drew (not getting mail)]

Comment on attachment 476083 [details] [diff] [review]
Fixed some bitrot and formatting issues

This patch doesn't seem to remove mSize

Comment 12

[tom brinkman]

Attachment: Removed msize

Removed msize. It was not removed from the bitrotted version.

Comment 13

[Oleg Romashin (:romaxa)]

I've got crash with this patch... hmm
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1284709363.1284709584.20870.gz&fulltext=1#err0

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 476114 [details] [diff] [review]
Removed msize


>+
> long
> gfxImageSurface::ComputeStride() const
> {
>     long stride;
> 
>     if (mFormat == ImageFormatARGB32)
>-        stride = mSize.width * 4;
>+        stride = Width() * 4;

this will crash if ComputeStride called before image surface created:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1284709363.1284709584.20870.gz&fulltext=1#err0


###!!! ASSERTION: gfxASurface::CairoSurface called with mSurface == nsnull!: 'mSurface != nsnull', file ../../dist/include/gfxASurface.h, line 117
gfxASurface::CairoSurface [gfxASurface.h:118]
gfxImageSurface::Width [gfx/thebes/gfxImageSurface.cpp:148]
gfxImageSurface::ComputeStride [gfx/thebes/gfxImageSurface.cpp:162]
gfxImageSurface::gfxImageSurface [gfx/thebes/gfxImageSurface.cpp:91]
imgFrame::Init [modules/libpr0n/src/imgFrame.cpp:232]


I think we should call ComputeStride with aSize argument because  ComputeStride called usually before image surface created.

Comment 15

[tom brinkman]

Attachment: Moved ComputeStride into base class and made it a static method

Moved ComputeStride into base class and made it a static method.  Fixes crash.

Comment 16

[tom brinkman]

Attachment: Moved ComputeStride into base class and made it a static method.

Moved ComputeStride into base class and made it a static method.  Fixed formatting issues.  Fixed crash.

Comment 17

[Oleg Romashin (:romaxa)]

Comment on attachment 476531 [details] [diff] [review]
Moved ComputeStride into base class and made it a static method.

ok, it seems not cause problems on try server, and don't crash.

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 476531 [details] [diff] [review]
Moved ComputeStride into base class and made it a static method.

Please add a crashtest for the way the previous patch failed too.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 474263 [details] [diff] [review]
Fixed tab/spacing Push ME

(bookkeeping)

Comment 20

[(no longer active)]

The patch no longer applies cleanly on trunk.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:bhood, could you have a look please?
For more information, please visit auto_nag documentation.