Refactor ImageCache::find_image_or_metadata -> ImageCache::{get_image, track_image} #23661

julientregoat · 2019-06-30T02:28:22Z

Updated the ImageCache trait to replace find_image_or_metadata with two new functions track_image and get_image, as well as a new enum (ImageCacheResult).

As a result, I was able to refactor the functions that previously called find_image_or_metadata pretty cleanly. For a list of these functions, please see the commit information.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #21289 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because tests already exist for these components. I ran cargo test in net, net_traits, layout, and script successfully.

This change is

highfive · 2019-06-30T02:28:29Z

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

highfive · 2019-06-30T02:28:30Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmlvideoelement.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/htmlimageelement.rs, components/script/dom/htmlcanvaselement.rs
@KiChjang: components/script/dom/htmlvideoelement.rs, components/script/dom/canvasrenderingcontext2d.rs, components/net/image_cache.rs, components/net_traits/image_cache.rs, components/script/dom/htmlimageelement.rs and 1 more
@emilio: components/layout/context.rs

highfive · 2019-06-30T02:28:31Z

Warning

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

julientregoat · 2019-07-02T00:26:53Z

@jdm re:

It's possible we could split this further into APIs that are allowed to load a placeholder and those that aren't, and reduce the set of possible responses that be received in the case where a placeholder image is not allowed.

I explored this, but it seemed like every way I thought of refactoring it would add the same amount of code, or duplicate functionality. I was looking into areas that called these functions, disallowing placeholder images.From what I can tell, the UsePlaceholder enum ultimately comes down to ImageCacheStore::get_completed_image_if_available which handles it reasonably elegantly... but idk, I'm new to this codebase and I'm sure there's something I'm not understanding or missing. What did you have in mind for that part of the refactor?

components/layout/context.rs

jdm

Good start! I've left some suggestions that I think will reduce some of the duplicated code and make it easier to understand the intentions of the code using the image cache.

components/layout/context.rs

components/net/image_cache.rs

components/script/dom/htmlimageelement.rs

components/net_traits/image_cache.rs

components/script/dom/htmlimageelement.rs

components/script/dom/htmlvideoelement.rs

julientregoat · 2019-07-11T18:42:41Z

thanks so much for the thorough review, will get on this ASAP!

jdm

This is much improved! I think we can still simplify the rules of the API a little bit more by ensuring that the callers cannot forget to add an image cache listener.

jdm · 2019-07-19T18:27:16Z

components/net/image_cache.rs

        }

+        let (sender, _receiver) = ipc::channel().unwrap();


This looks suspicious to me - I believe callers of track_image should be passing a sender as an argument so they can make use of the receiver. Right now any notifications go into the void because the receiver is ignored completely.

not sure what I was thinking here!

jdm · 2019-07-19T18:27:16Z

components/script/dom/htmlimageelement.rs

-
-            Err(ImageState::NotRequested(id)) => {
+            ImageCacheResult::Pending(id) => add_cache_listener_for_element(image_cache, id, self),
+            ImageCacheResult::ReadyForRequest(id) => {
                add_cache_listener_for_element(image_cache, id, self);


I think the idea I had is that the track_image API would automatically call ImageCache::add_listener so that users of the API would not have to do that.

julientregoat · 2019-07-28T00:07:22Z

components/layout/context.rs

-        // See if the image is already available
-        let result =
+        // Check for available image or start tracking.
+        let sender; // FIXME


@jdm I left this broken for the moment. I wasn't sure where & how I should generate the IpcSender for the call to track_image here, since the LayoutContext isn't an element like the other callers of track_image. Based on the comments in the code above I imagined what I would want to do would be to add a route to the ipc_channel::router::ROUTER with a callback that would trigger the reflow. However I also realized that it might make more sense to have the caller of get_or_request_image_or_meta pass a sender in. This would ultimately be Fragements and ImageFragmentInfos. any advice?

Ok, I spent a while looking at this, and I think we don't actually want to call track_image here, because it doesn't really fit the model of the other consumers of that API. This code cares about three states:

is there an image/metadata already?

is there an image cache entry pending?

is there no image cache entry?

We only want to add a cache listener in states 2 and 3, and that listener is added in separate code in components/script/dom/window.rs (in Window::force_reflow). I think we should add a new image cache API called get_cached_image_status which returns ImageCacheResult and does not take a Sender argument. Hopefully we can find a good way to avoid duplicating the implementation of track_image.

jdm · 2019-09-04T15:04:24Z

I'm really sorry for forgetting about this PR!


        Refactor ImageCache::find_image_or_metadata API.


        Don't continually re-request completed requests that had load errors.

jdm · 2020-04-17T16:00:56Z

@bors-servo r+
Thanks!

bors-servo · 2020-04-17T16:00:57Z

📌 Commit d4e85f9 has been approved by jdm

bors-servo · 2020-04-17T16:32:01Z

⌛ Testing commit d4e85f9 with merge fccfe08...


        Auto merge of #23661 - julientregoat:i-21289, r=jdm

Refactor ImageCache::find_image_or_metadata -> ImageCache::{get_image, track_image}  Updated the `ImageCache` trait to replace `find_image_or_metadata` with two new functions `track_image` and `get_image`, as well as a new enum (`ImageCacheResult`). As a result, I was able to refactor the functions that previously called `find_image_or_metadata` pretty cleanly. For a list of these functions, please see the commit information. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21289 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [x] These changes do not require tests because tests already exist for these components. I ran `cargo test` in `net`, `net_traits`, `layout`, and `script` successfully.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23661)

bors-servo · 2020-04-17T17:28:46Z

💔 Test failed - status-taskcluster

jdm · 2020-04-17T17:51:41Z

@bors-servo retry

#24622

bors-servo · 2020-04-17T17:55:58Z

⌛ Testing commit d4e85f9 with merge 912ecb3...


        Auto merge of #23661 - julientregoat:i-21289, r=jdm

Refactor ImageCache::find_image_or_metadata -> ImageCache::{get_image, track_image}  Updated the `ImageCache` trait to replace `find_image_or_metadata` with two new functions `track_image` and `get_image`, as well as a new enum (`ImageCacheResult`). As a result, I was able to refactor the functions that previously called `find_image_or_metadata` pretty cleanly. For a list of these functions, please see the commit information. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21289 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [x] These changes do not require tests because tests already exist for these components. I ran `cargo test` in `net`, `net_traits`, `layout`, and `script` successfully.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23661)

bors-servo · 2020-04-17T18:45:29Z

💔 Test failed - status-taskcluster

jdm · 2020-04-17T18:46:39Z

@bors-servo retry

#25513

bors-servo · 2020-04-17T19:56:32Z

⌛ Testing commit d4e85f9 with merge c9480c8...

bors-servo · 2020-04-17T21:03:43Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing c9480c8 to master...

highfive assigned asajeffrey Jun 30, 2019

highfive added the S-awaiting-review label Jun 30, 2019

julientregoat force-pushed the julientregoat:i-21289 branch from d79cffc to 4c700ef Jul 2, 2019

CYBAI reviewed Jul 2, 2019

View changes

components/layout/context.rs Outdated Show resolved Hide resolved

julientregoat force-pushed the julientregoat:i-21289 branch from 125e12c to 14b7460 Jul 2, 2019

jdm assigned jdm and unassigned asajeffrey Jul 4, 2019

jdm requested changes Jul 10, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 10, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 12, 2019

julientregoat force-pushed the julientregoat:i-21289 branch 2 times, most recently from 214c84d to 3427dc1 Jul 12, 2019

jdm requested changes Jul 19, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 19, 2019

julientregoat force-pushed the julientregoat:i-21289 branch from 3427dc1 to 09c74ef Jul 27, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 27, 2019

julientregoat reviewed Jul 28, 2019

View changes

julientregoat force-pushed the julientregoat:i-21289 branch from 09c74ef to 6994314 Jul 28, 2019

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 4, 2019

julientregoat and others added 2 commits Jun 29, 2019

Refactor ImageCache::find_image_or_metadata API.

2742fd2

Don't continually re-request completed requests that had load errors.

Loading status checks…

d4e85f9

jdm force-pushed the julientregoat:i-21289 branch from 81a14de to d4e85f9 Apr 17, 2020

highfive added S-awaiting-review and removed S-tests-failed labels Apr 17, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 17, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 17, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 17, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 17, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 17, 2020

bors-servo merged commit c9480c8 into servo:master Apr 17, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 17, 2020

servo / servo

Refactor ImageCache::find_image_or_metadata -> ImageCache::{get_image, track_image} #23661

Refactor ImageCache::find_image_or_metadata -> ImageCache::{get_image, track_image} #23661

julientregoat commented Jun 30, 2019 •

edited by SimonSapin

highfive commented Jun 30, 2019

highfive commented Jun 30, 2019

highfive commented Jun 30, 2019

julientregoat commented Jul 2, 2019

jdm left a comment

julientregoat commented Jul 11, 2019 •

edited

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Sep 4, 2019

jdm commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

jdm commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

jdm commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

servo / servo

Join GitHub today

Refactor ImageCache::find_image_or_metadata -> ImageCache::{get_image, track_image} #23661

Refactor ImageCache::find_image_or_metadata -> ImageCache::{get_image, track_image} #23661

Conversation

julientregoat commented Jun 30, 2019 • edited by SimonSapin

highfive commented Jun 30, 2019

highfive commented Jun 30, 2019

highfive commented Jun 30, 2019

julientregoat commented Jul 2, 2019

jdm left a comment

julientregoat commented Jul 11, 2019 • edited

jdm left a comment

This comment has been minimized.

jdm Jul 19, 2019

This comment has been minimized.

julientregoat Jul 27, 2019

This comment has been minimized.

jdm Jul 19, 2019

This comment has been minimized.

julientregoat Jul 28, 2019

This comment has been minimized.

jdm Sep 4, 2019

jdm commented Sep 4, 2019

jdm commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

jdm commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

jdm commented Apr 17, 2020

bors-servo commented Apr 17, 2020

bors-servo commented Apr 17, 2020

julientregoat commented Jun 30, 2019 •

edited by SimonSapin

julientregoat commented Jul 11, 2019 •

edited