Closed Bug 1596339 Opened 2 years ago Closed 2 years ago

Implement operator<< for logical types in WritingMode.h


(Core :: Layout, task, P3)




Tracking Status
firefox72 --- fixed


(Reporter: TYLin, Assigned: TYLin)



(7 files)

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.

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

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.

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

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

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

Depends on D52964

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?

Flags: needinfo?(aethanyc)

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

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?

Flags: needinfo?(aethanyc) → needinfo?(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?

Flags: needinfo?(dholbert)
Pushed by
Part 1 - Implement operator<< for WritingMode, and use it in frame tree dump. r=dholbert
Part 2 - Implement operator<< for LogicalRect, and use it in frame tree dump. r=dholbert
Part 3 - Implement operator<< for LogicalSize, and use it in frame tree dump. r=dholbert
Part 4 - Implement operator<< for LogicalPoint and LogicalMargin. r=dholbert
Part 5 - Use ToString() on nsRect in frame tree dump. r=dholbert
Part 6 - Use ToString() on nsPoint and nsSize in frame tree dump. r=dholbert
Part 7 - Delete unused operator<< for nsRect. r=dholbert
You need to log in before you can comment on or make changes to this bug.