Closed Bug 1347821 Opened 7 years ago Closed 7 years ago

stylo: Implement gecko glue for font-language-override

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: xidorn, Assigned: chenpighead)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1347821 - update stylo mochitest expections for font-language-override support.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
I think we should do bug 1347819 first, then the rest of the work in this bug can be done purely in Servo side.
Priority: -- → P2
Since bug 1347819 has been landed, I'd like to go ahead and fix this bug as well.

IIUC, I shall just

1. fix the computed value in servo side [1]
2. mark the products="gecko"
3. add glue code in [2]


[1] http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/servo/components/style/properties/longhand/font.mako.rs#865-915
[2] http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/servo/components/style/properties/gecko.mako.rs#1163-1286
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Priority: P2 → P1
Thanks Jeremy!
In this patch, I try to add an extern byteorder crate for style package, so we could do the serialization/computing much easier. I patched both "components/style/lib.rs" and "components/style/Cargo.toml", then run "cargo update". It worked fine in servo repo. However, I tried to do the same thing under m-c's servo folder, then I met the following build error while building stylo:

--
 0:24.46 error: no matching package named `style` found (required by `geckoservo`)
 0:24.46 location searched: file:///Users/jeremy/gecko-dev/servo/components/style
 0:24.46 version required: = 0.0.1
 0:24.46 versions found: 0.0.2
 0:24.46 consider running `cargo update` to update a path dependency's locked version
--

I wonder, do I need to manually update geckoservo package by myself? Or, is there any magic trick to do cargo update for stylo build?

Also, if I don't add anything and run "cargo update" under m-c's servo folder, a lot of packages are updated (whereas, running "cargo update" in pure servo repo does nothing since they are up-to-date).

Another thing I could try would be, not to use byteorder crate, but use the build-in rust support to do the serialization/computing... 


Hi xidorn, could you take a look and give me some feedback?
Attachment #8851205 - Flags: feedback?(xidorn+moz)
Why do you bump the version number of style crate to 0.0.2? You probably should not be doing that.

To add new dependency to style crate, you should run
> ./mach vendor rust
after you change Cargo.toml file.

Since byteorder is already a dependency for many other crates, I don't expect there to be any change under third_party/rust, so take care if you see any change there.
Comment on attachment 8851205 [details] [diff] [review]
(wip) stylo: Implement gecko glue for font-language-override

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

::: components/style/properties/longhand/font.mako.rs
@@ +880,5 @@
> +    impl ToCss for SpecifiedValue {
> +        fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> +            match *self {
> +                Either::First(SpecifiedValue::Normal) => dest.write_str("normal"),
> +                Either::Second(SpecifiedValue::Override(x)) => dest.write_str(x),

This is wrong. You need to use `serialize_string` from cssparser crate. There should be some other uses of this function in Servo.

@@ +898,5 @@
> +                    _ => {
> +                        let a : u32 = self.0;
> +                        let mut buf = [0; 4];
> +                        BigEndian::write_u32(&mut buf, a);
> +                        write!(dest, "\"{}\"", String::from_utf8(buf.to_vec()).unwrap());

Here as well, you need `serialize_string`.

Also, you need to strip trailing whitespaces.

@@ +936,5 @@
> +                    } else {
> +                        // store string tag into u32
> +                        let s = String::from(lang);
> +                        let mut bytes = s.into_bytes();
> +                        computed_value::T(BigEndian::read_u32(&bytes))

You need to append whitespaces if it isn't exactly four chars. Also you need to check whether all characters are ASCII.
Attachment #8851205 - Flags: feedback?(xidorn+moz) → feedback-
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> Why do you bump the version number of style crate to 0.0.2? You probably
> should not be doing that.
> 
> To add new dependency to style crate, you should run
> > ./mach vendor rust
> after you change Cargo.toml file.
> 
> Since byteorder is already a dependency for many other crates, I don't
> expect there to be any change under third_party/rust, so take care if you
> see any change there.

Thanks for the pointer. However, I met the issue (Bug 1348475) while running
> ./mach vendor rust
which wipes out the entire third_party/rust and prevent me from building stylo.... :/
Comment on attachment 8851205 [details] [diff] [review]
(wip) stylo: Implement gecko glue for font-language-override

Update the wip patch to MozReview.
Attachment #8851205 - Attachment is obsolete: true
Comment on attachment 8851368 [details]
Bug 1347821 - update stylo mochitest expections for font-language-override support.

Hi xidorn, I've addressed the comments you mentioned in Comment 5.

The only error left is the `extern crate byteorder;` build error like follows:

--
0:50.72 error[E0432]: unresolved import `byteorder::BigEndian`
 0:50.72      --> /Users/jeremy/gecko-dev/stylo/toolkit/library/x86_64-apple-darwin/release/build/style-2a83f91d92f587e1/out/properties.rs:24864:17
 0:50.72       |
 0:50.72 24864 |             use byteorder::BigEndian;
 0:50.72       |                 ^^^^^^^^^^^^^^^^^^^^ Maybe a missing `extern crate byteorder;`?
--

As I said in Comment 6, I can't run
>./mach vendor rust
to update the library automatically. (Due to Bug 1348475)

I can try a work around, something like sending a PR to Servo (add byteorder to Carto.toml, and run `cargo update` to generate the patch), and wait this PR to be merged to m-c. Do you think this is the right direction?
Attachment #8851368 - Flags: feedback?(xidorn+moz)
Comment on attachment 8851368 [details]
Bug 1347821 - update stylo mochitest expections for font-language-override support.

https://reviewboard.mozilla.org/r/123676/#review126138

::: servo/components/style/properties/longhand/font.mako.rs:972
(Diff revision 1)
>          pub enum T {
> -            Normal,
> +            Override(u32),
> -            Override(String),
>          }

If there is no other variant, it should just be
```rust
pub struct T(u32);
```
or
```rust
pub type T = u32;
```

Not sure which is better, though.

::: servo/components/style/properties/longhand/font.mako.rs:1002
(Diff revision 1)
> +                    if lang.len() == 0 || lang.len() > 4 || !lang.is_ascii() {
> +                        return computed_value::T::Override(0);
> +                    } else {
> +                        let mut x = lang.len();
> +                        while x < 4 {
> +                            lang.push(' ');

Have you tried to build? It doesn't seem to me this would work... self is not mut, and thus lang is not. You need a buffer for it.
It seems to me this would be a pure-Rust patch. Please submit PR to Servo directly, and ask Manishearth or emilio to review. I don't think I'm sensitive enough to catch issues in Rust code from people who are less experienced in Rust.
Comment on attachment 8851368 [details]
Bug 1347821 - update stylo mochitest expections for font-language-override support.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> Comment on attachment 8851368 [details]
> Bug 1347821 - (wip) stylo: Implement gecko glue for font-language-override.
> 
> https://reviewboard.mozilla.org/r/123676/#review126138
> 
> ::: servo/components/style/properties/longhand/font.mako.rs:972
> (Diff revision 1)
> >          pub enum T {
> > -            Normal,
> > +            Override(u32),
> > -            Override(String),
> >          }
> 
> If there is no other variant, it should just be
> ```rust
> pub struct T(u32);
> ```
> or
> ```rust
> pub type T = u32;
> ```
> 
> Not sure which is better, though.

Looks like "pub type T = u32;" is more conventional. I'll fix this in the next version.

> ::: servo/components/style/properties/longhand/font.mako.rs:1002
> (Diff revision 1)
> > +                    if lang.len() == 0 || lang.len() > 4 || !lang.is_ascii() {
> > +                        return computed_value::T::Override(0);
> > +                    } else {
> > +                        let mut x = lang.len();
> > +                        while x < 4 {
> > +                            lang.push(' ');
> 
> Have you tried to build? It doesn't seem to me this would work... self is
> not mut, and thus lang is not. You need a buffer for it.

Yeah, the patch indeed passed my local build.
I'll double check this.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> It seems to me this would be a pure-Rust patch. Please submit PR to Servo
> directly, and ask Manishearth or emilio to review. I don't think I'm
> sensitive enough to catch issues in Rust code from people who are less
> experienced in Rust.

Okay, I'll ask for review from other people once my patch is ready.
Thank you for the feedback.
Attachment #8851368 - Flags: feedback?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> ::: servo/components/style/properties/longhand/font.mako.rs:1002
> (Diff revision 1)
> > +                    if lang.len() == 0 || lang.len() > 4 || !lang.is_ascii() {
> > +                        return computed_value::T::Override(0);
> > +                    } else {
> > +                        let mut x = lang.len();
> > +                        while x < 4 {
> > +                            lang.push(' ');
> 
> Have you tried to build? It doesn't seem to me this would work... self is
> not mut, and thus lang is not. You need a buffer for it.

Quick reply, you're right.
The error didn't show up due to the early error caused by the extern crate use. Once I bypassed the extern crate stuff, this error shows up. I'll fix it.
Attachment #8851368 - Flags: review?(emilio+bugs)
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19d58788a41778ca0f2ae8ac9087e6dfaab48ddb

Looks like I did reduced couple failures [1] :

```
    test_compute_data_with_start_struct.html font-language-override [2] => [0]
    test_inherit_computation.html font-language-override [8] => [0]
    test_inherit_storage.html font-language-override [12] => [2]
    test_initial_computation.html font-language-override [4] => [0]
    test_initial_storage.html font-language-override [6] => [1]
    test_value_storage.html font-language-override [58] => [28]
```

There're still few failures, so I might miss something...


[1] http://searchfox.org/mozilla-central/source/layout/style/test/stylo-failures.md
Comment on attachment 8851368 [details]
Bug 1347821 - update stylo mochitest expections for font-language-override support.

https://reviewboard.mozilla.org/r/123676/#review126248

Looks good! I have a few comments that should be addressed before landing. r=me with those.

Thanks for fixing this!

::: servo/components/style/properties/longhand/font.mako.rs:956
(Diff revision 4)
> +        use cssparser;
>  
>          impl ToCss for T {
>              fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> -                match *self {
> -                    T::Normal => dest.write_str("normal"),
> +                if self.0 == 0 {
> +                    dest.write_str("normal")

nit: let's just `return dest.write_str("normal")`, and deindent the rest of the function.

::: servo/components/style/properties/longhand/font.mako.rs:959
(Diff revision 4)
>              fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> -                match *self {
> -                    T::Normal => dest.write_str("normal"),
> -                    T::Override(ref lang) => write!(dest, "\"{}\"", lang),
> +                if self.0 == 0 {
> +                    dest.write_str("normal")
> +                } else {
> +                    let mut buf = [0; 4];
> +                    BigEndian::write_u32(&mut buf, self.0);

Do we really need to pull the `byteorder` dependency for this? Seems we always do it as `BigEndian`, and the string is always ascii, so I'm not sure I see the point on it as long as we're consistent with how we store it. I guess we already depend on it for other stuff, so should be ok... Feel free to leave it there if you wish :)

::: servo/components/style/properties/longhand/font.mako.rs:960
(Diff revision 4)
> -                match *self {
> -                    T::Normal => dest.write_str("normal"),
> -                    T::Override(ref lang) => write!(dest, "\"{}\"", lang),
> +                if self.0 == 0 {
> +                    dest.write_str("normal")
> +                } else {
> +                    let mut buf = [0; 4];
> +                    BigEndian::write_u32(&mut buf, self.0);
> +                    let s_slice : &str = &*String::from_utf8(buf.to_vec()).unwrap();

nit: let's use instead `str::from_utf8(&buf)` (https://doc.rust-lang.org/std/str/fn.from_utf8.html) to avoid allocating an extra buffer unnecessarily. We could also avoid the runtime checks in release builds (given we know the string is ascii after parsing) with something like:

```
let slice = if cfg!(debug_assertions) {
    str::from_utf8(&buf).unwrap()
} else {
    unsafe { str::from_utf8_unchecked(&buf) }
};
```

But not sure it's worth given the string is never more thatn 4 characters long.

::: servo/components/style/properties/longhand/font.mako.rs:966
(Diff revision 4)
> +                    cssparser::serialize_string(s_slice.trim_right(), dest)
>                  }
>              }
>          }
>  
> -        #[derive(Clone, Debug, PartialEq)]
> +        pub struct T(pub u32);

This definitely deserves a comment regarding how we store the tags, doesn't it? Please document it :)

::: servo/components/style/properties/longhand/font.mako.rs:990
(Diff revision 4)
> +            match *self {
> +                SpecifiedValue::Normal => computed_value::T(0),
> +                SpecifiedValue::Override(ref lang) => {
> +                    if lang.len() == 0 || lang.len() > 4 || !lang.is_ascii() {
> +                        return computed_value::T(0);
> +                    } else {

nit: You can deindent the block below this, given you return above.
Attachment #8851368 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8851368 [details]
Bug 1347821 - update stylo mochitest expections for font-language-override support.

https://reviewboard.mozilla.org/r/123676/#review126248

> Do we really need to pull the `byteorder` dependency for this? Seems we always do it as `BigEndian`, and the string is always ascii, so I'm not sure I see the point on it as long as we're consistent with how we store it. I guess we already depend on it for other stuff, so should be ok... Feel free to leave it there if you wish :)

I'm not sure how to simply the codes here w/o using the `byteorder` dependency, so I'll leave it there for now. I'm fine w/ removing the dependency as long as you could enlignten me a bit. Feel free to comment. :)

> nit: let's use instead `str::from_utf8(&buf)` (https://doc.rust-lang.org/std/str/fn.from_utf8.html) to avoid allocating an extra buffer unnecessarily. We could also avoid the runtime checks in release builds (given we know the string is ascii after parsing) with something like:
> 
> ```
> let slice = if cfg!(debug_assertions) {
>     str::from_utf8(&buf).unwrap()
> } else {
>     unsafe { str::from_utf8_unchecked(&buf) }
> };
> ```
> 
> But not sure it's worth given the string is never more thatn 4 characters long.

Well, a debug assertion seems good, and the code would looks more readable to me, so I'll take your advice.

> This definitely deserves a comment regarding how we store the tags, doesn't it? Please document it :)

Yes, you're right!!! Will do.

> nit: You can deindent the block below this, given you return above.

There's one similar pattern below, I'll fix that as well.
(In reply to Jeremy Chen [:jeremychen] UTC+8 (Away 4/3-4/4) from comment #17)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=19d58788a41778ca0f2ae8ac9087e6dfaab48ddb
> 
> Looks like I did reduced couple failures [1] :
> 
> ```
>     test_compute_data_with_start_struct.html font-language-override [2] =>
> [0]
>     test_inherit_computation.html font-language-override [8] => [0]
>     test_inherit_storage.html font-language-override [12] => [2]
>     test_initial_computation.html font-language-override [4] => [0]
>     test_initial_storage.html font-language-override [6] => [1]
>     test_value_storage.html font-language-override [58] => [28]
> ```
> 
> There're still few failures, so I might miss something...
> 

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c308028b4980051dbf58714d1daff402004a1c98
After I patched shorthand, the test expectations could be further improved:
```
  test_compute_data_with_start_struct.html font-language-override [2] => [0]
  test_inherit_computation.html font-language-override [8] => [0]
  test_inherit_storage.html font-language-override [12] => [0]
  test_initial_computation.html font-language-override [4] => [0]
  test_initial_storage.html font-language-override [6] => [0]
  test_value_storage.html font-language-override [58] => [16]
```

I'll send the PR with the shorthand fix, and re-request review there.
Comment on attachment 8851368 [details]
Bug 1347821 - update stylo mochitest expections for font-language-override support.

Clear r+ since this patch has not been reviewed. Looks like a MozReview bug...:/
Attachment #8851368 - Flags: review+
Update mochitest expectations according to the latest try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a8e32decc5253b2e37206f997543af00fb3282c

Hi Xidorn, since the PR (comment 22) is awaiting-merge, could you help review this part?
Attachment #8851368 - Flags: review?(xidorn+moz)
Comment on attachment 8851368 [details]
Bug 1347821 - update stylo mochitest expections for font-language-override support.

https://reviewboard.mozilla.org/r/123676/#review126580
Attachment #8851368 - Flags: review?(xidorn+moz) → review+
Looks like the PR has been merged to autoland for couple hours:
https://hg.mozilla.org/integration/autoland/rev/147f36db5c3a

Let's land the expection update part.
I think bholley has updated the expectation for you in https://hg.mozilla.org/integration/autoland/rev/dbcf115d2df400f7cd71e4a93b895baf9ac8c6d4

So you don't need to try to push that anymore.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Nice. Thank you. :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: