Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the unicode-bidi CSS property #6721

Merged
merged 1 commit into from Aug 3, 2015
Merged

Implement the unicode-bidi CSS property #6721

merged 1 commit into from Aug 3, 2015

Conversation

@mbrubeck
Copy link
Contributor

mbrubeck commented Jul 24, 2015

r? @pcwalton

Review on Reviewable
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jul 24, 2015

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

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.
components/gfx/text/glyph.rs Outdated
@@ -782,17 +784,23 @@ impl<'a> Iterator for GlyphIterator<'a> {
self.next_glyph_range()

This comment has been minimized.

Sign in to view
@pcwalton

pcwalton Jul 24, 2015

Contributor

nit: While you're here, could you change this into an early return? Then you can de-indent all your code below.
components/gfx/text/glyph.rs Outdated
if !self.char_range.contains(i) {
return None
}
assert!(i < self.store.char_len());

This comment has been minimized.

Sign in to view
@pcwalton

pcwalton Jul 24, 2015

Contributor

nit: debug_assert! — I bet this code is performance critical.
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jul 24, 2015

Review comments addressed. r? @pcwalton
@metajack
Copy link
Contributor

metajack commented Jul 29, 2015

pinging @pcwalton
@pcwalton
Copy link
Contributor

pcwalton commented Aug 3, 2015

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2015

📌 Commit 2ecce57 has been approved by pcwalton
bors-servo pushed a commit that referenced this pull request Aug 3, 2015
bors-servo
Auto merge of #6721 - mbrubeck:unicode-bidi, r=pcwalton
4bd4283 
Implement the unicode-bidi CSS property

r? @pcwalton

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6721)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2015

⌛ Testing commit 2ecce57 with merge 4bd4283...
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Aug 3, 2015

@bors-servo r-
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2015

💔 Test failed - linux3
@mbrubeck mbrubeck force-pushed the mbrubeck:unicode-bidi branch from 2ecce57 to 74fb222 Aug 3, 2015
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Aug 3, 2015

@bors-servo r=pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2015

📌 Commit 74fb222 has been approved by pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2015

⌛ Testing commit 74fb222 with merge ea96fd1...
bors-servo pushed a commit that referenced this pull request Aug 3, 2015
bors-servo
Auto merge of #6721 - mbrubeck:unicode-bidi, r=pcwalton
ea96fd1 
Implement the unicode-bidi CSS property

r? @pcwalton

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6721)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2015

💔 Test failed - mac3
@jdm
Copy link
Member

jdm commented Aug 3, 2015 



/css21_dev/html4/inline-formatting-context-007.htm
--------------------------------------------------
FAIL [Parent]

Can you take a look and see if anything in that test should be affected by this?
@jdm jdm added S-tests-failed and removed S-awaiting-merge labels Aug 3, 2015
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Aug 3, 2015

Also:

/html/semantics/text-level-semantics/the-bdi-element/bdi-neutral-nested.html
----------------------------------------------------------------------------
PASS expected FAIL [Parent]

Both of these unexpected results are Mac-only. (The Linux builders are green.)

  • inline-formatting-context-007.htm was previously failing on both platforms, now failing on Mac only.
  • bdi-neutral-nested.html was previously failing on both platforms, now failing on Linux only.

Looking at the reftest analyzer, even on the failing platforms, the rendering with the patch is an improvement over the rendering before. The remaining failures appear to be unrelated bugs. For example inline-formatting-context-007.htm has extra top/bottom borders, which are also the cause of the previously-existing failures in inline-formatting-context-004.htm and inline-formatting-context-005.htm.

I'll retry this with adjusted text expectations.
@mbrubeck
Implement the unicode-bidi property
Loading status checks…
f8e92b2
@mbrubeck mbrubeck force-pushed the mbrubeck:unicode-bidi branch from 74fb222 to f8e92b2 Aug 3, 2015
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Aug 3, 2015

@bors-servo r=pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2015

📌 Commit f8e92b2 has been approved by pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2015

⌛ Testing commit f8e92b2 with merge 5922ac6...
bors-servo pushed a commit that referenced this pull request Aug 3, 2015
bors-servo
Auto merge of #6721 - mbrubeck:unicode-bidi, r=pcwalton
Loading status checks…
5922ac6 
Implement the unicode-bidi CSS property

r? @pcwalton

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6721)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2015

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3
@bors-servo bors-servo merged commit f8e92b2 into servo:master Aug 3, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
This was referenced Mar 21, 2016
Closed
Closed
@mbrubeck mbrubeck deleted the mbrubeck:unicode-bidi branch May 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pcwalton pcwalton

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@mbrubeck @hoppipolla-critic-bot @metajack @pcwalton @bors-servo @jdm @highfive
You can’t perform that action at this time.