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

stylo: Implement gecko glue for font-language-override. #16155

Merged
merged 1 commit into from Mar 28, 2017
Merged

stylo: Implement gecko glue for font-language-override. #16155

merged 1 commit into from Mar 28, 2017

Conversation

@chenpighead
Copy link
Contributor

chenpighead commented Mar 27, 2017 •
edited by larsbergstrom

To be aligned with the implementation from Gecko side, we parse
font-language-override as Normal keyword or String. Then, we compute
and store it as a u32. So, as to the stylo glue, we can just pass the
u32 to Gecko.

The extra crate, byteorder, is used to simplify the computing and
serialization.

Since we now implement font-language-override for Gecko, we can remove
the additional branches for font-language-override in font shorthand.

ref: Gecko Bug 1347821

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Mar 27, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/Cargo.toml, components/style/lib.rs, components/style/properties/gecko.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/shorthand/font.mako.rs
  • @emilio: components/style/Cargo.toml, components/style/lib.rs, components/style/properties/gecko.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/shorthand/font.mako.rs
@highfive
Copy link

highfive commented Mar 27, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style code, but no tests are modified. Please consider adding a test!
@chenpighead
Copy link
Contributor Author

chenpighead commented Mar 27, 2017

r? @emilio
@highfive highfive assigned emilio and unassigned glennw Mar 27, 2017
@emilio
emilio approved these changes Mar 27, 2017
View changes
Copy link
Member

emilio left a comment

Looks good! r=me with those final nits addressed.
components/style/properties/longhand/font.mako.rs Outdated
match *self {
SpecifiedValue::Normal => computed_value::T(0),
SpecifiedValue::Override(ref lang) => {
if lang.len() == 0 || lang.len() > 4 || !lang.is_ascii() {

This comment has been minimized.

Sign in to view
@emilio

emilio Mar 27, 2017

Member

let's use lang.is_empty() here instead of len() == 0.
components/style/properties/longhand/font.mako.rs Outdated
let slice = if cfg!(debug_assertions) {
str::from_utf8(&buf).unwrap()
} else {
unsafe { str::from_utf8_unchecked(&buf) }

This comment has been minimized.

Sign in to view
@emilio

emilio Mar 27, 2017

Member

Let's add a comment here with something like "Safe because we ensure it's ASCII during parsing", or something like that.

This comment has been minimized.

Sign in to view
@chenpighead

chenpighead Mar 28, 2017

Author Contributor

Okay, I'm adding a comment as follows: "Safe because we ensure it's ASCII during computing"
components/style/properties/longhand/font.mako.rs Outdated
}
let mut buf = [0; 4];
BigEndian::write_u32(&mut buf, computed.0);
SpecifiedValue::Override(String::from_utf8(buf.to_vec()).unwrap())

This comment has been minimized.

Sign in to view
@emilio

emilio Mar 27, 2017

Member

This can also use if cfg!(debug_assertions) { String::from_utf8(..) } else { String::from_utf8_unchecked } if you want, but no big deal.

This comment has been minimized.

Sign in to view
@chenpighead

chenpighead Mar 28, 2017

Author Contributor

Will do. Thank you.
@chenpighead chenpighead force-pushed the chenpighead:gecko-glue-for-font-language-override branch from 383abf7 to 6c4b15f Mar 28, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2017

📌 Commit 6c4b15f has been approved by BorisChiou
@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2017

💡 This pull request was already approved, no need to approve it again.
@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2017

📌 Commit 6c4b15f has been approved by BorisChiou
@chenpighead
Copy link
Contributor Author

chenpighead commented Mar 28, 2017 •
edited

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

bors-servo commented Mar 28, 2017

@chenpighead: 🔑 Insufficient privileges: Not in reviewers
@BorisChiou
Copy link
Contributor

BorisChiou commented Mar 28, 2017

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

bors-servo commented Mar 28, 2017

💡 This pull request was already approved, no need to approve it again.
@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2017

📌 Commit 6c4b15f has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2017

@chenpighead: 🔑 Insufficient privileges: Not in reviewers
@chenpighead chenpighead force-pushed the chenpighead:gecko-glue-for-font-language-override branch from 6c4b15f to 520131e Mar 28, 2017
@chenpighead chenpighead force-pushed the chenpighead:gecko-glue-for-font-language-override branch 2 times, most recently from 57f52fb to 4304d2e Mar 28, 2017
@chenpighead
stylo: Implement gecko glue for font-language-override.
Loading status checks…
a9e4b9c 
To be aligned with the implementation from Gecko side, we parse
font-language-override as Normal keyword or String. Then, we compute
and store it as a u32. So, as to the stylo glue, we can just pass the
u32 to Gecko.

The extra crate, byteorder, is used to simplify the computing and
serialization.

Since we now implement font-language-override for Gecko, we can remove
the additional branches for font-language-override in font shorthand.

ref: Gecko Bug 1347821
@chenpighead chenpighead force-pushed the chenpighead:gecko-glue-for-font-language-override branch from 4304d2e to a9e4b9c Mar 28, 2017
@wafflespeanut
Copy link
Member

wafflespeanut commented Mar 28, 2017

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

bors-servo commented Mar 28, 2017

📌 Commit a9e4b9c has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2017

⌛ Testing commit a9e4b9c with merge af243d5...
bors-servo added a commit that referenced this pull request Mar 28, 2017
@bors-servo
Auto merge of #16155 - chenpighead:gecko-glue-for-font-language-overr…
Loading status checks…
af243d5 
…ide, r=emilio

stylo: Implement gecko glue for font-language-override.

To be aligned with the implementation from Gecko side, we parse
font-language-override as Normal keyword or String. Then, we compute
and store it as a u32. So, as to the stylo glue, we can just pass the
u32 to Gecko.

The extra crate, byteorder, is used to simplify the computing and
serialization.

Since we now implement font-language-override for Gecko, we can remove
the additional branches for font-language-override in font shorthand.

ref: Gecko [Bug 1347821](https://bugzilla.mozilla.org/show_bug.cgi?id=1347821)

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16155)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: emilio
Pushing af243d5 to master...
@bors-servo bors-servo merged commit a9e4b9c into servo:master Mar 28, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@chenpighead @highfive @bors-servo @BorisChiou @wafflespeanut @emilio @glennw
You can’t perform that action at this time.