Basic support for bidirectional text #6471

mbrubeck · 2015-06-26T15:43:09Z

This re-orders text according to the Unicode bidirectional layout algorithm, using the unicode-bidi crate. It uses the natural order of the text based on Unicode character properties and the CSS direction property.

This does not yet support the CSS unicode-bidi property or the HTML dir attribute, but these should be straightforward to add.

r? @pcwalton. Also depends on servo/unicode-bidi#4.

hoppipolla-critic-bot · 2015-06-26T15:43:15Z

Critic review: https://critic.hoppipolla.co.uk/r/5384

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

pcwalton · 2015-07-06T18:13:11Z

components/layout/inline.rs

@@ -272,6 +310,7 @@ impl LineBreaker {
                              flow: &'a InlineFlow,
                              layout_context: &LayoutContext)
                              where I: Iterator<Item=Fragment> {
+


nit: remove this newline

pcwalton · 2015-07-06T18:13:32Z

components/layout/inline.rs

@@ -612,7 +651,7 @@ impl LineBreaker {
                             line_flush_mode: LineFlushMode) {
        let indentation = self.indentation_for_pending_fragment();
        if self.pending_line_is_empty() {
-            assert!(self.new_fragments.len() <= (u16::MAX as usize));
+            assert!(self.new_fragments.len() <= (isize::MAX as usize));


Can you change this to debug_assert! while you're here?

pcwalton · 2015-07-06T18:15:08Z

components/layout/inline.rs

+                (range.end().get() - 1, range.begin().get() - 1, -1)
+            } else {
+                (range.begin().get(), range.end().get(), 1)
+            };


I wonder if you can just create the iterator inside the then and else branches instead of creating all these temporaries.

pcwalton · 2015-07-06T18:15:47Z

components/layout/text.rs

 use util::geometry::Au;
 use util::linked_list::split_off_head;
 use util::logical_geometry::{LogicalSize, WritingMode};
 use util::range::{Range, RangeIndex};

+/// Returns the concatened text of a list of unscanned text fragments.


nit: typo: "concatenated"

pcwalton · 2015-07-06T18:23:31Z

components/layout/inline.rs

@@ -350,7 +389,7 @@ impl LineBreaker {
    fn flush_current_line(&mut self) {
        debug!("LineBreaker: flushing line {}: {:?}", self.lines.len(), self.pending_line);
        self.strip_trailing_whitespace_from_pending_line_if_necessary();
-        self.lines.push(self.pending_line);
+        self.lines.push(self.pending_line.clone());


I'm a bit worried about the performance implications of doing this, now that there's a vec (possibly) inside the line. It'd probably be better to have reset_line() swap out the current line and return the old one. What do you think?

pcwalton · 2015-07-06T18:28:18Z

components/util/logical_geometry.rs

+
+    #[inline]
+    pub fn to_bidi_level(&self) -> u8 {
+        if self.is_bidi_ltr() { 0 } else { 1 }


Could probably be (!self.is_bidi_ltr()) as u8, which may codegen better. (LLVM is not very good at simplifying branches.)

pcwalton · 2015-07-06T18:28:51Z

Looks good to me once these issues are addressed.

mbrubeck · 2015-07-06T20:59:23Z

Review status: 0 of 18 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

^{components/layout/inline.rs, line 313 [r6] (raw file):}
Done.

^{components/layout/inline.rs, line 392 [r6] (raw file):}
Done.

^{components/layout/inline.rs, line 654 [r6] (raw file):}
Done.

^{components/layout/inline.rs, line 979 [r6] (raw file):}
Done.

^{components/layout/text.rs, line 32 [r6] (raw file):}
Done.

^{components/util/logical_geometry.rs, line 91 [r6] (raw file):}
Done.

Comments from the review on Reviewable.io

mbrubeck · 2015-07-06T21:22:31Z

Squashed and rebased, with review comments addressed. Now just waiting on servo/unicode-bidi#4.

SimonSapin · 2015-07-13T01:27:09Z

I found some small issues in the unicode-bidi crate, but nothing that should block this.

@bors-servo r=pcwalton

bors-servo · 2015-07-13T01:27:09Z

📌 Commit 83c41d2 has been approved by pcwalton

bors-servo · 2015-07-13T01:27:12Z

⌛ Testing commit 83c41d2 with merge 391948c...


        Auto merge of #6471 - mbrubeck:bidi, r=pcwalton

@pcwalton

Basic support for bidirectional text This re-orders text according to the Unicode bidirectional layout algorithm, using the [unicode-bidi](https://github.com/mbrubeck/unicode-bidi) crate. It uses the natural order of the text based on Unicode character properties and the CSS `direction` property. This does not yet support the CSS `unicode-bidi` property or the HTML `dir` attribute, but these should be straightforward to add. r? @pcwalton. Also depends on servo/unicode-bidi#4.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6471)

bors-servo · 2015-07-13T01:58:10Z

💔 Test failed - mac1

jdm · 2015-07-13T02:19:31Z

/html/semantics/forms/the-textarea-element/textarea-newline-bidi.html
---------------------------------------------------------------------
FAIL [Parent]
/html/semantics/grouping-content/the-pre-element/pre-newline-bidi.html
----------------------------------------------------------------------
FAIL [Parent]
/html/semantics/text-level-semantics/the-bdo-element/bdo-override.html
----------------------------------------------------------------------
FAIL [Parent]
/html/semantics/text-level-semantics/the-br-element/br-bidi.html
----------------------------------------------------------------
FAIL [Parent]

bors-servo · 2015-07-14T09:48:18Z

☔ The latest upstream changes (presumably #6619) made this pull request unmergeable. Please resolve the merge conflicts.

mbrubeck · 2015-07-19T17:33:59Z

The test failures were caused by a failure to force paragraph breaks in preformatted text. This patch fixes the problem. Depends on servo/unicode-bidi#12. r? @pcwalton

This could use some further optimization (see the FIXME comment), but I want to wait a bit to work out how this fits in with other changes to fragmentation and text run construction.

bors-servo · 2015-07-20T21:27:39Z

☔ The latest upstream changes (presumably #6648) made this pull request unmergeable. Please resolve the merge conflicts.

pcwalton · 2015-07-24T00:16:17Z

This looks good. I just had one question.

mbrubeck · 2015-07-24T00:39:51Z

@bors-servo r=pcwalton

bors-servo · 2015-07-24T00:39:51Z

📌 Commit 70dbf03 has been approved by pcwalton

bors-servo · 2015-07-24T00:39:53Z

⌛ Testing commit 70dbf03 with merge 51930c8...


        Auto merge of #6471 - mbrubeck:bidi, r=pcwalton

@pcwalton

Basic support for bidirectional text This re-orders text according to the Unicode bidirectional layout algorithm, using the [unicode-bidi](https://github.com/mbrubeck/unicode-bidi) crate. It uses the natural order of the text based on Unicode character properties and the CSS `direction` property. This does not yet support the CSS `unicode-bidi` property or the HTML `dir` attribute, but these should be straightforward to add. r? @pcwalton. Also depends on servo/unicode-bidi#4.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6471)

bors-servo · 2015-07-24T01:09:41Z

💔 Test failed - mac3


        Basic support for bidirectional text

mbrubeck · 2015-07-24T03:07:23Z

@bors-servo r=pcwalton

bors-servo · 2015-07-24T03:07:23Z

📌 Commit dfac8ce has been approved by pcwalton

bors-servo · 2015-07-24T03:20:50Z

⌛ Testing commit dfac8ce with merge d3a36fa...


        Auto merge of #6471 - mbrubeck:bidi, r=pcwalton

@pcwalton

Basic support for bidirectional text This re-orders text according to the Unicode bidirectional layout algorithm, using the [unicode-bidi](https://github.com/mbrubeck/unicode-bidi) crate. It uses the natural order of the text based on Unicode character properties and the CSS `direction` property. This does not yet support the CSS `unicode-bidi` property or the HTML `dir` attribute, but these should be straightforward to add. r? @pcwalton. Also depends on servo/unicode-bidi#4.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6471)

bors-servo · 2015-07-24T04:01:34Z

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

highfive added the S-awaiting-review label Jun 26, 2015

metajack assigned pcwalton Jun 27, 2015

mbrubeck added I-enhancement A-layout/text labels Jun 27, 2015

pcwalton reviewed Jul 6, 2015
View changes

components/layout/inline.rs Outdated

@@ -272,6 +310,7 @@ impl LineBreaker {

flow: &'a InlineFlow,

layout_context: &LayoutContext)

where I: Iterator<Item=Fragment> {

This comment has been minimized.

Sign in to view

pcwalton Jul 6, 2015
Contributor

nit: remove this newline

pcwalton reviewed Jul 6, 2015
View changes

mbrubeck force-pushed the mbrubeck:bidi branch 3 times, most recently from 61caa3f to 83c41d2 Jul 6, 2015

mbrubeck added S-blocked-on-external and removed S-awaiting-review labels Jul 6, 2015

jdm added S-tests-failed and removed S-blocked-on-external labels Jul 13, 2015

mbrubeck force-pushed the mbrubeck:bidi branch from 83c41d2 to fd9598d Jul 19, 2015

highfive removed the S-tests-failed label Jul 19, 2015

mbrubeck added the S-needs-squash label Jul 19, 2015

mbrubeck force-pushed the mbrubeck:bidi branch from bd84a87 to 6a43283 Jul 23, 2015

mbrubeck removed the S-blocked-on-external label Jul 23, 2015

mbrubeck removed the S-awaiting-review label Jul 24, 2015

mbrubeck force-pushed the mbrubeck:bidi branch from 6a43283 to 70dbf03 Jul 24, 2015

highfive added the S-awaiting-review label Jul 24, 2015

mbrubeck added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Jul 24, 2015

Basic support for bidirectional text

Loading status checks…

dfac8ce

mbrubeck force-pushed the mbrubeck:bidi branch from 70dbf03 to dfac8ce Jul 24, 2015

highfive added S-awaiting-review and removed S-awaiting-merge labels Jul 24, 2015

mbrubeck added S-awaiting-merge and removed S-awaiting-review labels Jul 24, 2015

bors-servo merged commit dfac8ce into servo:master Jul 24, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

mbrubeck deleted the mbrubeck:bidi branch May 11, 2016

servo / servo

Basic support for bidirectional text #6471

Basic support for bidirectional text #6471

mbrubeck commented Jun 26, 2015

hoppipolla-critic-bot commented Jun 26, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pcwalton commented Jul 6, 2015

mbrubeck commented Jul 6, 2015

mbrubeck commented Jul 6, 2015

SimonSapin commented Jul 13, 2015

bors-servo commented Jul 13, 2015

bors-servo commented Jul 13, 2015

bors-servo commented Jul 13, 2015

jdm commented Jul 13, 2015

bors-servo commented Jul 14, 2015

mbrubeck commented Jul 19, 2015

bors-servo commented Jul 20, 2015

pcwalton commented Jul 24, 2015

mbrubeck commented Jul 24, 2015

bors-servo commented Jul 24, 2015

bors-servo commented Jul 24, 2015

bors-servo commented Jul 24, 2015

mbrubeck commented Jul 24, 2015

bors-servo commented Jul 24, 2015

bors-servo commented Jul 24, 2015

bors-servo commented Jul 24, 2015

servo / servo

Join GitHub today

Basic support for bidirectional text #6471

Basic support for bidirectional text #6471

Conversation

mbrubeck commented Jun 26, 2015

hoppipolla-critic-bot commented Jun 26, 2015

This comment has been minimized.

pcwalton Jul 6, 2015

This comment has been minimized.

pcwalton Jul 6, 2015

This comment has been minimized.

pcwalton Jul 6, 2015

This comment has been minimized.

pcwalton Jul 6, 2015

This comment has been minimized.

pcwalton Jul 6, 2015

This comment has been minimized.

pcwalton Jul 6, 2015

pcwalton commented Jul 6, 2015

mbrubeck commented Jul 6, 2015

mbrubeck commented Jul 6, 2015

SimonSapin commented Jul 13, 2015

bors-servo commented Jul 13, 2015

bors-servo commented Jul 13, 2015

bors-servo commented Jul 13, 2015

jdm commented Jul 13, 2015

bors-servo commented Jul 14, 2015

mbrubeck commented Jul 19, 2015

bors-servo commented Jul 20, 2015

pcwalton commented Jul 24, 2015

mbrubeck commented Jul 24, 2015

bors-servo commented Jul 24, 2015

bors-servo commented Jul 24, 2015

bors-servo commented Jul 24, 2015

mbrubeck commented Jul 24, 2015

bors-servo commented Jul 24, 2015

bors-servo commented Jul 24, 2015

bors-servo commented Jul 24, 2015