Closed
Bug 1297097
Opened 8 years ago
Closed 8 years ago
Remove preference "layout.css.vertical-text.enabled"
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: TYLin, Assigned: TYLin)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(5 files)
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
Writing mode had been enabled in all channels (Bug 1138384) for over an year. We could remove the preference "layout.css.vertical-text.enabled" from various reftest.list as well as from the code.
http://searchfox.org/mozilla-central/search?q=layout.css.vertical-text.enabled&case=false®exp=false&path=
Assignee | ||
Updated•8 years ago
|
Summary: Remove "layout.css.vertical-text.enabled" → Remove preference "layout.css.vertical-text.enabled"
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8783853 -
Flags: review?(jfkthame)
Attachment #8783854 -
Flags: review?(jfkthame)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → tlin
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8783853 [details]
Bug 1297097 Part 1 - Remove preference "layout.css.vertical-text.enabled" in test files.
https://reviewboard.mozilla.org/r/73514/#review71392
::: layout/style/test/property_database.js:5534
(Diff revision 1)
> - for (var prop in verticalTextProperties) {
> +for (var prop in verticalTextProperties) {
> - gCSSProperties[prop] = verticalTextProperties[prop];
> + gCSSProperties[prop] = verticalTextProperties[prop];
> - }
> +}
With the pref gone, the verticalTextProperties can just be included in the main gCSSProperties list, can't they? No point in creating a separate list that we then have to iterate over, copying them over to gCSSProperties.
I'd suggest adding a third patch to do this code movement.
Attachment #8783853 -
Flags: review?(jfkthame) → review+
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8783854 [details]
Bug 1297097 Part 2 - Remove preference "layout.css.vertical-text.enabled" in nsCSSPropList.h.
https://reviewboard.mozilla.org/r/73516/#review71394
Yes, I think it's been long enough that we can do this.
How about another small patch to remove the pref entry from all.js, too?
Attachment #8783854 -
Flags: review?(jfkthame) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8784011 [details]
Bug 1297097 Part 4 - Move vertical text properties into gCSSProperties.
https://reviewboard.mozilla.org/r/73608/#review71440
The diff looks ugly. What I did is
1) append all the vertical text properties to the end of gCSSProperties, and
2) delete "verticalTextProperties", the loop, and the prerequisites injection for "font" and "line-height".
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8784010 [details]
Bug 1297097 Part 3 - Remove preference "layout.css.vertical-text.enabled" in all.js.
https://reviewboard.mozilla.org/r/73606/#review71442
Attachment #8784010 -
Flags: review?(jfkthame) → review+
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8784011 [details]
Bug 1297097 Part 4 - Move vertical text properties into gCSSProperties.
https://reviewboard.mozilla.org/r/73608/#review71444
Yeah, mozreview really gets confused by this one, doesn't it! Thanks for the comment summarizing the actual change, that's much easier to understand.
I was wondering whether the properties should be put into gCSSProperties in alphabetical order, but it looks like there are already some major out-of-order sections, so leaving these all together at the end of the list seems fine for now.
So r=me, assuming tryserver agrees this is all OK. :)
Attachment #8784011 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Thank you for the review. Try results look good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc9174935547
Comment 13•8 years ago
|
||
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f157ad3f509
Part 1 - Remove preference "layout.css.vertical-text.enabled" in test files. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/29d9fb1f4cec
Part 2 - Remove preference "layout.css.vertical-text.enabled" in nsCSSPropList.h. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/85c47c859eeb
Part 3 - Remove preference "layout.css.vertical-text.enabled" in all.js. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/b54a7a48f417
Part 4 - Move vertical text properties into gCSSProperties. r=jfkthame
Comment 14•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/4412489c0c8e for two things, probably neither one your fault.
https://treeherder.mozilla.org/logviewer.html#?job_id=2509968&repo=autoland probably means that nsCSSPropList.h has bad dependencies and needed a clobber to get rebuilt, so you're stuck having to figure out whether that's bug 1276197 or something different, and reland touching CLOBBER.
https://treeherder.mozilla.org/logviewer.html#?job_id=2509186&repo=autoland apparently means that somehow your patch caused those tests to be just faster enough (or, I suppose, though that seems less likely, just slower enough) to frequently fall into the bug 1223198 trap. If it were me, I'd push to try running all Linux32 and Linux64 reftests, retrigger them all ten or twenty times, and then disable those and any other that you touched that start to fail on Linux in a separate leave-open bug depending on bug 1223198.
Assignee | ||
Comment 15•8 years ago
|
||
Another try result with CLOBBER touched.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f8a8e032b69
Again, I got a intermittent Linux Debug tc-R(R5) failed once like the second link in comment 14, but this time the diff result is different.
https://treeherder.mozilla.org/logviewer.html#?job_id=26497215&repo=try#L9153
Comment 16•8 years ago
|
||
Adding dev-doc-needed to double-check, when this lands, that we don't have any mention of this pref on MDN.
Keywords: dev-doc-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•8 years ago
|
||
Push a new try today, and the tc-R5 fail intermittent doesn't happen now ...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f02669f0bd53
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8792200 [details]
Bug 1297097 Part 5 - Touch CLOBBER to work around bug 1276197.
https://reviewboard.mozilla.org/r/79392/#review77920
OK, let's hope the landing sticks this time!
Attachment #8792200 -
Flags: review?(jfkthame) → review+
Comment 24•8 years ago
|
||
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ba76f0f5a66
Part 1 - Remove preference "layout.css.vertical-text.enabled" in test files. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/830b578a5dd9
Part 2 - Remove preference "layout.css.vertical-text.enabled" in nsCSSPropList.h. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/d5012adf0955
Part 3 - Remove preference "layout.css.vertical-text.enabled" in all.js. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/7d8267e9f27d
Part 4 - Move vertical text properties into gCSSProperties. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/0c9409351075
Part 5 - Touch CLOBBER to work around bug 1276197. r=jfkthame
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8ba76f0f5a66
https://hg.mozilla.org/mozilla-central/rev/830b578a5dd9
https://hg.mozilla.org/mozilla-central/rev/d5012adf0955
https://hg.mozilla.org/mozilla-central/rev/7d8267e9f27d
https://hg.mozilla.org/mozilla-central/rev/0c9409351075
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 26•8 years ago
|
||
Updated the note below the compat table in:
https://developer.mozilla.org/en-US/docs/Web/CSS/block-size
https://developer.mozilla.org/en-US/docs/Web/CSS/max-block-size
https://developer.mozilla.org/en-US/docs/Web/CSS/min-block-size
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-end
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-end-color
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-end-style
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-end-width
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-start
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-start-color
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-start-style
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-start-width
https://developer.mozilla.org/en-US/docs/Web/CSS/inline-size
https://developer.mozilla.org/en-US/docs/Web/CSS/max-inline-size
https://developer.mozilla.org/en-US/docs/Web/CSS/min-inline-size
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-block-end
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-block-start
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-end
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-start
https://developer.mozilla.org/en-US/docs/Web/CSS/offset-block-end
https://developer.mozilla.org/en-US/docs/Web/CSS/offset-block-start
https://developer.mozilla.org/en-US/docs/Web/CSS/offset-inline-end
https://developer.mozilla.org/en-US/docs/Web/CSS/offset-inline-start
https://developer.mozilla.org/en-US/docs/Web/CSS/padding-block-end
https://developer.mozilla.org/en-US/docs/Web/CSS/padding-block-start
https://developer.mozilla.org/en-US/docs/Web/CSS/padding-inline-end
https://developer.mozilla.org/en-US/docs/Web/CSS/padding-inline-start
https://developer.mozilla.org/en-US/docs/Web/CSS/writing-mode
https://developer.mozilla.org/en-US/docs/Web/CSS/text-orientation
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•