Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Load a placeholder when a url to an image is broken. 
I decided to use the old Netscape broken image link icon (later we may
replace the image asset for something more trendier). The ref test will
expect that a failed load should display the rippy image.

ImageCacheTask users can define if a placeholder image should be loaded
at start up or not. This enables both the new behavior (e.g. always
return an image even for broken urls) as also the previous one.
  • Loading branch information
@Adenilson
Adenilson committed Mar 30, 2015
1 parent 5ead929 commit cdebb3c
Show file tree
Hide file tree
Showing 8 changed files with 77 additions and 32 deletions.
94 changes: 68 additions & 26 deletions components/net/image_cache_task.rs
View file
Expand Up @@ -15,6 +15,7 @@ use std::mem::replace;
use std::sync::{Arc, Mutex};
use std::sync::mpsc::{channel, Receiver, Sender};
use url::Url;
use util::resource_files::resources_dir_path;
use util::task::spawn_named;
use util::taskpool::TaskPool;

Expand Down Expand Up @@ -75,7 +76,8 @@ pub struct ImageCacheTask {

impl ImageCacheTask {
pub fn new(resource_task: ResourceTask, task_pool: TaskPool,
time_profiler_chan: time::ProfilerChan) -> ImageCacheTask {
time_profiler_chan: time::ProfilerChan,
load_placeholder: LoadPlaceholder) -> ImageCacheTask {
let (chan, port) = channel();
let chan_clone = chan.clone();

Expand All @@ -89,8 +91,9 @@ impl ImageCacheTask {
need_exit: None,
task_pool: task_pool,
time_profiler_chan: time_profiler_chan,
placeholder_data: Arc::new(vec!()),
};
cache.run();
cache.run(load_placeholder);
});

ImageCacheTask {
Expand All @@ -99,12 +102,13 @@ impl ImageCacheTask {
}

pub fn new_sync(resource_task: ResourceTask, task_pool: TaskPool,
time_profiler_chan: time::ProfilerChan) -> ImageCacheTask {
time_profiler_chan: time::ProfilerChan,
load_placeholder: LoadPlaceholder) -> ImageCacheTask {
let (chan, port) = channel();

spawn_named("ImageCacheTask (sync)".to_owned(), move || {
let inner_cache = ImageCacheTask::new(resource_task, task_pool,
time_profiler_chan);
time_profiler_chan, load_placeholder);

loop {
let msg: Msg = port.recv().unwrap();
Expand Down Expand Up @@ -142,6 +146,8 @@ struct ImageCache {
need_exit: Option<Sender<()>>,
task_pool: TaskPool,
time_profiler_chan: time::ProfilerChan,
// Default image used when loading fails.
placeholder_data: Arc<Vec<u8>>,
}

#[derive(Clone)]
Expand All @@ -160,8 +166,32 @@ enum AfterPrefetch {
DoNotDecode
}

pub enum LoadPlaceholder {
Preload,
Ignore
}

impl ImageCache {
pub fn run(&mut self) {
// Used to preload the default placeholder.
fn init(&mut self) {
let mut placeholder_url = resources_dir_path();
// TODO (Savago): replace for a prettier one.
placeholder_url.push("rippy.jpg");
let image = load_image_data(Url::from_file_path(&*placeholder_url).unwrap(), self.resource_task.clone(), &self.placeholder_data);

match image {
Err(..) => debug!("image_cache_task: failed loading the placeholder."),
Ok(image_data) => self.placeholder_data = Arc::new(image_data),
}
}

pub fn run(&mut self, load_placeholder: LoadPlaceholder) {
// We have to load the placeholder before running.
match load_placeholder {
LoadPlaceholder::Preload => self.init(),
LoadPlaceholder::Ignore => debug!("image_cache_task: use old behavior."),
}

let mut store_chan: Option<Sender<()>> = None;
let mut store_prefetched_chan: Option<Sender<()>> = None;

Expand Down Expand Up @@ -245,12 +275,12 @@ impl ImageCache {
let to_cache = self.chan.clone();
let resource_task = self.resource_task.clone();
let url_clone = url.clone();

let placeholder = self.placeholder_data.clone();
spawn_named("ImageCacheTask (prefetch)".to_owned(), move || {
let url = url_clone;
debug!("image_cache_task: started fetch for {}", url.serialize());

let image = load_image_data(url.clone(), resource_task.clone());
let image = load_image_data(url.clone(), resource_task.clone(), &placeholder);
to_cache.send(Msg::StorePrefetchedImageData(url.clone(), image)).unwrap();
debug!("image_cache_task: ended fetch for {}", url.serialize());
});
Expand Down Expand Up @@ -446,9 +476,9 @@ impl ImageCacheTask {
}
}

fn load_image_data(url: Url, resource_task: ResourceTask) -> Result<Vec<u8>, ()> {
fn load_image_data(url: Url, resource_task: ResourceTask, placeholder: &[u8]) -> Result<Vec<u8>, ()> {
let (response_chan, response_port) = channel();
resource_task.send(resource_task::ControlMsg::Load(LoadData::new(url, response_chan))).unwrap();
resource_task.send(resource_task::ControlMsg::Load(LoadData::new(url.clone(), response_chan))).unwrap();

let mut image_data = vec!();

Expand All @@ -462,7 +492,19 @@ fn load_image_data(url: Url, resource_task: ResourceTask) -> Result<Vec<u8>, ()>
return Ok(image_data);
}
Done(Err(..)) => {
return Err(());
// Failure to load the requested image will return the
// placeholder instead. In case it failed to load at init(),
// we still recover and return Err() but nothing will be drawn.
if placeholder.len() != 0 {
debug!("image_cache_task: failed to load {:?}, use placeholder instead.", url);
// Clean in case there was an error after started loading the image.
image_data.clear();
image_data.push_all(&placeholder);
return Ok(image_data);
} else {
debug!("image_cache_task: invalid placeholder.");
return Err(());
}
}
}
}
Expand Down Expand Up @@ -595,7 +637,7 @@ mod tests {
fn should_exit_on_request() {
let mock_resource_task = mock_resource_task(box DoesNothing);

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Ignore);

image_cache_task.exit();
mock_resource_task.send(resource_task::ControlMsg::Exit);
Expand All @@ -606,7 +648,7 @@ mod tests {
fn should_panic_if_unprefetched_image_is_requested() {
let mock_resource_task = mock_resource_task(box DoesNothing);

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Preload);
let url = Url::parse("file:///").unwrap();

let (chan, port) = channel();
Expand All @@ -620,7 +662,7 @@ mod tests {

let mock_resource_task = mock_resource_task(box JustSendOK { url_requested_chan: url_requested_chan});

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Preload);
let url = Url::parse("file:///").unwrap();

image_cache_task.send(Prefetch(url));
Expand All @@ -635,7 +677,7 @@ mod tests {

let mock_resource_task = mock_resource_task(box JustSendOK { url_requested_chan: url_requested_chan});

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Ignore);
let url = Url::parse("file:///").unwrap();

image_cache_task.send(Prefetch(url.clone()));
Expand All @@ -655,7 +697,7 @@ mod tests {

let mock_resource_task = mock_resource_task(box WaitSendTestImage{wait_port: wait_port});

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Ignore);
let url = Url::parse("file:///").unwrap();

image_cache_task.send(Prefetch(url.clone()));
Expand All @@ -672,7 +714,7 @@ mod tests {
fn should_return_decoded_image_data_if_data_has_arrived() {
let mock_resource_task = mock_resource_task(box SendTestImage);

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Preload);
let url = Url::parse("file:///").unwrap();

let join_port = image_cache_task.wait_for_store();
Expand All @@ -698,7 +740,7 @@ mod tests {
fn should_return_decoded_image_data_for_multiple_requests() {
let mock_resource_task = mock_resource_task(box SendTestImage);

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Preload);
let url = Url::parse("file:///").unwrap();

let join_port = image_cache_task.wait_for_store();
Expand Down Expand Up @@ -752,7 +794,7 @@ mod tests {
}
});

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Ignore);
let url = Url::parse("file:///").unwrap();

image_cache_task.send(Prefetch(url.clone()));
Expand Down Expand Up @@ -804,7 +846,7 @@ mod tests {
}
});

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Ignore);
let url = Url::parse("file:///").unwrap();

image_cache_task.send(Prefetch(url.clone()));
Expand Down Expand Up @@ -833,7 +875,7 @@ mod tests {
fn should_return_failed_if_image_bin_cannot_be_fetched() {
let mock_resource_task = mock_resource_task(box SendTestImageErr);

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Preload);
let url = Url::parse("file:///").unwrap();

let join_port = image_cache_task.wait_for_store_prefetched();
Expand All @@ -859,7 +901,7 @@ mod tests {
fn should_return_failed_for_multiple_get_image_requests_if_image_bin_cannot_be_fetched() {
let mock_resource_task = mock_resource_task(box SendTestImageErr);

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Preload);
let url = Url::parse("file:///").unwrap();

let join_port = image_cache_task.wait_for_store_prefetched();
Expand Down Expand Up @@ -893,7 +935,7 @@ mod tests {
fn should_return_failed_if_image_decode_fails() {
let mock_resource_task = mock_resource_task(box SendBogusImage);

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Preload);
let url = Url::parse("file:///").unwrap();

let join_port = image_cache_task.wait_for_store();
Expand Down Expand Up @@ -921,7 +963,7 @@ mod tests {
fn should_return_image_on_wait_if_image_is_already_loaded() {
let mock_resource_task = mock_resource_task(box SendTestImage);

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Preload);
let url = Url::parse("file:///").unwrap();

let join_port = image_cache_task.wait_for_store();
Expand Down Expand Up @@ -949,7 +991,7 @@ mod tests {

let mock_resource_task = mock_resource_task(box WaitSendTestImage {wait_port: wait_port});

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Ignore);
let url = Url::parse("file:///").unwrap();

image_cache_task.send(Prefetch(url.clone()));
Expand All @@ -975,7 +1017,7 @@ mod tests {

let mock_resource_task = mock_resource_task(box WaitSendTestImageErr{wait_port: wait_port});

let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Ignore);
let url = Url::parse("file:///").unwrap();

image_cache_task.send(Prefetch(url.clone()));
Expand All @@ -999,7 +1041,7 @@ mod tests {
fn sync_cache_should_wait_for_images() {
let mock_resource_task = mock_resource_task(box SendTestImage);

let image_cache_task = ImageCacheTask::new_sync(mock_resource_task.clone(), TaskPool::new(4), profiler());
let image_cache_task = ImageCacheTask::new_sync(mock_resource_task.clone(), TaskPool::new(4), profiler(), LoadPlaceholder::Preload);
let url = Url::parse("file:///").unwrap();

image_cache_task.send(Prefetch(url.clone()));
Expand Down
6 changes: 3 additions & 3 deletions components/servo/lib.rs
View file
Expand Up @@ -33,7 +33,7 @@ use msg::constellation_msg::ConstellationChan;
use script::dom::bindings::codegen::RegisterBindings;

#[cfg(not(test))]
use net::image_cache_task::ImageCacheTask;
use net::image_cache_task::{ImageCacheTask, LoadPlaceholder};
#[cfg(not(test))]
use net::resource_task::new_resource_task;
#[cfg(not(test))]
Expand Down Expand Up @@ -84,10 +84,10 @@ impl Browser {
// image.
let image_cache_task = if opts.output_file.is_some() {
ImageCacheTask::new_sync(resource_task.clone(), shared_task_pool,
time_profiler_chan.clone())
time_profiler_chan.clone(), LoadPlaceholder::Preload)
} else {
ImageCacheTask::new(resource_task.clone(), shared_task_pool,
time_profiler_chan.clone())
time_profiler_chan.clone(), LoadPlaceholder::Preload)
};

let font_cache_task = FontCacheTask::new(resource_task.clone());
Expand Down
6 changes: 3 additions & 3 deletions ports/gonk/src/lib.rs
View file
Expand Up @@ -38,7 +38,7 @@ use msg::constellation_msg::ConstellationChan;
use script::dom::bindings::codegen::RegisterBindings;

#[cfg(not(test))]
use net::image_cache_task::ImageCacheTask;
use net::image_cache_task::{ImageCacheTask, LoadPlaceholder};
#[cfg(not(test))]
use net::storage_task::StorageTaskFactory;
#[cfg(not(test))]
Expand Down Expand Up @@ -89,10 +89,10 @@ impl Browser {
// image.
let image_cache_task = if opts.output_file.is_some() {
ImageCacheTask::new_sync(resource_task.clone(), shared_task_pool,
time_profiler_chan.clone())
time_profiler_chan.clone(), LoadPlaceholder::Preload)
} else {
ImageCacheTask::new(resource_task.clone(), shared_task_pool,
time_profiler_chan.clone())
time_profiler_chan.clone(), LoadPlaceholder::Preload)
};
let font_cache_task = FontCacheTask::new(resource_task.clone());
let storage_task = StorageTaskFactory::new();
Expand Down
Binary file added resources/rippy.jpg
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions tests/ref/basic.list
View file
Expand Up @@ -193,6 +193,7 @@ flaky_cpu == linebreak_simple_a.html linebreak_simple_b.html
== multiple_css_class_a.html multiple_css_class_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
== 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
Expand Down
1 change: 1 addition & 0 deletions tests/ref/no-image-ref.html
View file
@@ -0,0 +1 @@
<img width="100px" height="100px" src="rippy.jpg">
1 change: 1 addition & 0 deletions tests/ref/no-image.html
View file
@@ -0,0 +1 @@
<img width="100px" height="100px" src="i-feel-a-disturbance-in-the-force.wtf">
Binary file added tests/ref/rippy.jpg
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

5 comments on commit cdebb3c

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw approval from jdm
at Adenilson@cdebb3c

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging Adenilson/servo/loadPlaceholder01 = cdebb3c into auto

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adenilson/servo/loadPlaceholder01 = cdebb3c merged ok, testing candidate = 1e282d5

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all tests pass:
success: http://build.servo.org/builders/linux1/builds/1218
success: http://build.servo.org/builders/linux2/builds/1216
success: http://build.servo.org/builders/android/builds/1252
success: http://build.servo.org/builders/mac1/builds/1431
success: http://build.servo.org/builders/mac2/builds/1434
success: http://build.servo.org/builders/gonk/builds/398

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fast-forwarding master to auto = 1e282d5

Please sign in to comment.