Skip to content

Commit

Permalink
Don't send a load event when a loaded image is actually the placeholder
Browse files Browse the repository at this point in the history
The image cache returns an `ImageCacheResult::ImageAvailable `the second
time you try getting the placeholder. This means that in some cases, the
loading of an image would fail, then the same image would get fetched
from the cache, the placeholder would be loaded from that but would be
seen as a normal image, firing a load event.

This made the tests in
`fetch/cross-origin-resource-policy/image-loads.html` fail depending on
their order.
  • Loading branch information
Eijebong committed May 8, 2020
1 parent 8249be3 commit bdbfde9
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 15 deletions.
2 changes: 1 addition & 1 deletion components/layout/context.rs
Expand Up @@ -163,7 +163,7 @@ impl<'a> LayoutContext<'a> {
}

match self.get_or_request_image_or_meta(node, url.clone(), use_placeholder) {
Some(ImageOrMetadataAvailable::ImageAvailable(image, _)) => {
Some(ImageOrMetadataAvailable::ImageAvailable { image, .. }) => {
let image_info = WebRenderImageInfo::from_image(&*image);
if image_info.key.is_none() {
Some(image_info)
Expand Down
4 changes: 2 additions & 2 deletions components/layout/fragment.rs
Expand Up @@ -431,8 +431,8 @@ impl ImageFragmentInfo {
layout_context
.get_or_request_image_or_meta(node.opaque(), url, UsePlaceholder::Yes)
.map(|result| match result {
ImageOrMetadataAvailable::ImageAvailable(i, _) => {
ImageOrMetadata::Image(i)
ImageOrMetadataAvailable::ImageAvailable { image, .. } => {
ImageOrMetadata::Image(image)
},
ImageOrMetadataAvailable::MetadataAvailable(m) => {
ImageOrMetadata::Metadata(m)
Expand Down
20 changes: 14 additions & 6 deletions components/net/image_cache.rs
Expand Up @@ -466,9 +466,12 @@ impl ImageCache for ImageCacheImpl {
match result {
Ok((image, image_url)) => {
debug!("{} is available", url);
return ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable(
image, image_url,
));
let is_placeholder = image_url == store.placeholder_url;
return ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable {
image,
url: image_url,
is_placeholder,
});
},
Err(()) => {
debug!("{} is not available", url);
Expand Down Expand Up @@ -515,9 +518,14 @@ impl ImageCache for ImageCacheImpl {
// TODO: make this behaviour configurable according to the caller's needs.
store.handle_decoder(decoded);
match store.get_completed_image_if_available(url, origin, cors_setting, use_placeholder) {
Some(Ok((image, image_url))) => ImageCacheResult::Available(
ImageOrMetadataAvailable::ImageAvailable(image, image_url),
),
Some(Ok((image, image_url))) => {
let is_placeholder = image_url == store.placeholder_url;
ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable {
image,
url: image_url,
is_placeholder,
})
},
_ => ImageCacheResult::LoadError,
}
}
Expand Down
7 changes: 6 additions & 1 deletion components/net_traits/image_cache.rs
Expand Up @@ -17,7 +17,12 @@ use std::sync::Arc;
/// Indicating either entire image or just metadata availability
#[derive(Clone, Debug, Deserialize, MallocSizeOf, Serialize)]
pub enum ImageOrMetadataAvailable {
ImageAvailable(#[ignore_malloc_size_of = "Arc"] Arc<Image>, ServoUrl),
ImageAvailable {
#[ignore_malloc_size_of = "Arc"]
image: Arc<Image>,
url: ServoUrl,
is_placeholder: bool,
},
MetadataAvailable(ImageMetadata),
}

Expand Down
14 changes: 11 additions & 3 deletions components/script/dom/htmlimageelement.rs
Expand Up @@ -329,8 +329,16 @@ impl HTMLImageElement {
);

match cache_result {
ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable(image, url)) => {
self.process_image_response(ImageResponse::Loaded(image, url))
ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable {
image,
url,
is_placeholder,
}) => {
if is_placeholder {
self.process_image_response(ImageResponse::PlaceholderLoaded(image, url))
} else {
self.process_image_response(ImageResponse::Loaded(image, url))
}
},
ImageCacheResult::Available(ImageOrMetadataAvailable::MetadataAvailable(m)) => {
self.process_image_response(ImageResponse::MetadataLoaded(m))
Expand Down Expand Up @@ -1109,7 +1117,7 @@ impl HTMLImageElement {
);

match cache_result {
ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable(_image, _url)) => {
ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable { .. }) => {
// Step 15
self.finish_reacting_to_environment_change(
selected_source,
Expand Down
8 changes: 6 additions & 2 deletions components/script/dom/htmlvideoelement.rs
Expand Up @@ -166,8 +166,12 @@ impl HTMLVideoElement {
);

match cache_result {
ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable(img, url)) => {
self.process_image_response(ImageResponse::Loaded(img, url));
ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable {
image,
url,
..
}) => {
self.process_image_response(ImageResponse::Loaded(image, url));
},
ImageCacheResult::ReadyForRequest(id) => {
self.do_fetch_poster_frame(poster_url, id, cancel_receiver)
Expand Down

0 comments on commit bdbfde9

Please sign in to comment.