Closed Bug 1313083 Opened 5 years ago Closed 5 years ago

Repair line layout debug flags in nsLineLayout

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: chenpighead, Assigned: chenpighead)

Details

Attachments

(3 files)

While hacking around layout codes, I'd turn on debugging flags in nsLineLayout [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] http://searchfox.org/mozilla-central/rev/8cf1367dd89cc36ef8f025dfc6af6d5c086838a7/layout/generic/nsLineLayout.cpp#33-47
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] http://searchfox.org/mozilla-central/rev/8cf1367dd89cc36ef8f025dfc6af6d5c086838a7/layout/generic/nsBlockFrame.cpp#130-217
Summary: Repair debugging flags in nsLineLayout → Repair line layout debug flags in nsLineLayout
Comment on attachment 8804763 [details]
Bug 1313083 - Fix NOISY_BLOCKDIR_ALIGN line layout debug flag.

https://reviewboard.mozilla.org/r/88652/#review87710

Hi Ting-Yu, could you help review these patches? Please let me know if you're not comfortable with reviewing them, I could forward the requests to other candidates then.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #6)
> try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=051b1d3d78e4

Looks like huge amount of logs were printed, which causes either test timeout or log file size exceeding limits. Since all the build test are green, we shall be fine I think.
Comment on attachment 8804763 [details]
Bug 1313083 - Fix NOISY_BLOCKDIR_ALIGN line layout debug flag.

https://reviewboard.mozilla.org/r/88654/#review87992
Attachment #8804763 - Flags: review?(tlin) → review+
Comment on attachment 8804764 [details]
Bug 1313083 - Fix REALLY_NOISY_REFLOW line layout debug flag.

https://reviewboard.mozilla.org/r/88656/#review87994
Attachment #8804764 - Flags: review?(tlin) → review+
Comment on attachment 8804765 [details]
Bug 1313083 - Remove unused line layout debug flags.

https://reviewboard.mozilla.org/r/88658/#review87996

These flags do not appear to be used in nsLineLayout.cpp. Nice.
Attachment #8804765 - Flags: review?(tlin) → review+
> Hi Ting-Yu, could you help review these patches? Please let me know if
> you're not comfortable with reviewing them, I could forward the requests to
> other candidates then.

I'm happy to review these, but I'm not officially a layout peer. There patch fixed only build errors when certain debug flags are defined, so should be relative safe for me to review.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #11)
> > Hi Ting-Yu, could you help review these patches? Please let me know if
> > you're not comfortable with reviewing them, I could forward the requests to
> > other candidates then.
> 
> I'm happy to review these, but I'm not officially a layout peer. There patch
> fixed only build errors when certain debug flags are defined, so should be
> relative safe for me to review.

Yeah, I think so too. Thank you for the review. :-)
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e29de31acba0
Fix NOISY_BLOCKDIR_ALIGN line layout debug flag. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/c4b900415af6
Fix REALLY_NOISY_REFLOW line layout debug flag. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/9b93581a04ef
Remove unused line layout debug flags. r=TYLin
https://hg.mozilla.org/mozilla-central/rev/e29de31acba0
https://hg.mozilla.org/mozilla-central/rev/c4b900415af6
https://hg.mozilla.org/mozilla-central/rev/9b93581a04ef
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.