Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement HTMLImageElement decode #31269

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
+88 −173
Open

Implement HTMLImageElement decode #31269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ba35908
Implement HTMLImageElement decode
Taym95 Feb 6, 2024
dba5dad
Fix Decode doc link
Taym95 Feb 6, 2024
781fda5
Temp
Taym95 Feb 7, 2024
5daf5c5
Decode HTML Image
Taym95 Feb 29, 2024
ea11e15
Fix doc link
Taym95 Feb 29, 2024
445f368
Move image decode to process_image_response_for_environment_change
Taym95 Jun 2, 2024
1782dea
Update some wpt test result
Taym95 Jun 3, 2024
46b054a
Handle multiple image decode promises
Taym95 Jun 6, 2024
d15fbb5
Remove unnecessary promise calls
Taym95 Jun 7, 2024
aee6e84
Update more wpt test result
Taym95 Jun 7, 2024
aae04c6
Undo body-size-cross-origin.https.html.ini changes
Taym95 Jun 7, 2024
c117c01
Reject decode when src and srcset are missing
Taym95 Jun 9, 2024
3bf6a5b
Revert "Reject decode when src and srcset are missing"
Taym95 Jun 10, 2024
d6b7b70
Drain promises vec and run update_the_image_data when element is created
Taym95 Jun 10, 2024
1526af1
resolve decode promise in abort_request when request is CompletelyAva…
Taym95 Jun 10, 2024
ac82081
Update wpt test
Taym95 Jun 11, 2024
fb5b00a
Merge branch 'main' into Implement-HTMLImageElement-decode
Taym95 Jun 11, 2024
35aacbe
Move storing promise in decode task
Taym95 Jun 11, 2024
eee6096
Remove the resolve logic from decode task
Taym95 Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions components/script/dom/domexception.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub enum DOMErrorName {
TimeoutError = DOMExceptionConstants::TIMEOUT_ERR,
InvalidNodeTypeError = DOMExceptionConstants::INVALID_NODE_TYPE_ERR,
DataCloneError = DOMExceptionConstants::DATA_CLONE_ERR,
EncodingError,
NotReadableError,
OperationError,
}
Expand Down Expand Up @@ -70,6 +71,7 @@ impl DOMErrorName {
"TimeoutError" => Some(DOMErrorName::TimeoutError),
"InvalidNodeTypeError" => Some(DOMErrorName::InvalidNodeTypeError),
"DataCloneError" => Some(DOMErrorName::DataCloneError),
"EncodingError" => Some(DOMErrorName::EncodingError),
"NotReadableError" => Some(DOMErrorName::NotReadableError),
"OperationError" => Some(DOMErrorName::OperationError),
_ => None,
Expand Down Expand Up @@ -115,6 +117,9 @@ impl DOMException {
"The supplied node is incorrect or has an incorrect ancestor for this operation."
},
DOMErrorName::DataCloneError => "The object can not be cloned.",
DOMErrorName::EncodingError => {
"The encoding operation (either encoded or decoding) failed."
},
DOMErrorName::NotReadableError => "The I/O read operation failed.",
DOMErrorName::OperationError => {
"The operation failed for an operation-specific reason."
Expand Down
81 changes: 80 additions & 1 deletion components/script/dom/htmlimageelement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use std::cell::Cell;
use std::collections::HashSet;
use std::default::Default;
use std::rc::Rc;
use std::sync::{Arc, Mutex};
use std::{char, i32, mem};

Expand Down Expand Up @@ -47,6 +48,8 @@ use style::values::specified::AbsoluteLength;
use style_traits::ParsingMode;
use url::Url;

use super::domexception::DOMErrorName;
use super::types::DOMException;
use crate::document_loader::{LoadBlocker, LoadType};
use crate::dom::activation::Activatable;
use crate::dom::attr::Attr;
Expand Down Expand Up @@ -84,6 +87,7 @@ use crate::dom::node::{
UnbindContext,
};
use crate::dom::performanceresourcetiming::InitiatorType;
use crate::dom::promise::Promise;
use crate::dom::values::UNSIGNED_LONG_MAX;
use crate::dom::virtualmethods::VirtualMethods;
use crate::dom::window::Window;
Expand Down Expand Up @@ -169,6 +173,8 @@ pub struct HTMLImageElement {
#[ignore_malloc_size_of = "SourceSet"]
source_set: DomRefCell<SourceSet>,
last_selected_source: DomRefCell<Option<USVString>>,
#[ignore_malloc_size_of = "promises are hard"]
image_decode_promises: DomRefCell<Vec<Rc<Promise>>>,
}

impl HTMLImageElement {
Expand Down Expand Up @@ -424,6 +430,7 @@ impl HTMLImageElement {
LoadBlocker::terminate(&mut self.current_request.borrow_mut().blocker);
// Mark the node dirty
self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
self.resolve_image_decode_promises();
}

/// Step 24 of <https://html.spec.whatwg.org/multipage/#update-the-image-data>
Expand Down Expand Up @@ -525,6 +532,12 @@ impl HTMLImageElement {
request.state = state;
request.image = None;
request.metadata = None;

if matches!(state, State::Broken) {
self.reject_image_decode_promises();
} else if matches!(state, State::CompletelyAvailable) {
self.resolve_image_decode_promises();
}
}

/// <https://html.spec.whatwg.org/multipage/#update-the-source-set>
Expand Down Expand Up @@ -1159,6 +1172,39 @@ impl HTMLImageElement {
}
}

// Step 2 for <https://html.spec.whatwg.org/multipage/#dom-img-decode>
fn react_to_decode_image_sync_steps(&self, promise: Rc<Promise>) {
self.image_decode_promises
.borrow_mut()
.push(promise.clone());

let document = document_from_node(self);
// Step 2.1 of <https://html.spec.whatwg.org/multipage/#dom-img-decode>
if !document.is_fully_active() ||
matches!(self.current_request.borrow().state, State::Broken)
{
self.reject_image_decode_promises();
Copy link
Member

Choose a reason for hiding this comment

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

here, as per the comment below, let's only reject the promise associated with the task, and remove the resolve logic below. Instead, if the promise is not rejected, we should store it on pending_, and resolve/reject it later as the request state changes.

I'm going to run the wpt tests before this change, and after, just to see if this makes a difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the resolve logic but it broke lot of tests.

}
}

fn resolve_image_decode_promises(&self) {
for promise in self.image_decode_promises.borrow().iter() {
Taym95 marked this conversation as resolved.
Show resolved Hide resolved
promise.resolve_native(&());
}
self.image_decode_promises.borrow_mut().clear();
Copy link
Member

Choose a reason for hiding this comment

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

would it not be better to just do a drain(...) above?

Copy link
Contributor Author

@Taym95 Taym95 Jun 11, 2024 •
edited

Choose a reason for hiding this comment

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

We can use drain(..) from the doc I saw:

  /// // A full range clears the vector, like `clear()` does
 /// v.drain(..);
/// assert_eq!(v, &[]);

}

fn reject_image_decode_promises(&self) {
let document = document_from_node(self);
for promise in self.image_decode_promises.borrow().iter() {
Taym95 marked this conversation as resolved.
Show resolved Hide resolved
promise.reject_native(&DOMException::new(
&document.global(),
DOMErrorName::EncodingError,
));
}
self.image_decode_promises.borrow_mut().clear();
}

/// Step 15 for <https://html.spec.whatwg.org/multipage/#img-environment-changes>
fn finish_reacting_to_environment_change(
&self,
Expand Down Expand Up @@ -1247,6 +1293,7 @@ impl HTMLImageElement {
generation: Default::default(),
source_set: DomRefCell::new(SourceSet::new()),
last_selected_source: DomRefCell::new(None),
image_decode_promises: DomRefCell::new(vec![]),
}
}

Expand Down Expand Up @@ -1289,6 +1336,10 @@ impl HTMLImageElement {
image.SetHeight(h);
}

// run update_the_image_data when the element is created.
// https://html.spec.whatwg.org/multipage/#when-to-obtain-images
image.update_the_image_data();

Ok(image)
}
pub fn areas(&self) -> Option<Vec<DomRoot<HTMLAreaElement>>> {
Expand Down Expand Up @@ -1345,6 +1396,11 @@ pub enum ImageElementMicrotask {
elem: DomRoot<HTMLImageElement>,
generation: u32,
},
DecodeTask {
elem: DomRoot<HTMLImageElement>,
Copy link
Member

Choose a reason for hiding this comment

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

One last thing I thought about: there is currently a race condition between when the microtask runs, and the "in parallel" stuff described at 2.2. For example if the request becomes broken, we will reject all promises, including the one for which the micro task hasn't run yet.

I'm not sure this is actually a problem, but we may as well be consitent with the spec and:

  1. In Decode, not store the promise on pending_, but rather store it here in the task.
  2. change react_to_decode_image_sync_steps as described above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved storing promise in task and tests are okey

#[ignore_malloc_size_of = "promises are hard"]
promise: Rc<Promise>,
},
}

impl MicrotaskRunnable for ImageElementMicrotask {
Expand All @@ -1366,13 +1422,20 @@ impl MicrotaskRunnable for ImageElementMicrotask {
} => {
elem.react_to_environment_changes_sync_steps(*generation);
},
ImageElementMicrotask::DecodeTask {
Taym95 marked this conversation as resolved.
Show resolved Hide resolved
ref elem,
ref promise,
} => {
elem.react_to_decode_image_sync_steps(promise.clone());
},
}
}

fn enter_realm(&self) -> JSAutoRealm {
match self {
&ImageElementMicrotask::StableStateUpdateImageDataTask { ref elem, .. } |
&ImageElementMicrotask::EnvironmentChangesTask { ref elem, .. } => enter_realm(&**elem),
&ImageElementMicrotask::EnvironmentChangesTask { ref elem, .. } |
&ImageElementMicrotask::DecodeTask { ref elem, .. } => enter_realm(&**elem),
}
}
}
Expand Down Expand Up @@ -1605,6 +1668,22 @@ impl HTMLImageElementMethods for HTMLImageElement {
}
}

/// <https://html.spec.whatwg.org/multipage/#dom-img-decode>
fn Decode(&self) -> Rc<Promise> {
// Step 1
let promise = Promise::new(&self.global());

// Step 2
let task = ImageElementMicrotask::DecodeTask {
elem: DomRoot::from_ref(self),
promise: promise.clone(),
};
ScriptThread::await_stable_state(Microtask::ImageElement(task));

// Step 3
promise
}

// https://html.spec.whatwg.org/multipage/#dom-img-name
make_getter!(Name, "name");

Expand Down
3 changes: 3 additions & 0 deletions components/script/dom/webidls/HTMLImageElement.webidl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ interface HTMLImageElement : HTMLElement {
readonly attribute USVString currentSrc;
[CEReactions]
attribute DOMString referrerPolicy;

Promise<undefined> decode();

// also has obsolete members
};

Expand Down
6 changes: 0 additions & 6 deletions tests/wpt/meta-legacy-layout/html/dom/idlharness.https.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3558,9 +3558,6 @@
[HTMLFrameElement interface: attribute frameBorder]
expected: FAIL

[HTMLImageElement interface: new Image() must inherit property "decode()" with the proper type]
expected: FAIL

[HTMLTableColElement interface: document.createElement("col") must inherit property "width" with the proper type]
expected: FAIL

Expand Down Expand Up @@ -4248,9 +4245,6 @@
[HTMLInputElement interface: createInput("hidden") must inherit property "autocomplete" with the proper type]
expected: FAIL

[HTMLImageElement interface: document.createElement("img") must inherit property "decode()" with the proper type]
expected: FAIL

[HTMLAreaElement interface: document.createElement("area") must inherit property "username" with the proper type]
expected: FAIL

Expand Down
3 changes: 0 additions & 3 deletions ...ml/semantics/embedded-content/the-img-element/decode/image-decode-image-document.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,2 @@
[image-decode-image-document.html]
expected: ERROR
[HTMLImageElement.prototype.decode(), image document tests. Decode from iframe with image document, succeeds (img not loaded)]
expected: TIMEOUT

15 changes: 0 additions & 15 deletions ...html/semantics/embedded-content/the-img-element/decode/image-decode-path-changes.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,11 @@
[image-decode-path-changes.html]
type: testharness
[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes fail decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes fail decode; following good png decode succeeds.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes fail decode; following good svg decode succeeds.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes fail decode; following bad decode fails.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes to the same path succeed.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. srcset changes fail decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. srcset changes fail decode; following good decode succeeds.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. srcset changes fail decode; following bad decode fails.]
expected: FAIL

21 changes: 0 additions & 21 deletions ...yout/html/semantics/embedded-content/the-img-element/decode/image-decode-picture.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,5 @@
[image-decode-picture.html]
type: testharness
[HTMLImageElement.prototype.decode(), picture tests. Image with PNG source decodes with undefined.]
expected: FAIL

[HTMLImageElement.prototype.decode(), picture tests. Image with multiple sources decodes with undefined.]
expected: FAIL

[HTMLImageElement.prototype.decode(), picture tests. Image with PNG data URL source decodes with undefined.]
expected: FAIL

[HTMLImageElement.prototype.decode(), picture tests. Image with SVG source decodes with undefined.]
expected: FAIL

[HTMLImageElement.prototype.decode(), picture tests. Non-existent source fails decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), picture tests. Corrupt image in src fails decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), picture tests. Image without srcset fails decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), picture tests. Multiple decodes for images with src succeed.]
expected: FAIL

2 changes: 0 additions & 2 deletions ...semantics/embedded-content/the-img-element/decode/image-decode-with-quick-attach.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,2 @@
[image-decode-with-quick-attach.html]
[HTMLImageElement.prototype.decode(), attach to DOM before promise resolves.]
expected: FAIL

36 changes: 0 additions & 36 deletions ...egacy-layout/html/semantics/embedded-content/the-img-element/decode/image-decode.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,8 @@
[image-decode.html]
type: testharness
[HTMLImageElement.prototype.decode(), basic tests. Image with PNG src decodes with undefined.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Image with PNG data URL src decodes with undefined.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Image with SVG src decodes with undefined.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Non-existent src fails decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Inactive document fails decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Adopted active image into inactive document fails decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Adopted inactive image into active document succeeds.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Corrupt image in src fails decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Image without src/srcset fails decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Multiple decodes for images with src succeed.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Image with PNG srcset decodes with undefined.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Image with SVG srcset decodes with undefined.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Non-existent srcset fails decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), basic tests. Multiple decodes for images with srcset succeed.]
expected: FAIL

3 changes: 0 additions & 3 deletions tests/wpt/meta-legacy-layout/resource-timing/body-size-cross-origin.https.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
[Retrieving a same-origin resource with Timing-Allow-Origin should expose body size]
expected: FAIL

[Retrieving a no-cors resource without Timing-Allow-Origin should not expose body size]
expected: FAIL

[Retrieving a no-cors resource with Timing-Allow-Origin should not expose body size]
expected: FAIL

Expand Down
6 changes: 0 additions & 6 deletions tests/wpt/meta/html/dom/idlharness.https.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3381,9 +3381,6 @@
[HTMLFrameElement interface: attribute frameBorder]
expected: FAIL
[HTMLImageElement interface: new Image() must inherit property "decode()" with the proper type]
expected: FAIL
[HTMLTableColElement interface: document.createElement("col") must inherit property "width" with the proper type]
expected: FAIL
Expand Down Expand Up @@ -4044,9 +4041,6 @@
[HTMLTableColElement interface: document.createElement("colgroup") must inherit property "chOff" with the proper type]
expected: FAIL
[HTMLImageElement interface: document.createElement("img") must inherit property "decode()" with the proper type]
expected: FAIL
[HTMLAreaElement interface: document.createElement("area") must inherit property "username" with the proper type]
expected: FAIL
Expand Down
3 changes: 0 additions & 3 deletions ...ml/semantics/embedded-content/the-img-element/decode/image-decode-image-document.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1 @@
[image-decode-image-document.html]
expected: ERROR
[HTMLImageElement.prototype.decode(), image document tests. Decode from iframe with image document, succeeds (img not loaded)]
expected: TIMEOUT
15 changes: 0 additions & 15 deletions ...html/semantics/embedded-content/the-img-element/decode/image-decode-path-changes.html.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,9 @@
[image-decode-path-changes.html]
[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes fail decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes fail decode; following good png decode succeeds.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes fail decode; following good svg decode succeeds.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes fail decode; following bad decode fails.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. src changes to the same path succeed.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. srcset changes fail decode.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. srcset changes fail decode; following good decode succeeds.]
expected: FAIL

[HTMLImageElement.prototype.decode(), src/srcset mutation tests. srcset changes fail decode; following bad decode fails.]
expected: FAIL