944908 - Fix mingw compilation in gfx/2d/.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jacek Caban]

Attachment: patch.diff

- Log::operator<< is required for long to print HRESULT (otherwise the call is ambiguous)
- GetImageForSurface takes Matrix& parameter, so its real instance is needed

Comment 1

[Andreas Gal :gal]

Comment on attachment 8340653 [details] [diff] [review]
patch.diff

Why does constructing a temporary object doesn't work there?

Comment 2

[Robert Longson [:longsonr]]

It should take const reference. That's why constructing a temporary doesn't work.

Comment 3

[Markus Stange [:mstange]]

I agree with Robert. Please change GetImageForSurface to take a const Matrix&.

Comment 4

[Andreas Gal :gal]

Yup lets fix the callee.

Comment 5

[Jacek Caban]

Attachment: patch.diff

Thanks for reviews. GetImageForSurface calls CreatePartialBitmapForSurface with aSourceTransform argument and CreatePartialBitmapForSurface modifies the matrix (by design), so we need a separated instance in GetImageForSurface to handle that.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8341003 [details] [diff] [review]
patch.diff

Sorry, I dropped the ball on this one. I'm forwarding this to Bas because he knows more about this code.
To me it looks like we really do need to propagate the changed transform to the caller of GetImageForSurface, but then that caller should do something with it instead of ignoring it. Bas, is that correct or can the case I'm worried about never occur?

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 8341003 [details] [diff] [review]
patch.diff

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

I'm confused, GetImageForSurface -needs- to edit aSourceTransform (through use of CreatePartialBitmapForSurface), we can't just have a temporary and ignore the changes made there as far as I can tell? Maybe I'm misunderstanding things. Please let me know if I am :)

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 8341003 [details] [diff] [review]
patch.diff

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

I set the wrong flag, sorry :)

Comment 9

[Jacek Caban]

Attachment: patch.diff

OK, then we need to fix caller to not pass a temporary instance of Matrix. This is done by the first version of my patch. Since there are more places that ignore the matrix, this patch uses another approach by adding a new helper.

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 8350013 [details] [diff] [review]
patch.diff

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

There's theoretically a little bug in here, but you didn't add that, that was already there, so this patch looks fine to me.

Comment 11

[Jacek Caban]

Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f4c4fbfbe23

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7f4c4fbfbe23