[gfx] [layout] [style] Upgrade unicode-bidi to 0.3 #16779

behnam · 2017-05-09T01:18:08Z

Depends on servo/unicode-bidi#27 , which
upgrades unicode-bidi crate to 0.3.0.

Summary of changes:

Use unicode_bidi::Level (instead of u8) in all relevant places and
replace magic computations with (inline) method calls to Level API.
Doing so required adding unicode-bidi crate dependency to two more
components here: style and gfx. IMHO, totally makes sense, as
replaces local integer manipulations/checks with well-tested ones
already available in a common dependency.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable). [N/A]

There are tests for these changes OR
These changes do not require tests because unicode-bidi has its own tests and there's no logic change in this diff.

This change is

highfive · 2017-05-09T01:18:14Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @glennw (or someone else) soon.

highfive · 2017-05-09T01:18:14Z

Heads up! This PR modifies the following files:

@bholley: components/style/Cargo.toml, components/style/properties/longhand/inherited_text.mako.rs, components/style/lib.rs, components/style/logical_geometry.rs
@emilio: components/style/Cargo.toml, components/style/properties/longhand/inherited_text.mako.rs, components/style/lib.rs, components/layout/Cargo.toml, components/style/logical_geometry.rs and 2 more

highfive · 2017-05-09T01:18:15Z

Warning

These commits modify style, layout, and gfx code, but no tests are modified. Please consider adding a test!

behnam · 2017-05-09T01:49:32Z

r? @emilio @mbrubeck

behnam · 2017-05-16T19:37:45Z

unicode-bidi API is now going stable for 0.3.0. This should be good to go after servo/unicode-bidi#31 is landed.

emilio · 2017-05-17T09:11:44Z

./mach test-tidy is failing:

./Cargo.lock:1: duplicate versions for package `unicode-bidi`
	The following packages depend on version 0.2.5 from 'crates.io':
		idna
	The following packages depend on version 0.3.0 from '':
		gfx
		layout
		style

./Cargo.lock:1: duplicate versions for package `serde`
	The following packages depend on version 0.9.15 from 'crates.io':
		webvr_traits
		app_units
		bincode
		bluetooth_traits
		canvas_traits
		constellation
		core-graphics
		cssparser
		devtools
		devtools_traits
		dwrote
		euclid
		gfx
		gfx_traits
		hyper_serde
		ipc-channel
		layout
		msg
		net
		net_traits
		offscreen_gl_context
		profile
		profile_traits
		range
		rust-webvr
		script
		script_traits
		serde_json
		servo_config
		servo_url
		string_cache
		style
		style_traits
		unicode-bidi
		url_serde
		uuid
		webrender_traits
	The following packages depend on version 1.0.5 from 'crates.io':
		serde_test

behnam · 2017-05-17T17:37:42Z

Right, crates.io is not updated with the latest version yet. The idna crate is compatible with unicode-bidi:0.3.0 already, so getting 0.3.0 on crates.io should fix all the problems here.

I'll update here when it's ready. Thanks.

behnam · 2017-05-17T19:22:06Z

Okay, new unicode-bidi versions are published on crates.io and here's the latest state of this PR:

$ ./mach test-tidy
Checking the config file...
Checking directories for correct file extensions...
Checking files for tidiness...
./Cargo.lock:1: duplicate versions for package `unicode-bidi`
	The following packages depend on version 0.2.6 from 'crates.io':
		idna
	The following packages depend on version 0.3.0 from 'crates.io':
		gfx
		layout
		style
  Progress: 100% (11/11)

I have tried manually editing Cargo.lock to address this, but ./mach build puts 0.2.6 back in for idna. I'm guessing that it's because of servo depending on unicode-bidi's with_servo feature, but idna doesn't.

I suppose an eventual cargo update would fix this, but that changes just too much dependencies.

Anyways, I don't know what's the right fix here. Any suggestions, @emilio, @mbrubeck?

jdm · 2017-05-17T19:34:34Z

@behnam https://github.com/servo/rust-url/blob/master/idna/Cargo.toml#L22 needs to be updated, and a new version of idna needs to be published, and then Servo needs to update to the new idna version.


        [idna] Upgrade unicode-bidi to 0.3.0

For the needs of `idna`, `unicode-bidi:0.3.0` is backwards compatible, but this upgrade is needed to that `servo` can upgrade to the new version. Servo upgrade: servo/servo#16779


        [idna] Upgrade unicode-bidi to 0.3.0

For the needs of `idna`, `unicode-bidi:0.3.0` is backwards compatible, but this upgrade is needed to that `servo` can upgrade to the new version. Servo upgrade: servo/servo#16779

bors-servo · 2017-05-19T02:48:55Z

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


        Auto merge of #343 - behnam:bidi-0.3.0, r=mbrubeck

[idna] Upgrade unicode-bidi to 0.3.0 For the needs of `idna`, `unicode-bidi:0.3.0` is backwards compatible, but this upgrade is needed to that `servo` can upgrade to the new version. Servo upgrade: servo/servo#16779  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/343)

mbrubeck · 2017-05-19T17:44:39Z

Looks good! r=mbrubeck for the code changes. This should also fix some tests, so it should get a try run once it is mergeable.

behnam · 2017-05-22T17:26:29Z

Now good to go.

FYI, this triggers the following warning:

warning: use of deprecated item: please use `BidiInfo::visual_runs()` instead.
   --> /Users/behnam/code/servo/servo/components/layout/inline.rs:313:28
    |
313 |                 let runs = bidi::deprecated::visual_runs(range, &levels);
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: #[warn(deprecated)] on by default

I'm still not sure what's the expected behavior on this call-site, so the code is not updated. The new implementation of that function, BidiInfo::visual_runs(), needs access to the text (for the char length data) and their original bidi-type, and that's why it's now an instance method of BidiInfo.

behnam · 2017-05-23T00:04:04Z

Okay, fixed all the little side issues I caused here and all the tests are passing in idna and servo.

@mbrubeck, @emilio, r?

mbrubeck

r=mbrubeck with one minor change.

mbrubeck · 2017-05-23T00:51:15Z

components/layout/inline.rs

-                let runs = unicode_bidi::visual_runs(range, &levels);
+                // FIXME: Update to use BidiInfo::visual_runs, as this algorithm needs access to
+                // the original text and original BidiClass of its characters.
+                let runs = bidi::deprecated::visual_runs(range, &levels);


Please add #[allow(deprecated)] above this line, to silence the warning.

behnam · 2017-05-23T01:05:56Z

Review status: 0 of 10 files reviewed at latest revision, 1 unresolved discussion.

components/layout/inline.rs, line 313 at r1 (raw file):

Previously, mbrubeck (Matt Brubeck) wrote…

Please add #[allow(deprecated)] above this line, to silence the warning.

Done.

Comments from Reviewable


        [gfx] [layout] [style] Upgrade unicode-bidi to 0.3

mbrubeck · 2017-05-23T02:32:20Z

@bors-servo r+

bors-servo · 2017-05-23T02:32:21Z

📌 Commit 14c524d has been approved by mbrubeck

bors-servo · 2017-05-23T04:06:09Z

⌛ Testing commit 14c524d with merge edd6c2c...


        Auto merge of #16779 - behnam:bidi-0.3, r=mbrubeck

[gfx] [layout] [style] Upgrade unicode-bidi to 0.3 Depends on servo/unicode-bidi#27 , which upgrades `unicode-bidi` crate to `0.3.0`. Summary of changes: * Use `unicode_bidi::Level` (instead of `u8`) in all relevant places and replace magic computations with (inline) method calls to Level API. * Doing so required adding `unicode-bidi` crate dependency to two more components here: `style` and `gfx`. IMHO, totally makes sense, as replaces local integer manipulations/checks with well-tested ones already available in a common dependency. ---  - [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). [N/A]  - [ ] There are tests for these changes OR - [X] These changes do not require tests because `unicode-bidi` has its own tests and there's no logic change in this diff.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16779)

bors-servo · 2017-05-23T05:16:56Z

☀️ 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: mbrubeck
Pushing edd6c2c to master...

highfive assigned glennw May 9, 2017

highfive added the S-awaiting-review label May 9, 2017

highfive assigned emilio and unassigned glennw May 9, 2017

behnam force-pushed the behnam:bidi-0.3 branch 2 times, most recently from c66eab7 to ed427a7 May 16, 2017

behnam changed the title ~~Upgrade unicode-bid to ^0.3~~ [gfx] [layout] [style] Upgrade unicode-bidi to 0.3 May 16, 2017

behnam force-pushed the behnam:bidi-0.3 branch 3 times, most recently from 5f6bd02 to b67a184 May 16, 2017

behnam force-pushed the behnam:bidi-0.3 branch from b67a184 to 58caa87 May 17, 2017

behnam mentioned this pull request May 17, 2017

[idna] Upgrade unicode-bidi to 0.3.0 servo/rust-url#343

Merged

behnam force-pushed the behnam:bidi-0.3 branch from 58caa87 to 0b0f3c6 May 19, 2017

highfive added the S-needs-rebase label May 19, 2017

mbrubeck removed the S-awaiting-review label May 19, 2017

mbrubeck approved these changes May 19, 2017

View changes

behnam force-pushed the behnam:bidi-0.3 branch from 0b0f3c6 to a201b73 May 22, 2017

highfive added the S-awaiting-review label May 22, 2017

behnam mentioned this pull request May 22, 2017

Build with Rust 1.13 on Travis servo/rust-url#346

Merged

behnam force-pushed the behnam:bidi-0.3 branch 4 times, most recently from 5c4c61d to 2523e55 May 22, 2017

behnam removed the S-needs-rebase label May 23, 2017

mbrubeck requested changes May 23, 2017

View changes

[gfx] [layout] [style] Upgrade unicode-bidi to 0.3

Loading status checks…

14c524d

behnam force-pushed the behnam:bidi-0.3 branch from 2523e55 to 14c524d May 23, 2017

highfive assigned mbrubeck and unassigned emilio May 23, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels May 23, 2017

bors-servo merged commit 14c524d into servo:master May 23, 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

highfive removed the S-awaiting-merge label May 23, 2017

behnam deleted the behnam:bidi-0.3 branch May 23, 2017

servo / servo

[gfx] [layout] [style] Upgrade unicode-bidi to 0.3 #16779

[gfx] [layout] [style] Upgrade unicode-bidi to 0.3 #16779

behnam commented May 9, 2017 •

edited

highfive commented May 9, 2017

highfive commented May 9, 2017

highfive commented May 9, 2017

behnam commented May 9, 2017

behnam commented May 16, 2017

emilio commented May 17, 2017

behnam commented May 17, 2017

behnam commented May 17, 2017

jdm commented May 17, 2017

bors-servo commented May 19, 2017

mbrubeck commented May 19, 2017

behnam commented May 22, 2017

behnam commented May 23, 2017

mbrubeck left a comment

This comment has been minimized.

behnam commented May 23, 2017

mbrubeck commented May 23, 2017

bors-servo commented May 23, 2017

bors-servo commented May 23, 2017

bors-servo commented May 23, 2017

servo / servo

Join GitHub today

[gfx] [layout] [style] Upgrade unicode-bidi to 0.3 #16779

[gfx] [layout] [style] Upgrade unicode-bidi to 0.3 #16779

Conversation

behnam commented May 9, 2017 • edited

highfive commented May 9, 2017

highfive commented May 9, 2017

highfive commented May 9, 2017

behnam commented May 9, 2017

behnam commented May 16, 2017

emilio commented May 17, 2017

behnam commented May 17, 2017

behnam commented May 17, 2017

jdm commented May 17, 2017

bors-servo commented May 19, 2017

mbrubeck commented May 19, 2017

behnam commented May 22, 2017

behnam commented May 23, 2017

mbrubeck left a comment

This comment has been minimized.

mbrubeck May 23, 2017

behnam commented May 23, 2017

mbrubeck commented May 23, 2017

bors-servo commented May 23, 2017

bors-servo commented May 23, 2017

bors-servo commented May 23, 2017

behnam commented May 9, 2017 •

edited