Centralize construction of specified url() values in style. #13791

emilio · 2016-10-16T19:42:46Z

This reduces a decent amount of overhead in #13778.

r? @SimonSapin

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes OR

This change is

highfive · 2016-10-16T19:42:49Z

Heads up! This PR modifies the following files:

@bholley: components/style/parser.rs, components/style/stylesheets.rs, components/style/values/computed/image.rs, components/style/font_face.rs, components/style/properties/declaration_block.rs, components/style/keyframes.rs, components/style/values/specified/image.rs, components/style/properties/properties.mako.rs, components/style/viewport.rs
@fitzgen: components/script/dom/cssstyledeclaration.rs, components/script/dom/element.rs
@KiChjang: components/script/dom/cssstyledeclaration.rs, components/script/dom/element.rs

highfive · 2016-10-16T19:42:50Z

Warning

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

SimonSapin · 2016-10-17T12:48:55Z

I don’t see how this helps. You still need to do that work sooner or later. Worse, it’s possible for two threads to race and both do that work.

emilio · 2016-10-17T13:37:13Z

Maybe it doesn't show up anymore in the profile because I'm delaying and never getting there (maybe the font panic I'm getting in the demo happens before layout?).

In any case, this is going to be better in the case we don't need to resolve the computed url, for example if the element is not displayed. I'll investigate more though.

bors-servo · 2016-10-18T06:53:14Z

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

SimonSapin · 2016-11-05T22:19:15Z

I’m tempted to close this as I don’t think it’s a good idea, but let’s get a second opinion. @pcwalton, what do you think?

emilio · 2016-11-06T14:59:56Z

Yep, I agree resolving it lazily may not be a good idea.

I'd still like to refcount urls a lot more though, does that seem ok to you @SimonSapin?

bors-servo · 2016-11-09T15:21:15Z

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

emilio · 2016-11-09T15:40:23Z

So I ended up refactoring how we handle style urls, so the place is centralized and we can refactor this in the future more easily.

I agree with @SimonSapin we should be bypassing rust-url for data-uris, but I'd prefer to leave that as a followup for now.

r? @SimonSapin

SimonSapin · 2016-11-09T16:46:20Z

r=me with ToCss impl fixed. (See inline comment.)

Reviewed 15 of 15 files at r1, 4 of 5 files at r2.
Review status: 18 of 19 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

components/style/values/specified/url.rs, line 139 at r1 (raw file):

    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
        if let Some(ref original) = self.original {
            write!(dest, "url(\"{}\")", original)

This needs to use cssparser::serializer::CssStringWriter, for example in case original contains a double quote.

Comments from Reviewable


        Auto merge of #13791 - emilio:lazy-url, r=SimonSapin

@SimonSapin

Centralize construction of specified url() values in style. This reduces a decent amount of overhead in #13778. r? @SimonSapin ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13791)

bors-servo · 2016-11-09T20:16:05Z

⌛ Testing commit 7ea6f57 with merge c4f5deb...

bors-servo · 2016-11-09T21:04:09Z

💔 Test failed - linux-rel-wpt

highfive · 2016-11-09T21:04:13Z

  ▶ FAIL [expected PASS] /_mozilla/css/iframe/simple.html
  └   → /_mozilla/css/iframe/simple.html 6510831f41496e5a48d892ebc20e1815b08e27f2
/_mozilla/css/iframe/simple_ref.html 64daf248c924b7607e2ff04d1bdd7ef623df4cd7
Testing 6510831f41496e5a48d892ebc20e1815b08e27f2 == 64daf248c924b7607e2ff04d1bdd7ef623df4cd7

emilio · 2016-11-10T08:48:15Z

@bors-servo retry #14162

bors-servo · 2016-11-10T09:22:52Z

⌛ Testing commit 7ea6f57 with merge 200180a...


        Auto merge of #13791 - emilio:lazy-url, r=SimonSapin

@SimonSapin

Centralize construction of specified url() values in style. This reduces a decent amount of overhead in #13778. r? @SimonSapin ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13791)


        style: Centralize specified url value handling, and refcount urls.

emilio · 2016-11-10T12:17:43Z

@bors-servo r=SimonSapin

Homu seemed stuck so I rebased.

bors-servo · 2016-11-10T12:17:43Z

📌 Commit 5f2e7af has been approved by SimonSapin

nox · 2016-11-10T13:46:01Z

@bors-servo force clean retry

emilio · 2016-11-10T14:28:37Z

@bors-servo retry

Homu's back

bors-servo · 2016-11-10T14:28:41Z

⌛ Testing commit 5f2e7af with merge 164e956...


        Auto merge of #13791 - emilio:lazy-url, r=SimonSapin

@SimonSapin

Centralize construction of specified url() values in style. This reduces a decent amount of overhead in #13778. r? @SimonSapin ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13791)

bors-servo · 2016-11-10T15:19:50Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned SimonSapin Oct 16, 2016

highfive added the S-awaiting-review label Oct 16, 2016

emilio force-pushed the emilio:lazy-url branch from 4eb5f52 to bb29b72 Oct 16, 2016

SimonSapin added the S-awaiting-answer label Oct 17, 2016

highfive added the S-needs-rebase label Oct 18, 2016

emilio force-pushed the emilio:lazy-url branch from ae43627 to ef6cc69 Nov 9, 2016

highfive added the S-needs-rebase label Nov 9, 2016

emilio force-pushed the emilio:lazy-url branch 4 times, most recently from bc728ff to 2ed1422 Nov 9, 2016

emilio changed the title ~~Lazily resolve and refcount the URL in CSS image properties.~~ Centralize construction of specified url() values in style. Nov 9, 2016

emilio removed S-awaiting-answer S-needs-rebase labels Nov 9, 2016

emilio force-pushed the emilio:lazy-url branch from 2ed1422 to 97b6bec Nov 9, 2016

emilio force-pushed the emilio:lazy-url branch 3 times, most recently from 0b57d83 to 7f7faf8 Nov 9, 2016

emilio force-pushed the emilio:lazy-url branch 2 times, most recently from 1b41c7f to 0197734 Nov 9, 2016

highfive added the S-awaiting-merge label Nov 9, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 9, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 10, 2016

style: Centralize specified url value handling, and refcount urls.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

5f2e7af

emilio force-pushed the emilio:lazy-url branch from 7ea6f57 to 5f2e7af Nov 10, 2016

highfive added S-awaiting-review and removed S-awaiting-merge labels Nov 10, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 10, 2016

Ms2ger closed this Nov 10, 2016

Ms2ger reopened this Nov 10, 2016

bors-servo merged commit 5f2e7af into servo:master Nov 10, 2016
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 Nov 10, 2016

emilio deleted the emilio:lazy-url branch Nov 22, 2016

servo / servo

Centralize construction of specified url() values in style. #13791

Centralize construction of specified url() values in style. #13791

emilio commented Oct 16, 2016 •

edited by larsbergstrom

highfive commented Oct 16, 2016

highfive commented Oct 16, 2016

SimonSapin commented Oct 17, 2016

emilio commented Oct 17, 2016

bors-servo commented Oct 18, 2016

SimonSapin commented Nov 5, 2016

emilio commented Nov 6, 2016

bors-servo commented Nov 9, 2016

emilio commented Nov 9, 2016

SimonSapin commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

highfive commented Nov 9, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

nox commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

servo / servo

Join GitHub today

Centralize construction of specified url() values in style. #13791

Centralize construction of specified url() values in style. #13791

Conversation

emilio commented Oct 16, 2016 • edited by larsbergstrom

highfive commented Oct 16, 2016

highfive commented Oct 16, 2016

SimonSapin commented Oct 17, 2016

emilio commented Oct 17, 2016

bors-servo commented Oct 18, 2016

SimonSapin commented Nov 5, 2016

emilio commented Nov 6, 2016

bors-servo commented Nov 9, 2016

emilio commented Nov 9, 2016

SimonSapin commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

highfive commented Nov 9, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

nox commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

emilio commented Oct 16, 2016 •

edited by larsbergstrom