1596339 - Implement operator<< for logical types in WritingMode.h

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Description

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

I notice WritingMode, LogicalRect, LogicalPoint, LogicalSize, and LogicalMargin do not implement operator<<. They can not convert to a string via mozilla::ToString.

After their operator<< are implemented, the frame tree dump in nsIFrame::ListGeneric and nsLineBox::List can be simplified.

Comment 1

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

Attachment: Bug 1596339 Part 1 - Implement operator<< for WritingMode, and use it in frame tree dump. r=dholbert

WritingMode.h already depends on ostream header implicitly via
nsBidiUtils.h -> nsString.h. For completeness, I still add #include
<ostream>.

While I'm here, I make the format of debug prints in nsLineBox more
consistent with the counter-part in nsFrame. Some of them will be
revised in the later patches.

Comment 2

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

Attachment: Bug 1596339 Part 2 - Implement operator<< for LogicalRect, and use it in frame tree dump. r=dholbert

This change uses braces to enclose the dimension of LogicalRect. This
match the output of nsRect's operator<<.

Note: This introduces inconsistency in the frame tree dump because some
of the output format still use brackets to enclose the data. But in
later patches, I'll gradually change the format to use braces.

Depends on D52962

Comment 3

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

Attachment: Bug 1596339 Part 3 - Implement operator<< for LogicalSize, and use it in frame tree dump. r=dholbert

Note: I change the output format of BaseSize to use the format like
"(3,5)" instead "3 x 5" so that it is consistent with other types.

Depends on D52963

Comment 4

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

Attachment: Bug 1596339 Part 4 - Implement operator<< for LogicalPoint and LogicalMargin. r=dholbert

I'm not aware of any usage of LogicalPoint and LogicalPoint in frame
tree dump, but I still want to implement them for the sake of
completeness.

Depends on D52964

Comment 5

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

Attachment: Bug 1596339 Part 5 - Use ToString() on nsRect in frame tree dump. r=dholbert

Depends on D52965

Comment 6

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

Attachment: Bug 1596339 Part 6 - Use ToString() on nsPoint and nsSize in frame tree dump. r=dholbert

Depends on D52966

Comment 7

[Daniel Holbert [:dholbert]]

Is this really simplifying things? It feels like it might be adding extra layers of abstraction/wrappers/heap-allocated strings...

e.g. in the first chunk of the first patch, the current code has:

    aTo += nsPrintfCString(" wm=%s logical-size={%d,%d}", wm.DebugString(),
                           ISize(), BSize());

Here, wm.DebugString() is just a simple function that returns a char*, pointing to a statically known string literal.

Whereas the new code has:

    aTo += nsPrintfCString(" wm=%s logical-size={%d,%d}", ToString(wm).c_str(),
                           ISize(), BSize());

Here, the new expression (ToString(wm).c_str()) expression is, under the hood:

• starting with a statically known string literal
• passing it to an ostringstream
• returning a std::string for that ostringstream (which I think involves some heap allocation)
• and then gets a char* pointer off of that std::string

This seems significantly more complicated/expensive, as compared to just using the char* pointer directly. It's also iffy in that it's using an API that produces std::string which we don't tend to use.

Is there a higher-level win that this gets us somewhere, that makes it worth this added complication?

Comment 8

[Daniel Holbert [:dholbert]]

I guess things do get a bit visually-cleaner in part 5 and 6, which is nice.

The pipeline here does feel a bit wacky, though... It's roughly the following, I think:
[Data, perhaps a char*] --> ostringstream --> std::string --> char* --> nsPrintfCString --> nsCString --> char* --> fprintf

If feels to me like if we're using string-streams, then we should just be outputting the final data via a stream as well, in ::List (if it's possible to wrap our FILE* out in a stream). That would let us do away with some of the intermediate conversions here, I think -- particularly the nsPrintfCString and nsCString.

Alternately: if we're sticking with nsPrintfCString / nsCString as the "final form" of our output here (which gets handed off to fprintf), then it feels odd to be moving towards introducing intermediate streams & std::string usages only to have to convert out of them into mozilla string types and then into a fprintf expression...

Comment 9

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

I agree that given our existing frame dump interfaces are using nsPrintfCString / nsCString, it seems unnecessary to introduce interfaces using streams.

However, my motivation is that after adding operator<< to all the types in WritingMode.h, I can easily dump those types in my own debug print simply by calling std::cout << "logical rect " << logicalRect; or printf("logical rect %s", ToString(logicalRect).c_str());. The modification to the existing frame tree dump is a by-product to prove the operator<<s actually work.

If you agree, I still like to implement operator<< for types in WritingMode.h (as the bug title suggests), but drop the modification to nsIFrame::ListGeneric and nsLineBox::ListTag. Also I'll keep WritingMode::DebugString() intact. What do you think?

Comment 10

[Daniel Holbert [:dholbert]]

OK - that motivation makes sense and makes this patch stack a reasonable step, I think.

I'll...drop the modification to nsIFrame::ListGeneric and nsLineBox::ListTag. Also I'll keep WritingMode::DebugString() intact

Hmm, I would bother making those changes to the patch-stack, actually...

I think as long as we're adding these operators, it'd be good to:
(a) have some in-tree usages to increase the likelihood that they continue to work (so it's good for List*** to use them)
(b) avoid duplicated code (so it's good to get rid of DebugString())

Which I think means I'm good with the spirit of the original patch-stack -- though we should perhaps also add an // XXX comment in ListGeneric to point out that the string conversions are a bit awkward & could perhaps be simplified if we adjusted our frame-tree-dumping functions to use a stream for output?

Comment 11

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

Attachment: Bug 1596339 Part 7 - Delete unused operator<< for nsRect. r=dholbert

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/591e273e2d9e
Part 1 - Implement operator<< for WritingMode, and use it in frame tree dump. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/583f09d7e181
Part 2 - Implement operator<< for LogicalRect, and use it in frame tree dump. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2b8b36ac14d5
Part 3 - Implement operator<< for LogicalSize, and use it in frame tree dump. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e9e0e6b92381
Part 4 - Implement operator<< for LogicalPoint and LogicalMargin. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1622b327fb35
Part 5 - Use ToString() on nsRect in frame tree dump. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a7a60ea3a7e0
Part 6 - Use ToString() on nsPoint and nsSize in frame tree dump. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1e1617c67238
Part 7 - Delete unused operator<< for nsRect. r=dholbert

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/591e273e2d9e
https://hg.mozilla.org/mozilla-central/rev/583f09d7e181
https://hg.mozilla.org/mozilla-central/rev/2b8b36ac14d5
https://hg.mozilla.org/mozilla-central/rev/e9e0e6b92381
https://hg.mozilla.org/mozilla-central/rev/1622b327fb35
https://hg.mozilla.org/mozilla-central/rev/a7a60ea3a7e0
https://hg.mozilla.org/mozilla-central/rev/1e1617c67238