Add ToString to rect / matrix

RESOLVED FIXED in mozilla34

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

Trunk
mozilla34
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8479344 [details] [diff] [review]
patch

Note that I left BaseRect, which lives in moz2d, out because it can't use nsCString which is becoming our defactor return type of ToString.
Attachment #8479344 - Flags: review?(botond)
(Assignee)

Updated

4 years ago
Attachment #8479344 - Attachment is patch: true
Attachment #8479344 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 8479344 [details] [diff] [review]
patch

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

In the Part 3a and 3b patches of bug 961289, I started to go down a different road: I gave BaseRect an 'operator<<(ostream&, BaseRect)', and I introduce a non-member ToString() function in MFBT which output-streamed its argument into a string.

What do you think about using this existing mechanism for rects, and doing something similar to matrices?
(Assignee)

Comment 2

4 years ago
Created attachment 8479401 [details] [diff] [review]
patch

I agree this is a better approach. Which means it's missing from nsRegion and not the rect types.
Assignee: nobody → bgirard
Attachment #8479344 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8479344 - Flags: review?(botond)
Attachment #8479401 - Flags: review?(botond)
Comment on attachment 8479401 [details] [diff] [review]
patch

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

r+ with comments addressed.

Thanks for doing this!

::: gfx/src/nsRegion.h
@@ +68,5 @@
>      return IsEqual(aRgn);
>    }
>  
> +  friend std::ostream& operator<<(std::ostream& stream, const nsRegion& m) {
> +    return stream << m.ToString().get();

I would prefer that these nested the other way - that is, that operator<< did the actual work, and ToString() called it (possibly via the MFBT ToString()).

The reason is that right now, if you call, say, ToCString(myRegion), we're building a string, writing that to a stream, and building a second string.

If you'd prefer leaving this to a follow-up, feel free, but add a TODO comment so we don't forget.

@@ +466,5 @@
>      return IsEqual(aRgn);
>    }
>  
> +  friend std::ostream& operator<<(std::ostream& stream, const nsIntRegion& m) {
> +    return stream << m.ToString().get();

See comment for nsRegion.

::: gfx/thebes/gfx3DMatrix.h
@@ +44,5 @@
> +    return stream << "["
> +           "[" << m._11 << ", " << m._12 << ", " << m._13 << ", " << m._14 << "], "
> +           "[" << m._21 << ", " << m._22 << ", " << m._23 << ", " << m._24 << "], "
> +           "[" << m._31 << ", " << m._32 << ", " << m._33 << ", " << m._34 << "], "
> +           "[" << m._41 << ", " << m._42 << ", " << m._43 << ", " << m._44 << "], "

Can we print the 2D form if the matrix is 2D?

::: mfbt/ToString.h
@@ +26,5 @@
>    stream << aValue;
>    return stream.str();
>  }
>  
> +#define ToCString(v) ToString(v).c_str()

Please add a comment that describes the motivation/use case.
Attachment #8479401 - Flags: review?(botond) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 8479453 [details] [diff] [review]
patch

non trivial change. I changed the formatting a bit.
Attachment #8479401 - Attachment is obsolete: true
Attachment #8479453 - Flags: review?(botond)
Comment on attachment 8479453 [details] [diff] [review]
patch

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

r+ with comments addressed.

It occurs to me that I don't have the authority to review changes to MFBT, so you probably want to ask an MFBT peer to review the ToString.h change as well.

::: gfx/thebes/gfx3DMatrix.h
@@ +40,5 @@
> +    if (m.IsIdentity()) {
> +      return stream << "[ I ]";
> +    }
> +
> +    if (&& m.Is2D()) {

You have a syntax error here.

::: mfbt/ToString.h
@@ +27,5 @@
>    return stream.str();
>  }
>  
> +/**
> + * This is a helper to make it easier to print to stderr.

s/"print to stderr"/"use printf-like functions"

(You can print to stderr just fine with std::cerr without having to unwrap strings to char*).

@@ +29,5 @@
>  
> +/**
> + * This is a helper to make it easier to print to stderr.
> + * This can't be an inline function because we want
> + * automatically tracking of the char*.

s/"we want automatically tracking of the char*"/"we need the memory the char* points to to be freed at the end of the statement"
Attachment #8479453 - Flags: review?(botond) → review+
(Assignee)

Comment 6

4 years ago
Comment on attachment 8479453 [details] [diff] [review]
patch

froydnj can you provide an mfbt review?
Attachment #8479453 - Flags: review?(nfroyd)
Comment on attachment 8479453 [details] [diff] [review]
patch

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

I don't see the compelling-ness of this and the lifetime issues of the returned pointer seem like a footgun.
Attachment #8479453 - Flags: review?(nfroyd) → review-
https://hg.mozilla.org/mozilla-central/rev/3101d72dba3d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
This broke printing nsRegions on Windows. For some reason the correct << overload is not called and we just print the address of the region.
For the record, the issue in comment #11 have been fixed in bug 1185840.
You need to log in before you can comment on or make changes to this bug.