Repair line layout debug flags in nsLineLayout

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments)

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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
mozreview-review
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 8

2 years ago
mozreview-review
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 9

2 years ago
mozreview-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 10

2 years ago
mozreview-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. :-)

Comment 13

2 years ago
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

Comment 14

2 years ago
bugherder
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
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.