Closed Bug 1275831 Opened 4 years ago Closed 4 years ago

[DEBUG only] Repair block layout debug flags in nsBlockDebugFlags.h

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: chenpighead, Assigned: chenpighead)

Details

Attachments

(6 files, 1 obsolete file)

While hacking in layout code, I'd turn on nsBlockDebugFlags (see [1]) from time to time. It appears some of them are broken, and some of them are even not used anymore. I would like to fix this. Here is the plan:

1. Turn on each of these flags one-by-one, so I can verify if all the flags work as usual. If any error is found, fix it.
2. Remove the unused flags.


[1] https://dxr.mozilla.org/mozilla-central/rev/d6d4e8417d2fd71fdf47c319b7a217f6ace9d5a5/layout/generic/nsBlockDebugFlags.h
Summary: Repair and refactor nsBlockDebugFlags → [DEBUG only] Repair and refactor nsBlockDebugFlags
Review commit: https://reviewboard.mozilla.org/r/55374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55374/
Attachment #8756752 - Flags: review?(dholbert)
Attachment #8756753 - Flags: review?(dholbert)
Attachment #8756754 - Flags: review?(dholbert)
Attachment #8756755 - Flags: review?(dholbert)
Attachment #8756756 - Flags: review?(dholbert)
Attachment #8756757 - Flags: review?(dholbert)
Add back a helper function for listing framelist tags. This function has been
called under other flags as well.

Review commit: https://reviewboard.mozilla.org/r/55382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55382/
1.Remove NOISY_FIRST_LINE, NOISY_MAX_ELEMENT_SIZE ,NOISY_MAXIMUM_WIDTH.
2.Remove REALLY_NOISY_FIRST_LINE.
    Only one static function, which has no caller, but defined under this flag.
3.Move REALLY_NOISY_REFLOW_CHILD into REALLY_NOISY_REFLOW.
    We don't have REALLY_NOISY_REFLOW_CHILD anymore, so move this under
    REALLY_NOISY_REFLOW.

Review commit: https://reviewboard.mozilla.org/r/55384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55384/
https://reviewboard.mozilla.org/r/55378/#review52124

::: layout/generic/nsBlockFrame.cpp:1735
(Diff revision 1)
>                                     aBEndEdgeOfChildren, areas);
>    }
>  
>  #ifdef NOISY_COMBINED_AREA
>    ListTag(stdout);
> -  printf(": ca=%d,%d,%d,%d\n", area.x, area.y, area.width, area.height);
> +  nsRect& vis = areas.VisualOverflow();

Might be better to use `const nsRect& vis` and `const nsRect& scr`.
Maybe we could file a followup to further refactor all these flags in the form of what we did for GECKO_BLOCK_DEBUG_FLAGS [1]. These may gain us few advantages:

1. Get these flags and codes under protection of automatic tests (DEBUG build)
2. We can turn on/off these flags at runtime by exporting environment variables


[1] https://dxr.mozilla.org/mozilla-central/rev/8d0aadfe7da782d415363880008b4ca027686137/layout/generic/nsBlockFrame.cpp#127-214
Firstly: thanks for doing this cleanup work!

One commit message nit, which applies to all parts here:

> Bug 1275831 - part1: fix NOISY_FLOAT flag.

Please replace "flag" with "block layout debug flag".  (including in part 6, which should now be "remove unused block layout debug flags".)

With that, the commit messages will give better context for:
 - what part of the code it's touching. (block layout)
 - whether it's expected to impact behavior or not. ("debug flag" = not expected to impact behavior)
...which are important pieces of information when people are skimming commit messages in regression-range pushlogs.
Comment on attachment 8756752 [details]
MozReview Request: Bug 1275831 - part1: fix NOISY_FLOAT block layout debug flag.

https://reviewboard.mozilla.org/r/55374/#review52232

Ship It!
Attachment #8756752 - Flags: review?(dholbert) → review+
Comment on attachment 8756753 [details]
MozReview Request: Bug 1275831 - part2: fix NOISY_FINAL_SIZE block layout debug flag.

https://reviewboard.mozilla.org/r/55376/#review52234

Ship It!
Attachment #8756753 - Flags: review?(dholbert) → review+
Comment on attachment 8756755 [details]
MozReview Request: Bug 1275831 - part4: fix NOISY_BLOCK_DIR_MARGINS block layout debug flag.

https://reviewboard.mozilla.org/r/55380/#review52238

Ship It!
Attachment #8756755 - Flags: review?(dholbert) → review+
Comment on attachment 8756756 [details]
MozReview Request: Bug 1275831 - part5: fix NOISY_REFLOW_REASON block layout debug flag.

https://reviewboard.mozilla.org/r/55382/#review52240

::: layout/generic/nsIFrame.h:3473
(Diff revision 2)
>      ListTag(t, aFrame);
>      fputs(t.get(), out);
>    }
> +  static void ListTag(FILE* out, const nsFrameList& aFrameList) {
> +    for (nsFrameList::Enumerator e(aFrameList);
> +           !e.AtEnd(); e.Next()) {

Please simplify to use a range-based "for" loop here.

    for (nsIFrame frame : aFrameList) {
      ListTag(out, frame);
    }
Attachment #8756756 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #21)
>     for (nsIFrame frame : aFrameList) {

for (nsIFrame* frame : aFrameList) {

with a "*" :-)
Comment on attachment 8756757 [details]
MozReview Request: Bug 1275831 - part6: remove unused block layout debug flag.

https://reviewboard.mozilla.org/r/55384/#review52260

Nit on extended commit message here:
> 1.Remove NOISY_FIRST_LINE, NOISY_MAX_ELEMENT_SIZE ,NOISY_MAXIMUM_WIDTH.

Please fix the space-before-comma typo-------------^^

Also, historical note: I suspect "REALLY_NOISY_REFLOW_CHILD" *never actually existed* outside of the one chunk that you're adjusting here. That chunk was added in this commit:
 https://github.com/mozilla/gecko-dev/blob/2916ad11f95e65d337db843a9998df2421a8923d/layout/generic/nsBlockFrame.cpp
...without any other usages of REALLY_NOISY_REFLOW_CHILD existing in the file.  So, that chunk might even just want to get removed, as "code that was always dead".  But what you've got here seems fine, too (adjusting it to REALLY_NOISY_REFLOW).
Attachment #8756757 - Flags: review?(dholbert) → review+
Comment on attachment 8756754 [details]
MozReview Request: Bug 1275831 - part3: fix NOISY_COMBINED_AREA block layout debug flag.

https://reviewboard.mozilla.org/r/55378/#review52266

r=me on part 3, with one nit addressed:

::: layout/generic/nsBlockFrame.cpp:1738
(Diff revision 2)
>  #ifdef NOISY_COMBINED_AREA
>    ListTag(stdout);
> -  printf(": ca=%d,%d,%d,%d\n", area.x, area.y, area.width, area.height);
> +  const nsRect& vis = areas.VisualOverflow();
> +  printf(": VisualOverflowArea=%d,%d,%d,%d\n", vis.x, vis.y, vis.width, vis.height);
> +  const nsRect& scr = areas.ScrollableOverflow();
> +  printf(": ScrollableOverflowArea=%d,%d,%d,%d\n", scr.x, scr.y, scr.width, scr.height);

It looks like all of the "NOISY_COMBINED_AREA" debug sections include "ca" (short for "Combined Area") in the printf output.

Elsewhere we have "lineCA=...", and this chunk here has "ca=...", but this patch is removing the "ca" here.

Probably best to preserve that token -- maybe adjust your printf messages to say something like:
  VisualOverflow CA=...
  ScrollableOverflow CA=...

(Note that I've got CA capitalized, for consistency with the usages in the remaining chunks ("lineCA") which makes for better greppability when skimming debug output.)
Attachment #8756754 - Flags: review?(dholbert) → review+
Comment on attachment 8756756 [details]
MozReview Request: Bug 1275831 - part5: fix NOISY_REFLOW_REASON block layout debug flag.

https://reviewboard.mozilla.org/r/55382/#review52268

(r=me on part 5 as well, with a range-based "for" loop, per comment 21 / 22.)
Attachment #8756756 - Flags: review+
https://reviewboard.mozilla.org/r/55378/#review52266

> It looks like all of the "NOISY_COMBINED_AREA" debug sections include "ca" (short for "Combined Area") in the printf output.
> 
> Elsewhere we have "lineCA=...", and this chunk here has "ca=...", but this patch is removing the "ca" here.
> 
> Probably best to preserve that token -- maybe adjust your printf messages to say something like:
>   VisualOverflow CA=...
>   ScrollableOverflow CA=...
> 
> (Note that I've got CA capitalized, for consistency with the usages in the remaining chunks ("lineCA") which makes for better greppability when skimming debug output.)

The idea of consistency sounds thoughtful. Will address it.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #16)
> Maybe we could file a followup to further refactor all these flags in the
> form of what we did for GECKO_BLOCK_DEBUG_FLAGS [1].

Good idea. Please file a follow-up.
(In reply to Daniel Holbert [:dholbert] from comment #17)
> Firstly: thanks for doing this cleanup work!
> 
> One commit message nit, which applies to all parts here:
> 
> > Bug 1275831 - part1: fix NOISY_FLOAT flag.
> 
> Please replace "flag" with "block layout debug flag".  (including in part 6,
> which should now be "remove unused block layout debug flags".)
> 
> With that, the commit messages will give better context for:
>  - what part of the code it's touching. (block layout)
>  - whether it's expected to impact behavior or not. ("debug flag" = not
> expected to impact behavior)
> ...which are important pieces of information when people are skimming commit
> messages in regression-range pushlogs.

Lesson learned. Thank you for the review. :)
Comment on attachment 8756752 [details]
MozReview Request: Bug 1275831 - part1: fix NOISY_FLOAT block layout debug flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55374/diff/2-3/
Attachment #8756752 - Attachment description: MozReview Request: Bug 1275831 - part1: fix NOISY_FLOAT flag. → MozReview Request: Bug 1275831 - part1: fix NOISY_FLOAT block layout debug flag.
Attachment #8756753 - Attachment description: MozReview Request: Bug 1275831 - part2: fix NOISY_FINAL_SIZE flag. → MozReview Request: Bug 1275831 - part2: fix NOISY_FINAL_SIZE block layout debug flag.
Attachment #8756754 - Attachment description: MozReview Request: Bug 1275831 - part3: fix NOISY_COMBINED_AREA flag. → MozReview Request: Bug 1275831 - part3: fix NOISY_COMBINED_AREA block layout debug flag.
Attachment #8756755 - Attachment description: MozReview Request: Bug 1275831 - part4: fix NOISY_BLOCK_DIR_MARGINS flag. → MozReview Request: Bug 1275831 - part4: fix NOISY_BLOCK_DIR_MARGINS block layout debug flag.
Attachment #8756756 - Attachment description: MozReview Request: Bug 1275831 - part5: fix NOISY_REFLOW_REASON flag. → MozReview Request: Bug 1275831 - part5: fix NOISY_REFLOW_REASON block layout debug flag.
Attachment #8756757 - Attachment description: MozReview Request: Bug 1275831 - part6: remove unused flags. → MozReview Request: Bug 1275831 - part6: remove unused block layout debug flag.
Comment on attachment 8756753 [details]
MozReview Request: Bug 1275831 - part2: fix NOISY_FINAL_SIZE block layout debug flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55376/diff/2-3/
Comment on attachment 8756754 [details]
MozReview Request: Bug 1275831 - part3: fix NOISY_COMBINED_AREA block layout debug flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55378/diff/2-3/
Comment on attachment 8756755 [details]
MozReview Request: Bug 1275831 - part4: fix NOISY_BLOCK_DIR_MARGINS block layout debug flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55380/diff/2-3/
Comment on attachment 8756756 [details]
MozReview Request: Bug 1275831 - part5: fix NOISY_REFLOW_REASON block layout debug flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55382/diff/2-3/
Comment on attachment 8756757 [details]
MozReview Request: Bug 1275831 - part6: remove unused block layout debug flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55384/diff/2-3/
Attachment #8756758 - Attachment is obsolete: true
Summary: [DEBUG only] Repair and refactor nsBlockDebugFlags → [DEBUG only] Repair block layout debug flags in nsBlockDebugFlags.h
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #27)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #16)
> > Maybe we could file a followup to further refactor all these flags in the
> > form of what we did for GECKO_BLOCK_DEBUG_FLAGS [1].
> 
> Good idea. Please file a follow-up.

Filed Bug 1276180.
You need to log in before you can comment on or make changes to this bug.