Closed Bug 1097128 Opened 10 years ago Closed 10 years ago

turn on the compile-time switch for vertical writing-mode support

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We have both a compile-time switch in WritingModes.h that completely disables support for vertical writing modes, and a runtime pref that determines whether the CSS writing-mode, text-orientation, and text-combine-upright properties are actually recognized.

At this point, I think we should consider landing the patch to enable the compile-time switch on Nightly (we might want to keep it disabled on release for now). As long as the runtime vertical-text pref remains off, this should have no impact on behavior, but it becomes possible to start testing vertical modes in standard nightly builds by just toggling a pref.

This will mean that key low-level mozilla::WritingMode methods such as IsVertical() will actually be testing something, rather than returning a hard-coded value, and so we'll need to watch for any performance impact; the compiler will no longer be able to optimize away various tests, conversion codepaths, etc., throughout Layout.

I've tried pushing such a patch to tryserver recently, and looking at the Tp numbers;[1] I couldn't see any consistent difference compared to a standard mozilla-central build, but we still need to be alert for any significant regression.

[1] See https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ebbc87d99221. That push also enabled the runtime vertical-text pref, which currently results in the unit test oranges seen there; the compile-time switch alone will not cause this.
WDYT - are we far enough along that it's worth throwing the switch here, both to check for any perf impact and to make it easier for people to start experimenting?
Attachment #8520736 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Oops, sorry for the noise - I forgot to make it conditional on #ifndef RELEASE_BUILD. For now.
Attachment #8520738 - Flags: review?(smontagu)
Attachment #8520736 - Attachment is obsolete: true
Attachment #8520736 - Flags: review?(smontagu)
Comment on attachment 8520738 [details] [diff] [review]
Enable compile-time support for vertical writing modes.

Review of attachment 8520738 [details] [diff] [review]:
-----------------------------------------------------------------

It is time.
Attachment #8520738 - Flags: review?(smontagu) → review+
OK, let's see how this goes... I'm hopeful this will stick without problems, but we do need to be alert for possible regressions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c720eea5b1
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/c9c720eea5b1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.