Closed Bug 1297097 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

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)

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=
Summary: Remove "layout.css.vertical-text.enabled" → Remove preference "layout.css.vertical-text.enabled"
Attachment #8783853 - Flags: review?(jfkthame)
Attachment #8783854 - Flags: review?(jfkthame)
Assignee: nobody → tlin
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 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 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 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 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+
Thank you for the review. Try results look good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc9174935547
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.
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
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 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+
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
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
You need to log in before you can comment on or make changes to this bug.