1058919 - Add ToString to rect / matrix

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: 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.

Comment 1

[Botond Ballo [:botond]]

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?

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

I agree this is a better approach. Which means it's missing from nsRegion and not the rect types.

Comment 3

[Botond Ballo [: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.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

non trivial change. I changed the formatting a bit.

Comment 5

[Botond Ballo [: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"

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8479453 [details] [diff] [review]
patch

froydnj can you provide an mfbt review?

Comment 7

[Nathan Froyd [:froydnj]]

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.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=9e02ba0a30db

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=fcd7f3bb7df9

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3101d72dba3d

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This broke printing nsRegions on Windows. For some reason the correct << overload is not called and we just print the address of the region.

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

For the record, the issue in comment #11 have been fixed in bug 1185840.