Closed Bug 962443 Opened 6 years ago Closed 6 years ago

make frame dump code work on fennec and b2g

Categories

(Core :: Layout, defect)

29 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(2 files)

Attached patch patchSplinter Review
Not having this has been a inconvenience to me for a while. Finally decided to bite the bullet and do it.
Attachment #8363492 - Flags: review?(matspal)
Attached patch addendumSplinter Review
I'd like to have this incorporated into your patch to consolidate
the implementations of "ListTag" to one place.
Comment on attachment 8363492 [details] [diff] [review]
patch

Comment that applies to all files:
I worry that by using a lot of "nsAutoCString" in these methods
we'll use a lot of stack space for deep frame trees.  In most cases
it's unlikely that the default nsAutoCString space will be enough
anyway so I think you should change these to "nsCString". (it should
be easy to do that by just editing the patch file directly)

Now comments per file:

> layout/generic/nsLineBox.cpp
In ListFloats, it looks like you've re-ordered the output of:
>+      fprintf_stderr(out, "\n###!!! NULL out-of-flow frame");
visavi printing 'str' that comes later:
>+    fprintf(out, "%s\n", str.get());

I think the former line should use "str += ...".
Also, the latter should use fprintf_stderr?

Also in ListFloats:
>+      str += nsPrintfCString("%s", NS_LossyConvertUTF16toASCII(frameName).get());
I think this can be simplified to:
  str += NS_ConvertUTF16toUTF8(frameName).get();

> layout/generic/nsFrame.cpp
In nsIFrame::ListTag:
+  aTo += nsPrintfCString("%s@%p", NS_LossyConvertUTF16toASCII(tmp).get(), static_cast<const void*>(aFrame));
This is probably better as two lines:
  aTo += NS_ConvertUTF16toUTF8(tmp).get();
  aTo += nsPrintfCString("@%p", static_cast<const void*>(aFrame));
(or at least put the last arg on a separate line)

Looks good otherwise, thanks for doing this.
r=mats
Attachment #8363492 - Flags: review?(matspal) → review+
Made all those changes. There were a couple other places where fprintf needed to be replaced with fprintf_stderr too.
https://hg.mozilla.org/mozilla-central/rev/bafd46bb640c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.