Fix mingw compilation in gfx/2d/.

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jacek, Assigned: jacek)

Tracking

Trunk
mozilla29
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

4.83 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 8340653 [details] [diff] [review]
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
Attachment #8340653 - Flags: review?(mstange)

Comment 1

5 years ago
Comment on attachment 8340653 [details] [diff] [review]
patch.diff

Why does constructing a temporary object doesn't work there?
It should take const reference. That's why constructing a temporary doesn't work.
I agree with Robert. Please change GetImageForSurface to take a const Matrix&.

Comment 4

5 years ago
Yup lets fix the callee.
(Assignee)

Comment 5

5 years ago
Created attachment 8341003 [details] [diff] [review]
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.
Attachment #8340653 - Attachment is obsolete: true
Attachment #8340653 - Flags: review?(mstange)
Attachment #8341003 - Flags: review?(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?
Attachment #8341003 - Flags: review?(mstange) → review?(bas)
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 :)
Attachment #8341003 - Flags: review?(bas) → review+
Comment on attachment 8341003 [details] [diff] [review]
patch.diff

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

I set the wrong flag, sorry :)
Attachment #8341003 - Flags: review+ → review-
(Assignee)

Comment 9

5 years ago
Created attachment 8350013 [details] [diff] [review]
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.
Attachment #8341003 - Attachment is obsolete: true
Attachment #8350013 - Flags: review?(bas)
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.
Attachment #8350013 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/7f4c4fbfbe23
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.