Parse Srcset Attribute #18313

iamneha · 2017-08-30T12:44:59Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-08-30T12:45:06Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @cbrewster (or someone else) soon.

highfive · 2017-08-30T12:45:06Z

Heads up! This PR modifies the following files:

@bholley: components/style/str.rs
@canaltinova: components/style/str.rs
@KiChjang: components/script/test.rs, components/script/dom/webidls/HTMLImageElement.webidl, components/script/dom/htmlimageelement.rs
@fitzgen: components/script/test.rs, components/script/dom/webidls/HTMLImageElement.webidl, components/script/dom/htmlimageelement.rs
@emilio: components/style/str.rs

wafflespeanut

Just had a quick look. This looks good so far. Nice work! But, this still requires a lot of tidying up. Please fix the build warnings and run ./mach test-tidy to help cleanup the code.

wafflespeanut · 2017-08-31T14:51:57Z

components/script/dom/htmlimageelement.rs

+        let(spaces, position) = collect_sequence_characters(position, |c| *c ==',' || char::is_whitespace(*c));
+        let x = spaces.find(',');
+        match x {
+            Some(val) => println!("Parse Error"),


There shouldn't be any println! unless we really need it somewhere. Ideally, this should be ignoring parse errors completely and return only the values that have been parsed successfully (if any). Otherwise, it should simply be an empty vector.

Currently being used solely for debugging purpose and will be removed when PR is finalized

wafflespeanut · 2017-08-31T14:51:57Z

components/script/dom/htmlimageelement.rs

+                }
+            }
+        }
+        let mut error = Error::No;


This could simply be a bool, since there are only two possible values, and we can use bools easily in if conditionals.

wafflespeanut · 2017-08-31T14:51:57Z

components/script/dom/htmlimageelement.rs

+            let mut char_iter = descriptor.chars();
+            let (digits, remaining) = collect_sequence_characters(&descriptor, is_ascii_digit);
+            let valid_non_negative_integer = parse_unsigned_integer(digits.chars());
+            let has_w = if let "w" = remaining {


Instead of having a complicated pattern match and a variable binding, we should use remaining == "w" in the if conditionals below.

wafflespeanut · 2017-08-31T14:51:57Z

components/script/dom/htmlimageelement.rs

+            };
+            if valid_non_negative_integer.is_ok() && has_w {
+                //not support sizes attribute
+                if width != None && density != None {


Nit: Let's use is_some() and is_none() here and everywhere (appropriately). Note that there are plenty of useful methods for Option<T> and Result<T, E>

wafflespeanut · 2017-08-31T14:51:57Z

components/script/dom/htmlimageelement.rs

+                    error = Error::Yes;
+                }
+                let result = parse_unsigned_integer(char_iter.clone());
+                if result.is_err() {


If error becomes a bool, then we could write,

error = result.is_err(); if let Ok(w) = result { width = Some(w); }

wafflespeanut · 2017-08-31T14:51:57Z

components/script/dom/htmlimageelement.rs

+        }
+
+        // add the counts of spaces that we collect to advance the start index
+        let space_len = spaces.char_indices().count();


We should go for chars only if we're concerned with unicode codepoints. If it's just spaces, then we could simply use spaces.len()

wafflespeanut · 2017-08-31T14:51:57Z

components/script/dom/htmlimageelement.rs

+        let(url, spaces) = collect_sequence_characters(position, |c| !char::is_whitespace(*c));
+
+        // add the counts of urls that we parse to advance the start index
+        start += url.char_indices().count();


Why should we use char_indices instead of chars here? (and below?). We should prefer the latter, unless we're using the indices during the iteration.

wafflespeanut · 2017-08-31T14:51:57Z

components/script/dom/htmlimageelement.rs

+                            continue;
+                        }
+                        Some((idx, c @ ',')) => {
+                            position.chars().enumerate();


What's this?

This should be removed, since it does not do anything.

wafflespeanut · 2017-08-31T14:54:40Z

Also, please leave steps from spec in comments, so that it's easier to address/change parts of code later.

wafflespeanut · 2017-08-31T15:08:45Z

components/script/dom/htmlimageelement.rs

+            } else {
+                false
+            };
+            if valid_non_negative_integer.is_ok() && has_w {


This should be !has_w, or more simply, we should check has_w && width.is_some() first (and error if it's true) before checking the non-negative integer thingy.

During descriptor parsing, we ensure that there are no repetitions in descriptors, which means there shouldn't be any usage of w, x or h more than once - i.e., if the value ends with h and we already have a height value in the descriptor, then it's a parse error.

wafflespeanut · 2017-09-04T13:33:46Z

Step 13 in the spec looks funny.

If the descriptor consists of a valid non-negative integer followed by a U+0068 LATIN SMALL LETTER H character

This is a parse error.

If it's a parse error, then there's no reason to parse it. The value for future-compat-h is not being used after parsing (probably because it's planned for the future?), but that shouldn't mean that the entire descriptor should be rejected with a parse error. We should probably skip the value and still use the descriptor?

/cc @jdm

KiChjang · 2017-09-05T04:31:34Z

Sorry, but which issue does this PR solve?

iamneha · 2017-09-05T05:52:43Z

@KiChjang we are solving srcset attribute from issue Support responsive images #11416

creativcoder · 2017-09-05T09:48:53Z

components/script/dom/htmlimageelement.rs

+                false
+            };
+            let valid_floating_point = parse_double(digits);
+            let has_x = if remaining == "x" {


we can just do this: let has_x = remaining == "x";

creativcoder · 2017-09-05T09:49:16Z

components/script/dom/htmlimageelement.rs

+            } else {
+                false
+            };
+            let has_h = if remaining == "h" {


Same as above

creativcoder · 2017-09-05T09:50:42Z

components/script/dom/htmlimageelement.rs

+            };
+            if valid_non_negative_integer.is_ok() && has_w {
+                //not support sizes attribute
+                if width.is_some() && density.is_some() {


you can do this error = width.is_some() && density.is_some();

creativcoder · 2017-09-05T09:57:07Z

components/script/dom/htmlimageelement.rs

+                let result = parse_unsigned_integer(char_iter.clone());
+                error = result.is_err();
+                if let Ok(w) = result {
+                    width = Some(w);


As we are just pulling the value out and converting it to option type (i.e, Some(w)), we can use the ok method like: width = result.ok(). Calling ok() on a Result<T> type converts it to an Option<T>. ok() method does what you are currently doing here.

creativcoder · 2017-09-05T09:59:31Z

components/script/dom/htmlimageelement.rs

+        if future_compat_h.is_some() && width.is_none() {
+            error = true;
+        }
+        if error == false {


As error now is just a boolean we can reduce the conditional check to just this:

if !error { // stuff }

creativcoder · 2017-09-05T10:00:55Z

components/script/dom/htmlimageelement.rs

+            let char_iter = descriptor.chars();
+            let (digits, remaining) = collect_sequence_characters(&descriptor, is_ascii_digit);
+            let valid_non_negative_integer = parse_unsigned_integer(digits.chars());
+            let has_w = if remaining == "w" {


we can just do this: let has_w = remaining == "w";

creativcoder · 2017-09-05T10:07:53Z

components/script/dom/webidls/HTMLImageElement.webidl

@@ -10,7 +10,7 @@ interface HTMLImageElement : HTMLElement {
  [CEReactions]
           attribute DOMString src;
  // [CEReactions]
-  //         attribute DOMString srcset;


Nit: Please remove this commented line

creativcoder · 2017-09-05T10:09:07Z

components/script/dom/htmlimageelement.rs

+        let mut current_descriptor = String::new();
+        let mut state = ParseState::InDescriptor;
+        let mut char_stream = position.chars().enumerate();
+        //let mut last_index = 0;


Nit: Please remove the commented line

creativcoder · 2017-09-05T13:08:51Z

components/script/dom/htmlimageelement.rs

+            error = true;
+        }
+        if error == false {
+            if width.is_some() || density.is_some() {


Step 15 of spec is written in a misleading way. Due to this check (if width.is_some() || density.is_some() {), the case of a single srcset string without any descriptor/density : ( i.e., small_image.jpg), does not parses correctly and returns an empty vec. This is a valid srcset string as confirmed by neha.

The spec does not mean that we should append ImageSource only if width and density is present.
Rather it should just check if error is false and just append whatever is there in the width and density variable along with url.

So basically this :

if !error { // append stuff to candidates } else { println!("parsed error"); }

KiChjang · 2017-09-05T19:27:25Z

Merge commits are not accepted. Please remove them and rebase against master instead.

KiChjang

Made a preliminary review, I have not gotten too far into the spec before I started finding problems.

It would also be super helpful if you can add step annotations inside your function body, this will leave a good note of which part of the spec that the code is attempting to follow.

KiChjang · 2017-09-05T21:43:09Z

components/script/dom/htmlimageelement.rs

+
+pub fn collect_sequence_characters<F>(s: &str, predicate: F) -> (&str, &str)
+        where F: Fn(&char) -> bool
+        {


nit: weird indentation

KiChjang · 2017-09-05T21:43:09Z

components/script/dom/htmlimageelement.rs

+    let mut candidates: Vec<ImageSource> = Vec::new();
+    while start < input.len() {
+        let position = &input[start..];
+        let(spaces, position) = collect_sequence_characters(position, |c| *c ==',' || char::is_whitespace(*c));


nit: space after let.

Also, ignoring the point where you're supposed to throw a parse error when you encounter a comma, is this essentially position.filter(char::is_whitespace).collect()?

KiChjang · 2017-09-05T21:43:09Z

components/script/dom/htmlimageelement.rs

+        return (s, "");
+        }
+
+pub fn parse_a_srcset_attribute(input: String) -> Vec<ImageSource> {


Is there a reason why you need a String here instead of a &str?

Also, since this function can return an error, the return type should be Fallible<Vec<ImageSource>>.

Should also link to https://html.spec.whatwg.org/multipage/#parsing-a-srcset-attribute.

KiChjang · 2017-09-05T21:43:09Z

components/script/dom/htmlimageelement.rs

+
+pub fn parse_a_srcset_attribute(input: String) -> Vec<ImageSource> {
+    let mut start = 0;
+    let mut candidates: Vec<ImageSource> = Vec::new();


Not important, but vec![] is equivalent to Vec::new().

KiChjang · 2017-09-05T21:43:09Z

components/script/dom/htmlimageelement.rs

+    pub den: Option<f64>,
+}
+
+pub fn collect_sequence_characters<F>(s: &str, predicate: F) -> (&str, &str)


Link to https://infra.spec.whatwg.org/#collect-a-sequence-of-code-points.

I'm also doubtful whether this function belongs here. It sounds like script/dom/bindings/str.rs is a better place.

KiChjang · 2017-09-05T21:43:09Z

components/script/dom/htmlimageelement.rs

+        {
+            for (i, ch) in s.chars().enumerate() {
+                if !predicate(&ch) {
+                    return (&s[0..i], &s[i..])


This does not look correct; the spec says:

While position doesn’t point past the end of input and the code point at position within input meets the condition condition:

Append that code point to the end of result.

Advance position by 1.

In other words, whenever the condition (in the code it is named predicate instead) is fulfilled, the character that is fulfilling the condition must be appended to result.

Currently, your code would return immediately when it discovers the first character that returns false from invoking predicate, causing the remaining characters to not be parsed according to the spec.

KiChjang · 2017-09-05T21:43:09Z

components/script/dom/htmlimageelement.rs

+        let(spaces, position) = collect_sequence_characters(position, |c| *c ==',' || char::is_whitespace(*c));
+        let x = spaces.find(',');
+        match x {
+            Some(val) => println!("Parse Error"),


As previously stated, you should return an Error here instead of println!.

As previously discussed with Neha, the spec does not actually want us to abort parsing when a parse error is encountered.

KiChjang · 2017-09-05T21:43:09Z

components/script/dom/webidls/HTMLImageElement.webidl

@@ -9,8 +9,8 @@ interface HTMLImageElement : HTMLElement {
           attribute DOMString alt;
  [CEReactions]
           attribute DOMString src;
-  // [CEReactions]
-  //         attribute DOMString srcset;
+  [CEReactions]


In order to generate appropriate bindings for the return type, you'll need to add the [Throws] attribute here.

KiChjang · 2017-09-05T21:43:09Z

components/script/test.rs

@@ -10,6 +10,7 @@ pub use dom::bindings::cell::DOMRefCell;
 pub use dom::bindings::js::JS;
 pub use dom::node::Node;
 pub use dom::bindings::refcounted::TrustedPromise;
+pub use std::string::String;


This should not be necessary.

Let's remove this.

KiChjang · 2017-09-05T21:43:09Z

tests/unit/script/htmlimageelement.rs

@@ -0,0 +1,72 @@
+use script::test::String;


This should not be necessary.

Let's remove this.

wafflespeanut

Going good so far. Nice work! A few more steps towards merging. I think some of my comments should be thrown as warnings by rustc. We should get rid of the warnings just as we do the errors.

wafflespeanut · 2017-09-24T09:10:58Z

components/script/dom/htmlimageelement.rs

+    InParens,
+    AfterDescriptor,
+}
+#[derive(Debug)]


Nit: missing newline

wafflespeanut · 2017-09-24T09:10:58Z

components/script/dom/htmlimageelement.rs

+    pub url: String,
+    pub descriptor: Descriptor,
+}
+#[derive(PartialEq)]


Nit: newline

wafflespeanut · 2017-09-24T09:10:58Z

components/script/dom/htmlimageelement.rs

+    pub wid: Option<u32>,
+    pub den: Option<f64>,
+}
+#[derive(Clone, Copy, HeapSizeOf, JSTraceable)]


Nit: newline

wafflespeanut · 2017-09-24T09:10:58Z

components/script/dom/htmlimageelement.rs

@@ -978,3 +1004,177 @@ fn image_dimension_setter(element: &Element, attr: LocalName, value: u32) {
    let value = AttrValue::Dimension(value.to_string(), dim);
    element.set_attribute(&attr, value);
 }
+
+pub fn collect_sequence_characters<F>(s: &str, predicate: F) -> (&str, &str)
+        where F: Fn(&char) -> bool


Nit: Improper indentation (strip out spaces for this and the following lines)

wafflespeanut · 2017-09-24T09:10:58Z

components/script/dom/htmlimageelement.rs

+    let mut candidates: Vec<ImageSource> = vec![];
+    while start < input.len() {
+        let position = &input[start..];
+        let (spaces, position) = collect_sequence_characters(position, |c| *c ==',' || char::is_whitespace(*c));


Nit: missing space (should be *c == ',')

wafflespeanut · 2017-09-24T09:10:58Z

components/script/dom/htmlimageelement.rs

+        }
+        if !error {
+            let descriptor = Descriptor { wid: width, den: density };
+            let imageSource = ImageSource { url: url, descriptor: descriptor };


Nit: camel case - image_source

wafflespeanut · 2017-09-24T09:10:58Z

tests/unit/script/htmlimageelement.rs

+    let empty_vec = Vec::new();
+    assert_eq!(parse_a_srcset_attribute("small-image.jpg 320w 1.1x"), empty_vec);
+}
+#[test]


Nit: newline between functions please

wafflespeanut · 2017-09-24T09:10:58Z

components/script/dom/htmlimageelement.rs

+            let has_x = remaining == "x";
+            let has_h = remaining == "h";
+            if valid_non_negative_integer.is_ok() && has_w {
+                let result = parse_unsigned_integer(char_iter.clone());


Why should we parse this again if we already have the result in valid_non_negative_integer? We could just unwrap it, right?

wafflespeanut · 2017-09-24T09:10:58Z

components/script/dom/htmlimageelement.rs

+                    width = Some(w);
+                }
+            } else if valid_floating_point.is_ok() && has_x {
+                let result = parse_double(digits);


Ditto on unnecessary re-parsing. We should unwrap from valid_floating_point

wafflespeanut · 2017-09-24T09:10:58Z

components/script/dom/htmlimageelement.rs

+                    density = Some(x);
+                }
+            } else if valid_non_negative_integer.is_ok() && has_h {
+                let result = parse_unsigned_integer(char_iter.clone());


Ditto on re-parsing.

wafflespeanut · 2017-09-28T09:12:52Z

components/script/dom/htmlimageelement.rs

+            }
+        }
+    return (s, "");
+    }


Here's the correct indentation:

pub fn collect_sequence_characters<F>(s: &str, predicate: F) -> (&str, &str) where F: Fn(&char) -> bool { for (i, ch) in s.chars().enumerate() { if !predicate(&ch) { return (&s[0..i], &s[i..]) } } return (s, ""); }

wafflespeanut · 2017-09-28T09:15:20Z

Sorry, I just realized that you haven't finished updating the code. 😅


        Parse srcset attribute


        Test cases for parse srcset attribute

jdm · 2017-10-02T18:44:47Z

I've squashed these commits in #18716.


        Auto merge of #18716 - jdm:parseSrcset, r=jdm

Parse srcset attribute values Squashed version of #18313. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix (partially) #11416 - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18716)


        Auto merge of #18716 - jdm:parseSrcset, r=jdm

Parse srcset attribute values Squashed version of #18313. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix (partially) #11416 - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18716)

highfive assigned cbrewster Aug 30, 2017

highfive added the S-awaiting-review label Aug 30, 2017

jdm assigned jdm and unassigned cbrewster Aug 30, 2017

wafflespeanut requested changes Aug 31, 2017

View changes

wafflespeanut reviewed Aug 31, 2017

View changes

iamneha force-pushed the iamneha:parseSrcset branch from ecbae6f to 26c7c30 Sep 4, 2017

iamneha mentioned this pull request Sep 5, 2017

Support responsive images #11416

Closed

7 of 7 tasks complete

creativcoder reviewed Sep 5, 2017

View changes

components/script/dom/htmlimageelement.rs Outdated

} else {

false

};

let has_h = if remaining == "h" {

This comment has been minimized.

Sign in to view

creativcoder Sep 5, 2017
Contributor

Same as above

creativcoder reviewed Sep 5, 2017

View changes

iamneha force-pushed the iamneha:parseSrcset branch from 26c7c30 to 36e8270 Sep 5, 2017

KiChjang added the S-needs-rebase label Sep 5, 2017

KiChjang requested changes Sep 5, 2017

View changes

KiChjang assigned KiChjang and unassigned jdm Sep 5, 2017

jdm unassigned KiChjang Sep 5, 2017

iamneha force-pushed the iamneha:parseSrcset branch 3 times, most recently from 30c6228 to 956ac15 Sep 16, 2017

iamneha changed the title ~~WIP(Parse srcset) - Need to add more test cases~~ Parse Srcset Attribute Sep 20, 2017

iamneha force-pushed the iamneha:parseSrcset branch from 4d1f7fe to 3292fd6 Sep 21, 2017

wafflespeanut requested changes Sep 24, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase S-needs-squash labels Sep 25, 2017

iamneha force-pushed the iamneha:parseSrcset branch from 3292fd6 to 7ef3d37 Sep 26, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 26, 2017

wafflespeanut reviewed Sep 28, 2017

View changes

wafflespeanut added S-needs-code-changes and removed S-awaiting-review labels Sep 28, 2017

Parse srcset attribute

Loading status checks…

84a88d3

iamneha force-pushed the iamneha:parseSrcset branch from 43c332f to 84a88d3 Sep 29, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 29, 2017

Test cases for parse srcset attribute

Loading status checks…

4c427f4

jdm mentioned this pull request Oct 2, 2017

Parse srcset attribute values #18716

Merged

4 of 4 tasks complete

jdm closed this Oct 2, 2017

servo / servo

Join GitHub today

Parse Srcset Attribute #18313

Parse Srcset Attribute #18313

Conversation

iamneha commented Aug 30, 2017 • edited

highfive commented Aug 30, 2017

highfive commented Aug 30, 2017

wafflespeanut 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.

wafflespeanut commented Aug 31, 2017

This comment has been minimized.

wafflespeanut Aug 31, 2017 • edited

wafflespeanut commented Sep 4, 2017

KiChjang commented Sep 5, 2017

iamneha commented Sep 5, 2017

This comment has been minimized.

creativcoder Sep 5, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

creativcoder Sep 5, 2017 • edited

This comment has been minimized.

This comment has been minimized.

creativcoder Sep 5, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

creativcoder Sep 5, 2017 • edited

KiChjang commented Sep 5, 2017

KiChjang 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.

KiChjang Sep 5, 2017 • 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.

wafflespeanut 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.

wafflespeanut commented Sep 28, 2017

jdm commented Oct 2, 2017

iamneha commented Aug 30, 2017 •

edited

wafflespeanut Aug 31, 2017 •

edited

creativcoder Sep 5, 2017 •

edited

creativcoder Sep 5, 2017 •

edited

creativcoder Sep 5, 2017 •

edited

creativcoder Sep 5, 2017 •

edited

KiChjang Sep 5, 2017 •

edited