Remove preference "layout.css.vertical-text.enabled"

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla51
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

a year ago
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&regexp=false&path=
(Assignee)

Updated

a year 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

a year ago
Attachment #8783853 - Flags: review?(jfkthame)
Attachment #8783854 - Flags: review?(jfkthame)
(Assignee)

Updated

a year ago
Assignee: nobody → tlin

Comment 3

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
Thank you for the review. Try results look good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc9174935547

Comment 13

a year 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
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

a year 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
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

a year 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

a year 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

a year 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

a year 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
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

a year ago
Depends on: 1166147
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.