Improve behaviour of image elements that perform multiple requests #15771

jdm · 2017-02-28T20:03:19Z

This addresses cases where image elements end up making multiple requests, as well as makes the element respond to additional relevant mutations that trigger updating the image data.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15709 (github issue number if applicable).
There are tests for these changes

This change is


        Remove mutability requirement from fetch_async_background.


        Ignore image cache updates for image requests that have been aborted.

highfive · 2017-02-28T20:03:23Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/document.rs, components/script/dom/htmlimageelement.rs, components/script/document_loader.rs, components/script/script_thread.rs
@KiChjang: components/script/dom/document.rs, components/script/dom/htmlimageelement.rs, components/script/document_loader.rs, components/script/script_thread.rs

highfive · 2017-02-28T20:03:24Z

Warning

These commits modify unsafe code. Please review it carefully!

jdm · 2017-02-28T20:03:26Z

@bors-servo: try

bors-servo · 2017-02-28T20:03:30Z

⌛ Trying commit 06e8bc0 with merge 3d4a1c6...


        Auto merge of #15771 - jdm:img-panic, r=<try>

Improve behaviour of imag elements that perform multiple requests This addresses cases where image elements end up making multiple requests, as well as makes the element respond to additional relevant mutations that trigger updating the image data. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15709 (github issue number if applicable). - [X] There are tests for these changes

jdm · 2017-02-28T20:03:51Z

@nox You will be interested in ff53ec2 in particular.

bors-servo · 2017-02-28T20:06:03Z

💔 Test failed - arm32

notriddle · 2017-02-28T22:56:42Z

@bors-servo: retry

bors-servo · 2017-02-28T22:56:48Z

⌛ Trying commit 06e8bc0 with merge 353a59b...


        Auto merge of #15771 - jdm:img-panic, r=<try>

Improve behaviour of image elements that perform multiple requests This addresses cases where image elements end up making multiple requests, as well as makes the element respond to additional relevant mutations that trigger updating the image data. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15709 (github issue number if applicable). - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15771)

bors-servo · 2017-02-28T23:01:25Z

💔 Test failed - linux-dev


        Initiate a new image request when an image element's adopting steps a…

…re run. Remove the redundant tracked loads for the underlying image request; the load blockers present in the ImageRequest structure perform the same duty with better accuracy.

jdm · 2017-03-01T18:48:07Z

@bors-servo: try

bors-servo · 2017-03-01T18:48:15Z

⌛ Trying commit 859d7f7 with merge 54f0bac...


        Auto merge of #15771 - jdm:img-panic, r=<try>

Improve behaviour of image elements that perform multiple requests This addresses cases where image elements end up making multiple requests, as well as makes the element respond to additional relevant mutations that trigger updating the image data. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15709 (github issue number if applicable). - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15771)

bors-servo · 2017-03-01T19:16:36Z

💔 Test failed - mac-rel-wpt1

nox · 2017-03-03T10:05:07Z

components/script/script_thread.rs

@@ -483,6 +483,8 @@ pub struct ScriptThread {

    /// A handle to the webvr thread, if available
    webvr_thread: Option<IpcSender<WebVRMsg>>,
+
+    docs_with_no_blocking_loads: DOMRefCell<HashSet<PipelineId>>,


Please document the field.

You didn't document the field.

Weird, I remember writing the documentation. I wonder what happened to it...

nox · 2017-03-03T10:09:54Z

  │ assertion failed: !self.loader.borrow().events_inhibited() (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(1) }, at /Users/servo/buildbot/slave/mac-rel-wpt1/build/components/script/dom/document.rs:1622)


        Move checks for document completion to the end of the event loop.

This better reflects the text of the specification - rather than queuing a task to dispatch the load evnet as soon as the document loader is unblocked, we want to "spin the event loop until there is nothing that delays the load event in the Document." Spinning the event loop is a concept that requires running tasks completely, hence we check the condition before returning to the start of the event loop.


        Test adopting images into documents with different base URLs.


        Verify that aborted image requests do not cause extraneous load events.


        Avoid marking image element as complete before its image data is avai…

…lable.

jdm · 2017-03-07T10:00:23Z

@bors-servo: try

bors-servo · 2017-03-07T10:00:28Z

⌛ Trying commit f798507 with merge 14e7f24...


        Auto merge of #15771 - jdm:img-panic, r=<try>

Improve behaviour of image elements that perform multiple requests This addresses cases where image elements end up making multiple requests, as well as makes the element respond to additional relevant mutations that trigger updating the image data. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15709 (github issue number if applicable). - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15771)

bors-servo · 2017-03-07T10:35:26Z

☀️ 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-gnu-dev, windows-msvc-dev
State: approved= try=True

nox · 2017-03-07T11:06:47Z

@bors-servo r+

bors-servo · 2017-03-07T11:06:48Z

📌 Commit f798507 has been approved by nox

nox · 2017-03-08T12:03:11Z

@bors-servo try- retry

bors-servo · 2017-03-08T12:29:00Z

⌛ Testing commit f798507 with merge f90fc2f...


        Auto merge of #15771 - jdm:img-panic, r=nox

Improve behaviour of image elements that perform multiple requests This addresses cases where image elements end up making multiple requests, as well as makes the element respond to additional relevant mutations that trigger updating the image data. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15709 (github issue number if applicable). - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15771)

bors-servo · 2017-03-08T13:18:46Z

☀️ 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-gnu-dev, windows-msvc-dev
Approved by: nox
Pushing f90fc2f to master...


        Test adopting images into documents with different base URLs.

Upstreamed from servo/servo#15771 [ci skip]


        Avoid marking image element as complete before its image data is avai…

…lable. Upstreamed from servo/servo#15771 [ci skip]

jdm added 2 commits Feb 28, 2017

Remove mutability requirement from fetch_async_background.

f0f43ad

Ignore image cache updates for image requests that have been aborted.

208f974

highfive assigned emilio Feb 28, 2017

highfive added the S-awaiting-review label Feb 28, 2017

highfive added the S-tests-failed label Feb 28, 2017

jdm changed the title ~~Improve behaviour of imag elements that perform multiple requests~~ Improve behaviour of image elements that perform multiple requests Feb 28, 2017

jdm removed the S-tests-failed label Feb 28, 2017

highfive added the S-tests-failed label Feb 28, 2017

Initiate a new image request when an image element's adopting steps a…

2bd02fe

…re run. Remove the redundant tracked loads for the underlying image request; the load blockers present in the ImageRequest structure perform the same duty with better accuracy.

jdm force-pushed the jdm:img-panic branch from 06e8bc0 to 859d7f7 Mar 1, 2017

highfive removed the S-tests-failed label Mar 1, 2017

highfive added the S-tests-failed label Mar 1, 2017

nox reviewed Mar 3, 2017

View changes

nox added the S-needs-code-changes label Mar 3, 2017

nox added S-needs-code-changes and removed S-awaiting-review labels Mar 5, 2017

jdm added 4 commits Feb 28, 2017

Test adopting images into documents with different base URLs.

330e222

Verify that aborted image requests do not cause extraneous load events.

db79dfb

Avoid marking image element as complete before its image data is avai…

Loading status checks…

f798507

…lable.

jdm force-pushed the jdm:img-panic branch from fce8e34 to f798507 Mar 7, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 7, 2017

nox approved these changes Mar 7, 2017

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 7, 2017

bors-servo merged commit f798507 into servo:master Mar 8, 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 Mar 8, 2017

bors-servo mentioned this pull request Mar 8, 2017

Make SSL tests work #15784

Merged

jdm added a commit to web-platform-tests/wpt that referenced this pull request Apr 17, 2017

Test adopting images into documents with different base URLs.

Loading status checks…

f535704

Upstreamed from servo/servo#15771 [ci skip]

jdm mentioned this pull request Apr 17, 2017

Test adopting images into documents with different base URLs. web-platform-tests/wpt#5574

Merged

jdm added a commit to web-platform-tests/wpt that referenced this pull request Apr 17, 2017

Avoid marking image element as complete before its image data is avai…

Loading status checks…

180ca29

…lable. Upstreamed from servo/servo#15771 [ci skip]

jdm mentioned this pull request Apr 17, 2017

Avoid marking image element as complete before its image data is available. web-platform-tests/wpt#5575

Merged

gsnedders mentioned this pull request Aug 2, 2017

Add a couple of tests that img delays the load event web-platform-tests/wpt#5509

Merged

servo / servo

Improve behaviour of image elements that perform multiple requests #15771

Improve behaviour of image elements that perform multiple requests #15771

jdm commented Feb 28, 2017 •

edited by larsbergstrom

highfive commented Feb 28, 2017

highfive commented Feb 28, 2017

jdm commented Feb 28, 2017

bors-servo commented Feb 28, 2017

jdm commented Feb 28, 2017

bors-servo commented Feb 28, 2017

notriddle commented Feb 28, 2017

bors-servo commented Feb 28, 2017

bors-servo commented Feb 28, 2017

jdm commented Mar 1, 2017

bors-servo commented Mar 1, 2017

bors-servo commented Mar 1, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Mar 3, 2017

jdm commented Mar 7, 2017

bors-servo commented Mar 7, 2017

bors-servo commented Mar 7, 2017

nox commented Mar 7, 2017

bors-servo commented Mar 7, 2017

nox commented Mar 8, 2017

bors-servo commented Mar 8, 2017

bors-servo commented Mar 8, 2017

servo / servo

Join GitHub today

Improve behaviour of image elements that perform multiple requests #15771

Improve behaviour of image elements that perform multiple requests #15771

Conversation

jdm commented Feb 28, 2017 • edited by larsbergstrom

highfive commented Feb 28, 2017

highfive commented Feb 28, 2017

jdm commented Feb 28, 2017

bors-servo commented Feb 28, 2017

jdm commented Feb 28, 2017

bors-servo commented Feb 28, 2017

notriddle commented Feb 28, 2017

bors-servo commented Feb 28, 2017

bors-servo commented Feb 28, 2017

jdm commented Mar 1, 2017

bors-servo commented Mar 1, 2017

bors-servo commented Mar 1, 2017

This comment has been minimized.

nox Mar 3, 2017

This comment has been minimized.

nox Mar 5, 2017

This comment has been minimized.

jdm Mar 6, 2017

nox commented Mar 3, 2017

jdm commented Mar 7, 2017

bors-servo commented Mar 7, 2017

bors-servo commented Mar 7, 2017

nox commented Mar 7, 2017

bors-servo commented Mar 7, 2017

nox commented Mar 8, 2017

bors-servo commented Mar 8, 2017

bors-servo commented Mar 8, 2017

jdm commented Feb 28, 2017 •

edited by larsbergstrom