Auto merge of #5586 - pcwalton:no-broken-background-image-redux, r=glennw

r? @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5586)
<!-- Reviewable:end -->

components/layout/context.rs

@@ -16,7 +16,8 @@ use gfx::font_cache_task::FontCacheTask;
 use gfx::font_context::FontContext;
 use msg::constellation_msg::ConstellationChan;
 use net_traits::image::base::Image;
-use net_traits::image_cache_task::{ImageCacheChan, ImageCacheTask, ImageState};
+use net_traits::image_cache_task::{ImageCacheChan, ImageCacheTask, ImageResponse, ImageState};
+use net_traits::image_cache_task::{UsePlaceholder};
 use script::layout_interface::{Animation, LayoutChan, ReflowGoal};
 use std::boxed;
 use std::cell::Cell;
@@ -158,9 +159,11 @@ impl<'a> LayoutContext<'a> {
         }
     }
 
-    pub fn get_or_request_image(&self, url: Url) -> Option<Arc<Image>> {
+    pub fn get_or_request_image(&self, url: Url, use_placeholder: UsePlaceholder)
+                                -> Option<Arc<Image>> {
         // See if the image is already available
-        let result = self.shared.image_cache_task.get_image_if_available(url.clone());
+        let result = self.shared.image_cache_task.get_image_if_available(url.clone(),
+                                                                         use_placeholder);
 
         match result {
             Ok(image) => Some(image),
@@ -178,7 +181,11 @@ impl<'a> LayoutContext<'a> {
                         self.shared.image_cache_task.request_image(url,
                                                                    ImageCacheChan(sync_tx),
                                                                    None);
-                        sync_rx.recv().unwrap().image
+                        match sync_rx.recv().unwrap().image_response {
+                            ImageResponse::Loaded(image) |
+                            ImageResponse::PlaceholderLoaded(image) => Some(image),
+                            ImageResponse::None => None,
+                        }
                     }
                     // Not yet requested, async mode - request image from the cache
                     (ImageState::NotRequested, false) => {

components/layout/display_list_builder.rs

@@ -34,6 +34,7 @@ use gfx::paint_task::{PaintLayer, THREAD_TINT_COLORS};
 use msg::compositor_msg::{ScrollPolicy, LayerId};
 use msg::constellation_msg::ConstellationChan;
 use msg::constellation_msg::Msg as ConstellationMsg;
+use net_traits::image_cache_task::UsePlaceholder;
 use png::{self, PixelsByColorType};
 use std::cmp;
 use std::default::Default;
@@ -440,7 +441,7 @@ impl FragmentDisplayListBuilding for Fragment {
                                                clip: &ClippingRegion,
                                                image_url: &Url) {
         let background = style.get_background();
-        let image = layout_context.get_or_request_image(image_url.clone());
+        let image = layout_context.get_or_request_image(image_url.clone(), UsePlaceholder::No);
         if let Some(image) = image {
             debug!("(building display list) building background image");
 

components/layout/fragment.rs

@@ -27,6 +27,7 @@ use gfx::text::glyph::CharIndex;
 use gfx::text::text_run::{TextRun, TextRunSlice};
 use msg::constellation_msg::{ConstellationChan, Msg, PipelineId, SubpageId};
 use net_traits::image::base::Image;
+use net_traits::image_cache_task::UsePlaceholder;
 use rustc_serialize::{Encodable, Encoder};
 use script_traits::UntrustedNodeAddress;
 use std::borrow::ToOwned;
@@ -335,7 +336,9 @@ impl ImageFragmentInfo {
                    .map(Au::from_px)
         }
 
-        let image = url.and_then(|url| layout_context.get_or_request_image(url));
+        let image = url.and_then(|url| {
+            layout_context.get_or_request_image(url, UsePlaceholder::Yes)
+        });
 
         ImageFragmentInfo {
             replaced_image_fragment_info: ReplacedImageFragmentInfo::new(node,

components/net/image_cache_task.rs

@@ -4,7 +4,8 @@
 
 use collections::borrow::ToOwned;
 use net_traits::image::base::{Image, load_from_memory};
-use net_traits::image_cache_task::{ImageState, ImageCacheTask, ImageCacheChan, ImageCacheCommand, ImageCacheResult};
+use net_traits::image_cache_task::{ImageState, ImageCacheTask, ImageCacheChan, ImageCacheCommand};
+use net_traits::image_cache_task::{ImageCacheResult, ImageResponse, UsePlaceholder};
 use net_traits::load_whole_resource;
 use std::collections::HashMap;
 use std::collections::hash_map::Entry::{Occupied, Vacant};
@@ -53,13 +54,13 @@ impl PendingLoad {
 /// failure) are still stored here, so that they aren't
 /// fetched again.
 struct CompletedLoad {
-    image: Option<Arc<Image>>,
+    image_response: ImageResponse,
 }
 
 impl CompletedLoad {
-    fn new(image: Option<Arc<Image>>) -> CompletedLoad {
+    fn new(image_response: ImageResponse) -> CompletedLoad {
         CompletedLoad {
-            image: image,
+            image_response: image_response,
         }
     }
 }
@@ -79,11 +80,11 @@ impl ImageListener {
         }
     }
 
-    fn notify(self, image: Option<Arc<Image>>) {
+    fn notify(self, image_response: ImageResponse) {
         let ImageCacheChan(ref sender) = self.sender;
         let msg = ImageCacheResult {
             responder: self.responder,
-            image: image,
+            image_response: image_response,
         };
         sender.send(msg).ok();
     }
@@ -210,10 +211,19 @@ impl ImageCache {
             ImageCacheCommand::RequestImage(url, result_chan, responder) => {
                 self.request_image(url, result_chan, responder);
             }
-            ImageCacheCommand::GetImageIfAvailable(url, consumer) => {
+            ImageCacheCommand::GetImageIfAvailable(url, use_placeholder, consumer) => {
                 let result = match self.completed_loads.get(&url) {
                     Some(completed_load) => {
-                        completed_load.image.clone().ok_or(ImageState::LoadError)
+                        match (completed_load.image_response.clone(), use_placeholder) {
+                            (ImageResponse::Loaded(image), _) |
+                            (ImageResponse::PlaceholderLoaded(image), UsePlaceholder::Yes) => {
+                                Ok(image)
+                            }
+                            (ImageResponse::PlaceholderLoaded(_), UsePlaceholder::No) |
+                            (ImageResponse::None, _) => {
+                                Err(ImageState::LoadError)
+                            }
+                        }
                     }
                     None => {
                         let pending_load = self.pending_loads.get(&url);
@@ -255,8 +265,13 @@ impl ImageCache {
                         });
                     }
                     Err(_) => {
-                        let placeholder_image = self.placeholder_image.clone();
+                        match self.placeholder_image.clone() {
-                        self.complete_load(msg.url, placeholder_image);
+                            Some(placeholder_image) => {
+                                self.complete_load(msg.url, ImageResponse::PlaceholderLoaded(
+                                        placeholder_image))
+                            }
+                            None => self.complete_load(msg.url, ImageResponse::None),
+                        }
                     }
                 }
             }
@@ -265,31 +280,37 @@ impl ImageCache {
 
     // Handle a message from one of the decoder worker threads
     fn handle_decoder(&mut self, msg: DecoderMsg) {
-        let image = msg.image.map(Arc::new);
+        let image = match msg.image {
+            None => ImageResponse::None,
+            Some(image) => ImageResponse::Loaded(Arc::new(image)),
+        };
         self.complete_load(msg.url, image);
     }
 
     // Change state of a url from pending -> loaded.
-    fn complete_load(&mut self, url: Url, image: Option<Arc<Image>>) {
+    fn complete_load(&mut self, url: Url, image_response: ImageResponse) {
         let pending_load = self.pending_loads.remove(&url).unwrap();
 
-        let completed_load = CompletedLoad::new(image.clone());
+        let completed_load = CompletedLoad::new(image_response.clone());
         self.completed_loads.insert(url, completed_load);
 
         for listener in pending_load.listeners.into_iter() {
-            listener.notify(image.clone());
+            listener.notify(image_response.clone());
         }
     }
 
     // Request an image from the cache
-    fn request_image(&mut self, url: Url, result_chan: ImageCacheChan, responder: Option<Box<ImageResponder>>) {
+    fn request_image(&mut self,
+                     url: Url,
+                     result_chan: ImageCacheChan,
+                     responder: Option<Box<ImageResponder>>) {
         let image_listener = ImageListener::new(result_chan, responder);
 
         // Check if already completed
         match self.completed_loads.get(&url) {
             Some(completed_load) => {
                 // It's already completed, return a notify straight away
-                image_listener.notify(completed_load.image.clone());
+                image_listener.notify(completed_load.image_response.clone());
             }
             None => {
                 // Check if the load is already pending
@@ -366,3 +387,4 @@ pub fn new_image_cache_task(resource_task: ResourceTask) -> ImageCacheTask {
 
     ImageCacheTask::new(cmd_sender)
 }
+

components/net_traits/image_cache_task.rs

@@ -12,7 +12,7 @@ use std::sync::mpsc::{channel, Sender};
 /// image load completes. It is typically used to trigger a reflow
 /// and/or repaint.
 pub trait ImageResponder : Send {
-    fn respond(&self, Option<Arc<Image>>);
+    fn respond(&self, ImageResponse);
 }
 
 /// The current state of an image in the cache.
@@ -23,6 +23,17 @@ pub enum ImageState {
     NotRequested,
 }
 
+/// The returned image.
+#[derive(Clone)]
+pub enum ImageResponse {
+    /// The requested image was loaded.
+    Loaded(Arc<Image>),
+    /// The requested image failed to load, so a placeholder was loaded instead.
+    PlaceholderLoaded(Arc<Image>),
+    /// Neither the requested image nor the placeholder could be loaded.
+    None
+}
+
 /// Channel for sending commands to the image cache.
 #[derive(Clone)]
 pub struct ImageCacheChan(pub Sender<ImageCacheResult>);
@@ -31,7 +42,7 @@ pub struct ImageCacheChan(pub Sender<ImageCacheResult>);
 /// caller.
 pub struct ImageCacheResult {
     pub responder: Option<Box<ImageResponder>>,
-    pub image: Option<Arc<Image>>,
+    pub image_response: ImageResponse,
 }
 
 /// Commands that the image cache understands.
@@ -45,12 +56,18 @@ pub enum ImageCacheCommand {
     /// TODO(gw): Profile this on some real world sites and see
     /// if it's worth caching the results of this locally in each
     /// layout / paint task.
-    GetImageIfAvailable(Url, Sender<Result<Arc<Image>, ImageState>>),
+    GetImageIfAvailable(Url, UsePlaceholder, Sender<Result<Arc<Image>, ImageState>>),
 
     /// Clients must wait for a response before shutting down the ResourceTask
     Exit(Sender<()>),
 }
 
+#[derive(Copy, Clone, PartialEq)]
+pub enum UsePlaceholder {
+    No,
+    Yes,
+}
+
 /// The client side of the image cache task. This can be safely cloned
 /// and passed to different tasks.
 #[derive(Clone)]
@@ -78,9 +95,10 @@ impl ImageCacheTask {
     }
 
     /// Get the current state of an image. See ImageCacheCommand::GetImageIfAvailable.
-    pub fn get_image_if_available(&self, url: Url) -> Result<Arc<Image>, ImageState> {
+    pub fn get_image_if_available(&self, url: Url, use_placeholder: UsePlaceholder)
+                                  -> Result<Arc<Image>, ImageState> {
         let (sender, receiver) = channel();
-        let msg = ImageCacheCommand::GetImageIfAvailable(url, sender);
+        let msg = ImageCacheCommand::GetImageIfAvailable(url, use_placeholder, sender);
         self.chan.send(msg).unwrap();
         receiver.recv().unwrap()
     }
@@ -92,3 +110,4 @@ impl ImageCacheTask {
         response_port.recv().unwrap();
     }
 }
+

components/script/dom/canvasrenderingcontext2d.rs

@@ -33,14 +33,12 @@ use canvas_traits::{FillOrStrokeStyle, LinearGradientStyle, RadialGradientStyle}
 use canvas_traits::{LineCapStyle, LineJoinStyle, CompositionOrBlending};
 use canvas::canvas_paint_task::CanvasPaintTask;
 
-use net_traits::image::base::Image;
+use net_traits::image_cache_task::{ImageCacheChan, ImageResponse};
-use net_traits::image_cache_task::ImageCacheChan;
 use png::PixelsByColorType;
 
 use num::{Float, ToPrimitive};
 use std::borrow::ToOwned;
 use std::cell::RefCell;
-use std::sync::{Arc};
 use std::sync::mpsc::{channel, Sender};
 
 use util::str::DOMString;
@@ -260,8 +258,8 @@ impl CanvasRenderingContext2D {
         };
 
         let img = match self.request_image_from_cache(url) {
-            Some(img) => img,
+            ImageResponse::Loaded(img) => img,
-            None => return None,
+            ImageResponse::PlaceholderLoaded(_) | ImageResponse::None => return None,
         };
 
         let image_size = Size2D(img.width as f64, img.height as f64);
@@ -277,7 +275,7 @@ impl CanvasRenderingContext2D {
         return Some((image_data, image_size));
     }
 
-    fn request_image_from_cache(&self, url: Url) -> Option<Arc<Image>> {
+    fn request_image_from_cache(&self, url: Url) -> ImageResponse {
         let canvas = self.canvas.root();
         let window = window_from_node(canvas.r()).root();
         let window = window.r();
@@ -285,7 +283,7 @@ impl CanvasRenderingContext2D {
         let (response_chan, response_port) = channel();
         image_cache.request_image(url, ImageCacheChan(response_chan), None);
         let result = response_port.recv().unwrap();
-        result.image
+        result.image_response
     }
 
     fn create_drawable_rect(&self, x: f64, y: f64, w: f64, h: f64) -> Option<Rect<f32>> {

components/script/dom/htmlimageelement.rs

@@ -25,7 +25,7 @@ use util::str::DOMString;
 use string_cache::Atom;
 
 use net_traits::image::base::Image;
-use net_traits::image_cache_task::ImageResponder;
+use net_traits::image_cache_task::{ImageResponder, ImageResponse};
 use url::{Url, UrlParser};
 
 use std::borrow::ToOwned;
@@ -74,11 +74,16 @@ impl Responder {
 }
 
 impl ImageResponder for Responder {
-    fn respond(&self, image: Option<Arc<Image>>) {
+    fn respond(&self, image: ImageResponse) {
         // Update the image field
         let element = self.element.to_temporary().root();
         let element_ref = element.r();
-        *element_ref.image.borrow_mut() = image;
+        *element_ref.image.borrow_mut() = match image {
+            ImageResponse::Loaded(image) | ImageResponse::PlaceholderLoaded(image) => {
+                Some(image)
+            }
+            ImageResponse::None => None,
+        };
 
         // Mark the node dirty
         let node = NodeCast::from_ref(element.r());

components/script/script_task.rs

@@ -799,7 +799,7 @@ impl ScriptTask {
     }
 
     fn handle_msg_from_image_cache(&self, msg: ImageCacheResult) {
-        msg.responder.unwrap().respond(msg.image);
+        msg.responder.unwrap().respond(msg.image_response);
     }
 
     fn handle_webdriver_msg(&self, pipeline_id: PipelineId, msg: WebDriverScriptCommand) {

tests/ref/basic.list

@@ -207,6 +207,7 @@ flaky_cpu == linebreak_simple_a.html linebreak_simple_b.html
 == negative_margin_uncle_a.html negative_margin_uncle_b.html
 == negative_margins_a.html negative_margins_b.html
 == no-image.html no-image-ref.html
+== no_image_background_a.html no_image_background_ref.html
 == noscript.html noscript_ref.html
 != noteq_attr_exists_selector.html attr_exists_selector_ref.html
 == nth_child_pseudo_a.html nth_child_pseudo_b.html

tests/ref/no_image_background_a.html

@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body {
+    background: peachpuff;
+}
+section {
+    display: block;
+    width: 256px;
+    height: 256px;
+    border: solid black 1px;
+    background: url(bogusybogusbogus.jpg);
+}
+</style>
+</head>
+<body>
+<section></section>
+</body>
+</html>
+

tests/ref/no_image_background_ref.html

@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body {
+    background: peachpuff;
+}
+section {
+    display: block;
+    width: 256px;
+    height: 256px;
+    border: solid black 1px;
+}
+</style>
+</head>
+<body>
+<section></section>
+</body>
+</html>
+