Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upWIP : Using the entry settings object in CanvasRenderingContext2d::is_origin_clean #15887
Conversation
highfive
commented
Mar 9, 2017
highfive
commented
Mar 9, 2017
|
|
sendilkumarn
force-pushed the
sendilkumarn:canvas2d
branch
2 times, most recently
from
a691976
to
19da53e
| url.origin() == node.owner_doc().url().origin() | ||
| match image.GetCrossOrigin() { | ||
| None => { | ||
| match image.get_url() { |
This comment has been minimized.
This comment has been minimized.
wafflespeanut
Mar 14, 2017
Member
I think this could be better written with map and unwrap_or like,
image.get_url().map(|url| ...).unwrap_or(true)
This comment has been minimized.
This comment has been minimized.
|
Please make sure that this has a proper commit message in the end. r? @Ms2ger |
sendilkumarn
changed the title
|
r? @jdm I don't know the img code well enough to figure out how to match it to the spec. |
|
The HTMLImageElement and image cache implementations will require changes in order to properly support this change. I'll describe the necessary changes in a subsequent comment. |
| // the entry settings object, but for now check it against the canvas' doc. | ||
| let node: &Node = &*self.canvas.upcast(); | ||
| url.origin() == node.owner_doc().url().origin() | ||
| match image.GetCrossOrigin() { |
This comment has been minimized.
This comment has been minimized.
jdm
Mar 17, 2017
•
Member
Unfortunately this is not enough. We should add a origin method to HTMLImageElement that returns Option<Origin> and use that for the comparison, ignoring anything related to GetCrossOrigin in this particular code.
This comment has been minimized.
This comment has been minimized.
|
We have a couple missing pieces here:
The image cache receives the FetchMetadata value associated with the image response in ImageCache::handle_progress (in the ProcessResponse branch) but ignores it. We will need to extract the appropriate Metadata value from this and store the final_url value in the image cache. The ImageResponder API will need to be modified to support sending the url along with the ImageResponse value, and the code in HTMLImageElement that in interacts with this API (ImageResponseHandlerRunnable and process_image_response) will need to account for this. Once we have the URL, we can store it and return a meaningful value for the new |
|
@jdm I think this also changes few methods in windows.rs. Can mind to help on this ? |
|
@sendilkumarn Could you be more specific? What help are you looking for? |
|
I can fetch the response in ImageResponseHandlerRunnable and process_image_response i am not quite sure what has to be changed. |
|
process_image_response needs to store the origin of the final URL in the current image request, so that the new |
sendilkumarn
changed the title
|
@jdm I am clueless and missing a link here. |
|
|
|
Additionally, I have made a pull request that modifies the tests in |
bors-servo
added a commit
that referenced
this pull request
May 15, 2017
…frey,jgraham Make cross-origin canvas drawing tests use a same-origin redirect. These tests either pass or maintain existing known failures in Firefox and Chrome, and expose the problem in our current implementation that #15887 is addressing. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16699) <!-- Reviewable:end -->
|
@bors-servo: try |
bors-servo
added a commit
that referenced
this pull request
May 16, 2017
WIP : Using the entry settings object in CanvasRenderingContext2d::is_origin_clean <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15409 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15887) <!-- Reviewable:end -->
|
|
|
There are a lot of failures like this: |
|
Also, it is now possible to run |
cleanup cors-settings linting fixes moved to more SS fixing comments improved fixing comments adding servourl to net_traits metadataAvail and Loaded linting fixes fixing comments refactoring and adding image url fixing canvas2d method linting fixes
|
@jdm The test cases don't report any error [Edited] I will rebase the pull |
highfive
added
S-awaiting-review
and removed
S-tests-failed
S-needs-code-changes
labels
|
I looked into both problems and figured out the solutions. The "Image's origin is missing" panics happen because it turns out that we don't always send MetadataAvailable messages. In this case, the code to determine image width/height does not recognize BMP images, so the message was never sent and the image element never was provided with a final URL. The second problem was the canvas element tests continuing to fail with these changes. I discovered that #16699 had a mistake, and the diff --git a/components/layout/context.rs b/components/layout/context.rs
index d425388..3a4fac9 100644
--- a/components/layout/context.rs
+++ b/components/layout/context.rs
@@ -183,7 +183,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)
@@ -194,7 +194,7 @@ impl<'a> LayoutContext<'a> {
Some(image_info)
}
}
- None | Some(ImageOrMetadataAvailable::MetadataAvailable(_, _)) => None,
+ None | Some(ImageOrMetadataAvailable::MetadataAvailable(_)) => None,
}
}
}
diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs
index a896c30..52171dc 100644
--- a/components/layout/fragment.rs
+++ b/components/layout/fragment.rs
@@ -378,10 +378,10 @@ impl ImageFragmentInfo {
});
let (image, metadata) = match image_or_metadata {
- Some(ImageOrMetadataAvailable::ImageAvailable(i)) => {
+ Some(ImageOrMetadataAvailable::ImageAvailable(i, _)) => {
(Some(i.clone()), Some(ImageMetadata { height: i.height, width: i.width } ))
}
- Some(ImageOrMetadataAvailable::MetadataAvailable(m, _url)) => {
+ Some(ImageOrMetadataAvailable::MetadataAvailable(m)) => {
(None, Some(m))
}
None => {
diff --git a/components/net/image_cache.rs b/components/net/image_cache.rs
index f7b8c94..b8467e5 100644
--- a/components/net/image_cache.rs
+++ b/components/net/image_cache.rs
@@ -15,6 +15,7 @@ use std::collections::hash_map::Entry::{Occupied, Vacant};
use std::fs::File;
use std::io::{self, Read};
use std::mem;
+use std::path::PathBuf;
use std::sync::{Arc, Mutex};
use std::thread;
use webrender_traits;
@@ -51,10 +52,8 @@ fn decode_bytes_sync(key: LoadKey, bytes: &[u8]) -> DecoderMsg {
}
}
-fn get_placeholder_image(webrender_api: &webrender_traits::RenderApi) -> io::Result<Arc<Image>> {
- let mut placeholder_path = try!(resources_dir_path());
- placeholder_path.push("rippy.png");
- let mut file = try!(File::open(&placeholder_path));
+fn get_placeholder_image(webrender_api: &webrender_traits::RenderApi, path: &PathBuf) -> io::Result<Arc<Image>> {
+ let mut file = try!(File::open(path));
let mut image_data = vec![];
try!(file.read_to_end(&mut image_data));
let mut image = load_from_memory(&image_data).unwrap();
@@ -196,15 +195,13 @@ enum CacheResult<'a> {
struct CompletedLoad {
image_response: ImageResponse,
id: PendingImageId,
- final_url: ServoUrl,
}
impl CompletedLoad {
- fn new(image_response: ImageResponse, id: PendingImageId, final_url: ServoUrl) -> CompletedLoad {
+ fn new(image_response: ImageResponse, id: PendingImageId) -> CompletedLoad {
CompletedLoad {
image_response: image_response,
id: id,
- final_url: final_url,
}
}
}
@@ -325,6 +322,9 @@ struct ImageCacheStore {
// The placeholder image used when an image fails to load
placeholder_image: Option<Arc<Image>>,
+ // The URL used for the placeholder image
+ placeholder_url: ServoUrl,
+
// Webrender API instance.
webrender_api: webrender_traits::RenderApi,
}
@@ -361,14 +361,15 @@ impl ImageCacheStore {
LoadResult::PlaceholderLoaded(..) | LoadResult::None => {}
}
+ let url = pending_load.final_url.clone();
let image_response = match load_result {
- LoadResult::Loaded(image) => ImageResponse::Loaded(Arc::new(image)),
- LoadResult::PlaceholderLoaded(image) => ImageResponse::PlaceholderLoaded(image),
+ LoadResult::Loaded(image) => ImageResponse::Loaded(Arc::new(image), url.unwrap()),
+ LoadResult::PlaceholderLoaded(image) =>
+ ImageResponse::PlaceholderLoaded(image, self.placeholder_url.clone()),
LoadResult::None => ImageResponse::None,
};
- let url = pending_load.final_url.clone();
- let completed_load = CompletedLoad::new(image_response.clone(), key, url.unwrap());
+ let completed_load = CompletedLoad::new(image_response.clone(), key);
self.completed_loads.insert(pending_load.url.into(), completed_load);
for listener in pending_load.listeners {
@@ -384,13 +385,13 @@ impl ImageCacheStore {
-> Option<Result<ImageOrMetadataAvailable, ImageState>> {
self.completed_loads.get(url).map(|completed_load| {
match (&completed_load.image_response, placeholder) {
- (&ImageResponse::Loaded(ref image), _) |
- (&ImageResponse::PlaceholderLoaded(ref image), UsePlaceholder::Yes) => {
- Ok(ImageOrMetadataAvailable::ImageAvailable(image.clone()))
+ (&ImageResponse::Loaded(ref image, ref url), _) |
+ (&ImageResponse::PlaceholderLoaded(ref image, ref url), UsePlaceholder::Yes) => {
+ Ok(ImageOrMetadataAvailable::ImageAvailable(image.clone(), url.clone()))
}
- (&ImageResponse::PlaceholderLoaded(_), UsePlaceholder::No) |
+ (&ImageResponse::PlaceholderLoaded(_, _), UsePlaceholder::No) |
(&ImageResponse::None, _) |
- (&ImageResponse::MetadataLoaded(_, _), _) => {
+ (&ImageResponse::MetadataLoaded(_), _) => {
Err(ImageState::LoadError)
}
}
@@ -415,11 +416,16 @@ pub struct ImageCacheImpl {
impl ImageCache for ImageCacheImpl {
fn new(webrender_api: webrender_traits::RenderApi) -> ImageCacheImpl {
debug!("New image cache");
+
+ let mut placeholder_path = resources_dir_path().expect("Can't figure out resources path.");
+ placeholder_path.push("rippy.png");
+
ImageCacheImpl {
store: Arc::new(Mutex::new(ImageCacheStore {
pending_loads: AllPendingLoads::new(),
completed_loads: HashMap::new(),
- placeholder_image: get_placeholder_image(&webrender_api).ok(),
+ placeholder_image: get_placeholder_image(&webrender_api, &placeholder_path).ok(),
+ placeholder_url: ServoUrl::from_file_path(&placeholder_path).unwrap(),
webrender_api: webrender_api,
}))
}
@@ -450,8 +456,7 @@ impl ImageCache for ImageCacheImpl {
}
(&None, &Some(ref meta)) => {
debug!("Metadata available for {} ({:?})", url, key);
- return Ok(ImageOrMetadataAvailable::MetadataAvailable(meta.clone(),
- pl.final_url.clone().unwrap()))
+ return Ok(ImageOrMetadataAvailable::MetadataAvailable(meta.clone()))
}
(&Some(Err(_)), _) | (&None, &None) => {
debug!("{} ({:?}) is still pending", url, key);
@@ -486,7 +491,7 @@ impl ImageCache for ImageCacheImpl {
let mut store = self.store.lock().unwrap();
if let Some(load) = store.pending_loads.get_by_key_mut(&id) {
if let Some(ref metadata) = load.metadata {
- listener.respond(ImageResponse::MetadataLoaded(metadata.clone(), load.final_url.clone().unwrap()));
+ listener.respond(ImageResponse::MetadataLoaded(metadata.clone()));
}
load.add_listener(listener);
return;
@@ -531,8 +536,7 @@ impl ImageCache for ImageCacheImpl {
height: dimensions.height };
for listener in &pending_load.listeners {
listener.respond(
- ImageResponse::MetadataLoaded(img_metadata.clone(),
- pending_load.final_url.clone().unwrap()));
+ ImageResponse::MetadataLoaded(img_metadata.clone()));
}
pending_load.metadata = Some(img_metadata);
}
diff --git a/components/net_traits/image_cache.rs b/components/net_traits/image_cache.rs
index bdda678..99f8d57 100644
--- a/components/net_traits/image_cache.rs
+++ b/components/net_traits/image_cache.rs
@@ -25,8 +25,8 @@ pub enum CanRequestImages {
/// Indicating either entire image or just metadata availability
#[derive(Clone, Deserialize, Serialize, HeapSizeOf)]
pub enum ImageOrMetadataAvailable {
- ImageAvailable(Arc<Image>),
- MetadataAvailable(ImageMetadata, ServoUrl),
+ ImageAvailable(Arc<Image>, ServoUrl),
+ MetadataAvailable(ImageMetadata),
}
/// This is optionally passed to the image cache when requesting
@@ -63,11 +63,11 @@ impl ImageResponder {
#[derive(Clone, Deserialize, Serialize, HeapSizeOf)]
pub enum ImageResponse {
/// The requested image was loaded.
- Loaded(Arc<Image>),
+ Loaded(Arc<Image>, ServoUrl),
/// The request image metadata was loaded.
- MetadataLoaded(ImageMetadata, ServoUrl),
+ MetadataLoaded(ImageMetadata),
/// The requested image failed to load, so a placeholder was loaded instead.
- PlaceholderLoaded(Arc<Image>),
+ PlaceholderLoaded(Arc<Image>, ServoUrl),
/// Neither the requested image nor the placeholder could be loaded.
None,
}
diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs
index 88ef4cb..1936728 100644
--- a/components/script/dom/canvasrenderingcontext2d.rs
+++ b/components/script/dom/canvasrenderingcontext2d.rs
@@ -425,10 +425,10 @@ impl CanvasRenderingContext2D {
};
let img = match self.request_image_from_cache(url) {
- ImageResponse::Loaded(img) => img,
- ImageResponse::PlaceholderLoaded(_) |
+ ImageResponse::Loaded(img, _) => img,
+ ImageResponse::PlaceholderLoaded(_, _) |
ImageResponse::None |
- ImageResponse::MetadataLoaded(_, _) => {
+ ImageResponse::MetadataLoaded(_) => {
return None;
}
};
diff --git a/components/script/dom/htmlcanvaselement.rs b/components/script/dom/htmlcanvaselement.rs
index 6dd10aa..c8956c1 100644
--- a/components/script/dom/htmlcanvaselement.rs
+++ b/components/script/dom/htmlcanvaselement.rs
@@ -371,8 +371,8 @@ pub mod utils {
UsePlaceholder::No,
CanRequestImages::No);
match response {
- Ok(ImageOrMetadataAvailable::ImageAvailable(image)) =>
- ImageResponse::Loaded(image),
+ Ok(ImageOrMetadataAvailable::ImageAvailable(image, url)) =>
+ ImageResponse::Loaded(image, url),
_ => ImageResponse::None,
}
}
diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs
index 31d6686..e38b615 100644
--- a/components/script/dom/htmlimageelement.rs
+++ b/components/script/dom/htmlimageelement.rs
@@ -217,12 +217,12 @@ impl HTMLImageElement {
UsePlaceholder::Yes,
CanRequestImages::Yes);
match response {
- Ok(ImageOrMetadataAvailable::ImageAvailable(image)) => {
- self.process_image_response(ImageResponse::Loaded(image));
+ Ok(ImageOrMetadataAvailable::ImageAvailable(image, url)) => {
+ self.process_image_response(ImageResponse::Loaded(image, url));
}
- Ok(ImageOrMetadataAvailable::MetadataAvailable(m, img_url)) => {
- self.process_image_response(ImageResponse::MetadataLoaded(m, img_url));
+ Ok(ImageOrMetadataAvailable::MetadataAvailable(m)) => {
+ self.process_image_response(ImageResponse::MetadataLoaded(m));
}
Err(ImageState::Pending(id)) => {
@@ -275,14 +275,14 @@ impl HTMLImageElement {
fn process_image_response(&self, image: ImageResponse) {
let (image, metadata, trigger_image_load, trigger_image_error) = match image {
- ImageResponse::Loaded(image) | ImageResponse::PlaceholderLoaded(image) => {
+ ImageResponse::Loaded(image, url) | ImageResponse::PlaceholderLoaded(image, url) => {
+ self.current_request.borrow_mut().final_url = Some(url);
(Some(image.clone()),
Some(ImageMetadata { height: image.height, width: image.width }),
true,
false)
}
- ImageResponse::MetadataLoaded(meta, url) => {
- self.current_request.borrow_mut().final_url = Some(url);
+ ImageResponse::MetadataLoaded(meta) => {
(None, Some(meta), false, false)
}
ImageResponse::None => (None, None, false, true)
diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs
index 42bdd13..09f9972 100644
--- a/components/script/dom/webglrenderingcontext.rs
+++ b/components/script/dom/webglrenderingcontext.rs
@@ -465,9 +465,9 @@ impl WebGLRenderingContext {
let window = window_from_node(&*self.canvas);
let img = match canvas_utils::request_image_from_cache(&window, img_url) {
- ImageResponse::Loaded(img) => img,
- ImageResponse::PlaceholderLoaded(_) | ImageResponse::None |
- ImageResponse::MetadataLoaded(_, _)
+ ImageResponse::Loaded(img, _) => img,
+ ImageResponse::PlaceholderLoaded(_, _) | ImageResponse::None |
+ ImageResponse::MetadataLoaded(_)
=> return Err(()),
};
diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs
index 84d0b16..037892b 100644
--- a/components/script/dom/window.rs
+++ b/components/script/dom/window.rs
@@ -385,9 +385,9 @@ impl Window {
node.dirty(NodeDamage::OtherNodeDamage);
}
match response.response {
- ImageResponse::MetadataLoaded(_, _) => {}
- ImageResponse::Loaded(_) |
- ImageResponse::PlaceholderLoaded(_) |
+ ImageResponse::MetadataLoaded(_) => {}
+ ImageResponse::Loaded(_, _) |
+ ImageResponse::PlaceholderLoaded(_, _) |
ImageResponse::None => { nodes.remove(); }
}
self.add_pending_reflow();
diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json
index deefe04..191c2a3 100644
--- a/tests/wpt/metadata/MANIFEST.json
+++ b/tests/wpt/metadata/MANIFEST.json
@@ -399550,7 +399550,7 @@
"support"
],
"common/canvas-tests.js": [
- "2db347399bee84e76c01a15ca5c0c3006fcd4d4e",
+ "329e3ae1cfde2ee0525302e6a3260faed3885f28",
"support"
],
"common/css-red.txt": [
diff --git a/tests/wpt/web-platform-tests/common/canvas-tests.js b/tests/wpt/web-platform-tests/common/canvas-tests.js
index b41ba33..bf13d4a 100644
--- a/tests/wpt/web-platform-tests/common/canvas-tests.js
+++ b/tests/wpt/web-platform-tests/common/canvas-tests.js
@@ -101,4 +101,5 @@ function addCrossOriginRedirectYellowImage()
img.className = "resource";
img.src = get_host_info().HTTP_ORIGIN + "/common/redirect.py?location=" +
get_host_info().HTTP_REMOTE_ORIGIN + "/images/yellow.png";
+ document.body.appendChild(img);
}In particular, I moved the final URL out of MetadataAvailable and into the ImageAvailable/PlaceholderAvailable messages, which are always sent to the image cache consumers. |
|
Closing in favour of #16913. Thanks for working on this! |
bors-servo
added a commit
that referenced
this pull request
May 19, 2017
Make canvas origin clean checks use origin of image response Adapted from #15887. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15409 - [X] There are tests for these changes <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16913) <!-- Reviewable:end -->
bors-servo
added a commit
that referenced
this pull request
May 19, 2017
Make canvas origin clean checks use origin of image response Adapted from #15887. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15409 - [X] There are tests for these changes <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16913) <!-- Reviewable:end -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
sendilkumarn commentedMar 9, 2017
•
edited by larsbergstrom
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is