Implement update source set & select image source #21181

nupurbaghel · 2018-07-15T19:35:47Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11416 (github issue number if applicable).
These changes require tests but cannot be written until implementation of responsive images is complete

This change is

highfive · 2018-07-15T19:35:52Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmlimageelement.rs, components/script/dom/bindings/trace.rs
@KiChjang: components/script/dom/htmlimageelement.rs, components/script/dom/bindings/trace.rs
@emilio: components/style/values/specified/source_size_list.rs

highfive · 2018-07-15T19:35:54Z

Warning

These commits modify style and script code, but no tests are modified. Please consider adding a test!

nupurbaghel · 2018-07-15T19:37:28Z

r? @jdm
Step 5 of select_image_source will be added later. Also this PR includes changes made by #21178

emilio

Thanks for working on this! @jdm should probably review the source selection logic and such, I just skimmed through the style-related parts :)

emilio · 2018-07-16T08:45:58Z

components/script/dom/htmlimageelement.rs

+        );
+        let mut parserInput = ParserInput::new(&media_query);
+        let mut parser = Parser::new(&mut parserInput);
+        let condition = MediaCondition::parse(&context, &mut parser);


This should use MediaList instead of MediaCondition.

emilio · 2018-07-16T08:45:58Z

components/script/dom/htmlimageelement.rs

+            _ => false,
+        };
+
+        if *(&media_query.trim().is_empty()) || result {


And once it does that the is_empty check is not necessary, not the owned String.

Do you mean that MediaList checks for empty string condition too? Or that dereferencing is not needed ?

@jdm Does this mean that we have to evaluate the MediaList to generate the final bool result?

Yeah, an empty MediaList evaluates to true:

window.matchMedia("").matches <- true

emilio · 2018-07-16T08:45:58Z

components/style/values/specified/source_size_list.rs

 pub struct SourceSize {
-    condition: MediaCondition,
-    value: Length,
+    /// https://html.spec.whatwg.org/multipage/#source-size


There's no need to make these public, nor below. And why is PartialEq needed?

The normalising_source_densities requires to extract "value" from the SourceSize.
let length = &source_set.sourceSize.value.clone().unwrap();
Hence value has to be public. Also since we are using sourceSize from SourceSizeList above, we needed its members to be public.

PartialEq was added for unit tests as I remember, I can remove them from here if needed.

emilio · 2018-07-16T08:45:58Z

components/script/dom/htmlimageelement.rs

+
+                // Step 4.1.6
+                return ;
+            } else {


nit: I'd avoid the else, since given you return above it's not needed.

emilio · 2018-07-16T08:45:58Z

components/script/dom/htmlimageelement.rs

+                // Step 4.2
+                if !element.is::<HTMLSourceElement>() {
+                    continue;
+                } else {


Similarly, else after a continue statement is not needed.

emilio · 2018-07-16T08:45:58Z

components/script/dom/htmlimageelement.rs

+        };
+        let nodes = parent.unwrap().upcast::<Node>().ChildNodes();
+        let elements = match is_parent_picture {
+            true => nodes.iter().map(|n| DomRoot::from_ref(&*(*n).downcast::<Element>().unwrap())).collect(),


Is there any guarantee that the child nodes are Elements? I suspect you can end up with text nodes there easily, and then this will crash.

(So you should probably do filter_map(DomRoot::downcast::<Element>) instead).

Sure will update 👍

emilio · 2018-07-16T08:45:58Z

components/script/dom/htmlimageelement.rs

 enum ParseState {
    InDescriptor,
    InParens,
    AfterDescriptor,
 }

-#[derive(Debug, PartialEq)]
+pub struct SourceSet {
+    pub imageSources: Vec<ImageSource>,


let's use snake_case for the members, like: image_sources and source_size.

nupurbaghel · 2018-07-16T09:19:14Z

Thanks a lot for reviewing 😄 !

jdm

I did not have any difficulties matching this code against the algorithms in the specification. Great work!

jdm · 2018-07-17T01:55:23Z

components/script/dom/htmlimageelement.rs

-#[derive(Debug, PartialEq)]
+pub struct SourceSet {
+    pub image_sources: Vec<ImageSource>,
+    pub source_size: SourceSizeList,


Do these members need to be public?

Step 4.1.1 and 4.1.2 require setting of individual params of SourceSet. I think we should keep them public

Public only affects uses of the structure outside of the current module (ie. file). Since all uses are within the file, it should not need to be public.

jdm · 2018-07-17T01:55:23Z

components/script/dom/htmlimageelement.rs

+#[derive(Debug, PartialEq)]
+pub struct Size {
+    pub query: Option<MediaQuery>,
+    pub length: Length,


Do these members need to be public?

This can be avoided 👍 In fact, I think I can remove this struct totally

jdm · 2018-07-17T01:55:23Z

components/script/dom/htmlimageelement.rs

 }

 impl HTMLImageElement {
    pub fn get_url(&self) -> Option<ServoUrl> {
        self.current_request.borrow().parsed_url.clone()
    }
+
+    pub fn new_source_set() -> SourceSet {


It's more idiomatic to make this a static method named new on SourceSet itself.

jdm · 2018-07-17T01:55:23Z

components/script/dom/htmlimageelement.rs

+
+    pub fn new_source_set() -> SourceSet {
+        SourceSet {
+            image_sources: Vec::<ImageSource>::new(),


This can be vec![] or Vec::new(); no need to specify the type.

jdm · 2018-07-17T01:55:23Z

components/script/dom/htmlimageelement.rs

+            Some(xparent) => xparent.is::<HTMLPictureElement>(),
+            None => false
+        };
+        let nodes = parent.unwrap().upcast::<Node>().ChildNodes();


This looks like it would break if used on any image element that does not have a parent. Let's do this instead:

let elements = match parent { Some(p) => { let nodes = p.upcast::<Node>().ChildNodes(); nodes.iter().filter_map(DomRoot::downcast::<Element>) .map(|n| DomRoot::from_ref(&*n)).collect() } None => { vec![DomRoot::from_ref(&*elem)] } };

Keeping nodes inside the Some{ } gives "borrowed value does not live long enough" error. That is the reason it was kept outside before. 😅 Any other approach to solve this error ?

What specific part of the code is the error message referring to?

jdm · 2018-07-17T01:55:23Z

components/script/dom/htmlimageelement.rs

-    fn select_image_source(&self) -> Option<DOMString> {
-        // TODO: select an image source from source set
-        self.update_source_set().first().cloned()
+    fn select_image_source(&self) -> Option<ImageSource> {


Instead of an ImageSource, this should return Option<(DOMString, f32)> (or an Option of a new struct that contains those members).

jdm · 2018-07-17T01:55:23Z

components/script/dom/htmlimageelement.rs

+
+        // Step 2
+        if len == 0 {
+            return Some(ImageSource {


This should return None instead of Some.

jdm · 2018-07-17T01:55:23Z

components/script/dom/htmlimageelement.rs

+
+        // Step 4
+        let mut repeat_indices = HashSet::new();
+        for mut outer_index in 0..len {


No need for mut, right?

jdm · 2018-07-17T01:55:23Z

components/script/dom/htmlimageelement.rs

+            }
+            let imgsource = &source_set.image_sources[outer_index];
+            let pixel_density = imgsource.descriptor.den.unwrap();
+            for mut inner_index in (outer_index + 1)..len {


No need for mut, right?

jdm · 2018-07-17T01:55:23Z

components/style/values/specified/source_size_list.rs

+    /// https://html.spec.whatwg.org/multipage/#source-size-list
+    pub source_sizes: Vec<SourceSize>,
+    /// https://html.spec.whatwg.org/multipage/#source-size-list
+    pub value: Option<Length>,


To avoid making these fields public, perhaps we can add a method called set_fallback_value that mutates self.value.

nupurbaghel · 2018-07-17T17:59:12Z

Thank you for giving such a detailed review :)
We have made all the suggested changes 👍 And ready for the next review :P

jdm · 2018-07-17T18:04:51Z

This looks great! Please squash the commits together, and then we can merge this PR!

nupurbaghel · 2018-07-17T19:42:50Z

Thanks for the heads up!
I have altered Step 2 of update_source_set to check for HTMLPictureElement, which got missed in the last commit. Hope its right now 😄 !

jdm · 2018-07-17T20:25:29Z

Good catch!
@bors-servo r+

bors-servo · 2018-07-17T20:25:30Z

📌 Commit 536b869 has been approved by jdm

bors-servo · 2018-07-17T20:25:37Z

⌛ Testing commit 536b869 with merge b8e2dbc...


        Auto merge of #21181 - nupurbaghel:update_source_set, r=jdm

Implement update source set & select image source  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11416 (github issue number if applicable). - [x] These changes require tests but cannot be written until implementation of responsive images is complete    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21181)

bors-servo · 2018-07-17T20:45:52Z

💔 Test failed - linux-rel-css

nupurbaghel · 2018-07-18T19:47:17Z

I ran the update-wpt command. Many tests have started passing it seems :)

jdm · 2018-07-18T20:09:55Z

@bors-servo r+

bors-servo · 2018-07-18T20:09:55Z

📌 Commit 9d1af62 has been approved by jdm

bors-servo · 2018-07-18T20:18:32Z

⌛ Testing commit 9d1af62 with merge 4bbc2c4...


        Auto merge of #21181 - nupurbaghel:update_source_set, r=jdm

Implement update source set & select image source  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11416 (github issue number if applicable). - [x] These changes require tests but cannot be written until implementation of responsive images is complete    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21181)

bors-servo · 2018-07-18T20:38:59Z

💔 Test failed - linux-rel-css

jdm · 2018-07-18T20:44:53Z

 ▶ OK [expected TIMEOUT] /html/semantics/embedded-content/the-img-element/adoption.html

  ▶ Unexpected subtest result in /html/semantics/embedded-content/the-img-element/adoption.html:
  │ FAIL [expected TIMEOUT] img (srcset 1 cand)
  │   → assert_equals: expected "http://web-platform.test:8000/images/green-1x1.png" but got "/images/green-1x1.png"
  │ FAIL [expected TIMEOUT] img (srcset 1 cand), parent is picture
  │   → assert_equals: expected "http://web-platform.test:8000/images/green-1x1.png" but got "/images/green-1x1.png"
  │ FAIL [expected TIMEOUT] img (srcset 1 cand), previous sibling is source
  │   → assert_equals: expected "http://web-platform.test:8000/images/green-1x1.png" but got "/images/green-1x1.png"
  │ 
  │ t/</i.onload<@http://web-platform.test:8000/html/semantics/embedded-content/the-img-element/adoption.html:31:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
  └ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:1578:17

  ▶ Unexpected subtest result in /html/semantics/embedded-content/the-img-element/update-the-image-data/fail-to-resolve.html:
  └ PASS [expected FAIL] <img srcset="//[" src="/images/red.png">

jdm · 2018-07-18T20:46:21Z

I think we can mark all of those as expected results - we'll need to investigate the new failures adoption.html, but they look like a logic bug in the existing code, not something added by this PR.


        implement update_source_set, select_image_source

nupurbaghel · 2018-07-19T03:55:18Z

Okay updated other results as expected too. New failures in adoption.html will be investigated in another PR i guess 😅

jdm · 2018-07-19T12:18:08Z

@bors-servo r+

bors-servo · 2018-07-19T12:18:09Z

📌 Commit 8d06da8 has been approved by jdm

bors-servo · 2018-07-19T12:18:16Z

⌛ Testing commit 8d06da8 with merge e571873...


        Auto merge of #21181 - nupurbaghel:update_source_set, r=jdm

Implement update source set & select image source  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11416 (github issue number if applicable). - [x] These changes require tests but cannot be written until implementation of responsive images is complete    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21181)

bors-servo · 2018-07-19T13:38:16Z

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing e571873 to master...


        Bug 1477883: Cherry-pick style changes from servo/servo#21181. r=emilio

MozReview-Commit-ID: FpZ0YvXEYmP


        Bug 1477883: Cherry-pick style changes from servo/servo#21181. r=emilio

MozReview-Commit-ID: FpZ0YvXEYmP UltraBlame original commit: d62f2b15b2a4c7598a6c53d4c2da0e3e3d782789


        Bug 1477883: Cherry-pick style changes from servo/servo#21181. r=emilio

MozReview-Commit-ID: FpZ0YvXEYmP UltraBlame original commit: d62f2b15b2a4c7598a6c53d4c2da0e3e3d782789


        Bug 1477883: Cherry-pick style changes from servo/servo#21181. r=emilio

MozReview-Commit-ID: FpZ0YvXEYmP UltraBlame original commit: d62f2b15b2a4c7598a6c53d4c2da0e3e3d782789

highfive assigned gw3583 Jul 15, 2018

highfive added the S-awaiting-review label Jul 15, 2018

highfive assigned jdm and unassigned gw3583 Jul 15, 2018

emilio reviewed Jul 16, 2018

View changes

jdm requested changes Jul 17, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 17, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 17, 2018

jdm added S-needs-squash and removed S-awaiting-review labels Jul 17, 2018

jdm mentioned this pull request Jul 17, 2018

Reintroducing parse_a_sizes along with tests #21178

Closed

4 of 4 tasks complete

nupurbaghel force-pushed the nupurbaghel:update_source_set branch from 5ca8897 to 536b869 Jul 17, 2018

highfive added the S-awaiting-review label Jul 17, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Jul 17, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 18, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 18, 2018

implement update_source_set, select_image_source

Loading status checks…

8d06da8

nupurbaghel force-pushed the nupurbaghel:update_source_set branch from 9d1af62 to 8d06da8 Jul 19, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Jul 19, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 19, 2018

bors-servo merged commit 8d06da8 into servo:master Jul 19, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 19, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 24, 2018

Bug 1477883: Cherry-pick style changes from servo/servo#21181. r=emilio

0db62ce

MozReview-Commit-ID: FpZ0YvXEYmP

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019

Bug 1477883: Cherry-pick style changes from servo/servo#21181. r=emilio

8805a23

MozReview-Commit-ID: FpZ0YvXEYmP UltraBlame original commit: d62f2b15b2a4c7598a6c53d4c2da0e3e3d782789

servo / servo

Join GitHub today

Implement update source set & select image source #21181

Implement update source set & select image source #21181

Conversation

nupurbaghel commented Jul 15, 2018 • edited by SimonSapin

highfive commented Jul 15, 2018

highfive commented Jul 15, 2018

nupurbaghel commented Jul 15, 2018 • edited

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nupurbaghel commented Jul 16, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nupurbaghel Jul 17, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nupurbaghel commented Jul 17, 2018

jdm commented Jul 17, 2018

nupurbaghel commented Jul 17, 2018

jdm commented Jul 17, 2018

bors-servo commented Jul 17, 2018

bors-servo commented Jul 17, 2018

bors-servo commented Jul 17, 2018

nupurbaghel commented Jul 18, 2018

jdm commented Jul 18, 2018

bors-servo commented Jul 18, 2018

bors-servo commented Jul 18, 2018

bors-servo commented Jul 18, 2018

jdm commented Jul 18, 2018

jdm commented Jul 18, 2018

nupurbaghel commented Jul 19, 2018

jdm commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

nupurbaghel commented Jul 15, 2018 •

edited by SimonSapin

nupurbaghel commented Jul 15, 2018 •

edited

nupurbaghel Jul 17, 2018 •

edited