Propagate image resolution errors in layout context (#36692)

This commit modifies layout context to propagate any issues that occur
during image
resolution.

At the moment, when errors occur during image resolution we propagate
None upwards. This
hides any potential issues that may be actionable, for example, we may
want to avoid
trying to load an image that failed to load for whatever reason or has
an invalid url.

This commit instead propagates these errors upwards to consumers where
they may become
actionable. This is part of an investigation into #36679.

Signed-off-by: Astraea Quinn Skoutelli
<astraea.quinn.skoutelli@huawei.com>

Signed-off-by: Astraea Quinn Skoutelli <astraea.quinn.skoutelli@huawei.com>
This commit is contained in:
Astraea Quinn S 2025-04-28 14:32:51 +02:00 committed by GitHub
parent 4d975e947b
commit 56882a3d5b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 51 additions and 28 deletions

View file

@ -61,6 +61,20 @@ impl Drop for LayoutContext<'_> {
} }
} }
#[derive(Debug)]
pub enum ResolveImageError {
LoadError,
ImagePending,
ImageRequested,
OnlyMetadata,
InvalidUrl,
MissingNode,
ImageMissingFromImageSet,
FailedToResolveImageFromImageSet,
NotImplementedYet(&'static str),
None,
}
impl LayoutContext<'_> { impl LayoutContext<'_> {
#[inline(always)] #[inline(always)]
pub fn shared_context(&self) -> &SharedStyleContext { pub fn shared_context(&self) -> &SharedStyleContext {
@ -72,7 +86,7 @@ impl LayoutContext<'_> {
node: OpaqueNode, node: OpaqueNode,
url: ServoUrl, url: ServoUrl,
use_placeholder: UsePlaceholder, use_placeholder: UsePlaceholder,
) -> Option<ImageOrMetadataAvailable> { ) -> Result<ImageOrMetadataAvailable, ResolveImageError> {
// Check for available image or start tracking. // Check for available image or start tracking.
let cache_result = self.image_cache.get_cached_image_status( let cache_result = self.image_cache.get_cached_image_status(
url.clone(), url.clone(),
@ -82,7 +96,7 @@ impl LayoutContext<'_> {
); );
match cache_result { match cache_result {
ImageCacheResult::Available(img_or_meta) => Some(img_or_meta), ImageCacheResult::Available(img_or_meta) => Ok(img_or_meta),
// Image has been requested, is still pending. Return no image for this paint loop. // Image has been requested, is still pending. Return no image for this paint loop.
// When the image loads it will trigger a reflow and/or repaint. // When the image loads it will trigger a reflow and/or repaint.
ImageCacheResult::Pending(id) => { ImageCacheResult::Pending(id) => {
@ -93,7 +107,7 @@ impl LayoutContext<'_> {
origin: self.origin.clone(), origin: self.origin.clone(),
}; };
self.pending_images.lock().push(image); self.pending_images.lock().push(image);
None Result::Err(ResolveImageError::ImagePending)
}, },
// Not yet requested - request image or metadata from the cache // Not yet requested - request image or metadata from the cache
ImageCacheResult::ReadyForRequest(id) => { ImageCacheResult::ReadyForRequest(id) => {
@ -104,10 +118,10 @@ impl LayoutContext<'_> {
origin: self.origin.clone(), origin: self.origin.clone(),
}; };
self.pending_images.lock().push(image); self.pending_images.lock().push(image);
None Result::Err(ResolveImageError::ImageRequested)
}, },
// Image failed to load, so just return nothing // Image failed to load, so just return nothing
ImageCacheResult::LoadError => None, ImageCacheResult::LoadError => Result::Err(ResolveImageError::LoadError),
} }
} }
@ -136,31 +150,34 @@ impl LayoutContext<'_> {
node: OpaqueNode, node: OpaqueNode,
url: ServoUrl, url: ServoUrl,
use_placeholder: UsePlaceholder, use_placeholder: UsePlaceholder,
) -> Option<WebRenderImageInfo> { ) -> Result<WebRenderImageInfo, ResolveImageError> {
if let Some(existing_webrender_image) = self if let Some(existing_webrender_image) = self
.webrender_image_cache .webrender_image_cache
.read() .read()
.get(&(url.clone(), use_placeholder)) .get(&(url.clone(), use_placeholder))
{ {
return Some(*existing_webrender_image); return Ok(*existing_webrender_image);
} }
let image_or_meta =
match self.get_or_request_image_or_meta(node, url.clone(), use_placeholder) { self.get_or_request_image_or_meta(node, url.clone(), use_placeholder)?;
Some(ImageOrMetadataAvailable::ImageAvailable { image, .. }) => { match image_or_meta {
ImageOrMetadataAvailable::ImageAvailable { image, .. } => {
self.handle_animated_image(node, image.clone()); self.handle_animated_image(node, image.clone());
let image_info = WebRenderImageInfo { let image_info = WebRenderImageInfo {
size: Size2D::new(image.width, image.height), size: Size2D::new(image.width, image.height),
key: image.id, key: image.id,
}; };
if image_info.key.is_none() { if image_info.key.is_none() {
Some(image_info) Ok(image_info)
} else { } else {
let mut webrender_image_cache = self.webrender_image_cache.write(); let mut webrender_image_cache = self.webrender_image_cache.write();
webrender_image_cache.insert((url, use_placeholder), image_info); webrender_image_cache.insert((url, use_placeholder), image_info);
Some(image_info) Ok(image_info)
} }
}, },
None | Some(ImageOrMetadataAvailable::MetadataAvailable(..)) => None, ImageOrMetadataAvailable::MetadataAvailable(..) => {
Result::Err(ResolveImageError::OnlyMetadata)
},
} }
} }
@ -168,11 +185,15 @@ impl LayoutContext<'_> {
&self, &self,
node: Option<OpaqueNode>, node: Option<OpaqueNode>,
image: &'a Image, image: &'a Image,
) -> Option<ResolvedImage<'a>> { ) -> Result<ResolvedImage<'a>, ResolveImageError> {
match image { match image {
// TODO: Add support for PaintWorklet and CrossFade rendering. // TODO: Add support for PaintWorklet and CrossFade rendering.
Image::None | Image::CrossFade(_) | Image::PaintWorklet(_) => None, Image::None => Result::Err(ResolveImageError::None),
Image::Gradient(gradient) => Some(ResolvedImage::Gradient(gradient)), Image::CrossFade(_) => Result::Err(ResolveImageError::NotImplementedYet("CrossFade")),
Image::PaintWorklet(_) => {
Result::Err(ResolveImageError::NotImplementedYet("PaintWorklet"))
},
Image::Gradient(gradient) => Ok(ResolvedImage::Gradient(gradient)),
Image::Url(image_url) => { Image::Url(image_url) => {
// FIXME: images wont always have in intrinsic width or // FIXME: images wont always have in intrinsic width or
// height when support for SVG is added, or a WebRender // height when support for SVG is added, or a WebRender
@ -180,18 +201,20 @@ impl LayoutContext<'_> {
// //
// FIXME: It feels like this should take into account the pseudo // FIXME: It feels like this should take into account the pseudo
// element and not just the node. // element and not just the node.
let image_url = image_url.url()?; let image_url = image_url.url().ok_or(ResolveImageError::InvalidUrl)?;
let node = node.ok_or(ResolveImageError::MissingNode)?;
let webrender_info = self.get_webrender_image_for_url( let webrender_info = self.get_webrender_image_for_url(
node?, node,
image_url.clone().into(), image_url.clone().into(),
UsePlaceholder::No, UsePlaceholder::No,
)?; )?;
Some(ResolvedImage::Image(webrender_info)) Ok(ResolvedImage::Image(webrender_info))
}, },
Image::ImageSet(image_set) => { Image::ImageSet(image_set) => {
image_set image_set
.items .items
.get(image_set.selected_index) .get(image_set.selected_index)
.ok_or(ResolveImageError::ImageMissingFromImageSet)
.and_then(|image| { .and_then(|image| {
self.resolve_image(node, &image.image) self.resolve_image(node, &image.image)
.map(|info| match info { .map(|info| match info {

View file

@ -838,8 +838,8 @@ impl<'a> BuilderForBoxFragment<'a> {
// Reverse because the property is top layer first, we want to paint bottom layer first. // Reverse because the property is top layer first, we want to paint bottom layer first.
for (index, image) in b.background_image.0.iter().enumerate().rev() { for (index, image) in b.background_image.0.iter().enumerate().rev() {
match builder.context.resolve_image(node, image) { match builder.context.resolve_image(node, image) {
None => {}, Err(_) => {},
Some(ResolvedImage::Gradient(gradient)) => { Ok(ResolvedImage::Gradient(gradient)) => {
let intrinsic = NaturalSizes::empty(); let intrinsic = NaturalSizes::empty();
let Some(layer) = let Some(layer) =
&background::layout_layer(self, painter, builder, index, intrinsic) &background::layout_layer(self, painter, builder, index, intrinsic)
@ -875,7 +875,7 @@ impl<'a> BuilderForBoxFragment<'a> {
}, },
} }
}, },
Some(ResolvedImage::Image(image_info)) => { Ok(ResolvedImage::Image(image_info)) => {
// FIXME: https://drafts.csswg.org/css-images-4/#the-image-resolution // FIXME: https://drafts.csswg.org/css-images-4/#the-image-resolution
let dppx = 1.0; let dppx = 1.0;
let intrinsic = NaturalSizes::from_width_and_height( let intrinsic = NaturalSizes::from_width_and_height(
@ -1063,8 +1063,8 @@ impl<'a> BuilderForBoxFragment<'a> {
.context .context
.resolve_image(node, &border.border_image_source) .resolve_image(node, &border.border_image_source)
{ {
None => return false, Err(_) => return false,
Some(ResolvedImage::Image(image_info)) => { Ok(ResolvedImage::Image(image_info)) => {
let Some(key) = image_info.key else { let Some(key) = image_info.key else {
return false; return false;
}; };
@ -1073,7 +1073,7 @@ impl<'a> BuilderForBoxFragment<'a> {
height = image_info.size.height as f32; height = image_info.size.height as f32;
NinePatchBorderSource::Image(key, ImageRendering::Auto) NinePatchBorderSource::Image(key, ImageRendering::Auto)
}, },
Some(ResolvedImage::Gradient(gradient)) => { Ok(ResolvedImage::Gradient(gradient)) => {
match gradient::build(&self.fragment.style, gradient, border_image_size, builder) { match gradient::build(&self.fragment.style, gradient, border_image_size, builder) {
WebRenderGradient::Linear(gradient) => { WebRenderGradient::Linear(gradient) => {
NinePatchBorderSource::Gradient(gradient) NinePatchBorderSource::Gradient(gradient)

View file

@ -220,13 +220,13 @@ impl ReplacedContents {
image_url.clone().into(), image_url.clone().into(),
UsePlaceholder::No, UsePlaceholder::No,
) { ) {
Some(ImageOrMetadataAvailable::ImageAvailable { image, .. }) => { Ok(ImageOrMetadataAvailable::ImageAvailable { image, .. }) => {
(Some(image.clone()), image.width as f32, image.height as f32) (Some(image.clone()), image.width as f32, image.height as f32)
}, },
Some(ImageOrMetadataAvailable::MetadataAvailable(metadata, _id)) => { Ok(ImageOrMetadataAvailable::MetadataAvailable(metadata, _id)) => {
(None, metadata.width as f32, metadata.height as f32) (None, metadata.width as f32, metadata.height as f32)
}, },
None => return None, Err(_) => return None,
}; };
return Some(Self { return Some(Self {