Parse A Size Attribute

[sneha1302]

Hi ,

Have started the parse_a_size_attribute for image load conform , but need suggestions and so creating the pull Request .

---

This change is

[highfive]

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

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/net_traits/image_cache_thread.rs, components/net_traits/image_cache_thread.rs, components/script/dom/htmlimageelement.rs, components/net/image_cache_thread.rs

[highfive]

Warning

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

[jdm]

For the benefit of anyone else reading, copied from my last email:

Given that the sizes attribute follows a pattern (see example in http://bitsofco.de/the-srcset-and-sizes-attributes/ ), I recommend:
* define a structure that contains an optional media condition and a non-optional length
* make the function return instances of this new structure
* using Parser::try to attempt to parse a solitary length using Length::parse and returning that value if it succeeds
* otherwise, use MediaQuery::parse [1] to parse a media condition, then use Length::parse to parse a length, and return that value

You'll be using instances of the Result enum to properly interface with the parser, too: http://rustbyexample.com/error/result.html

If there are particular things which you are looking for suggestions about, please state them :)

[bors-servo]

☔ The latest upstream changes (presumably #10767) made this pull request unmergeable. Please resolve the merge conflicts.

[sneha1302]

@jdm : could you see if the function declaration looks right , and also - unparsed_size_list ?
Thanks ! IF any further suggestion on how to go about white space token check , would be helpful !

[jdm]

I strongly recommend writing unit tests for your code so you have some idea of exactly what it's doing. You can do this directly in tests/unit/script/lib.rs by adding:

use script::dom::htmlimageelement::parse_a_sizes_attribute;
#[test]
fn some_parse_sizes_test() {
    let result = parse_a_sizes_attribute(...);
    assert!(/* something about result that is boolean */);
}

and run it with ./mach test-unit -p script.

Now, as for the code: expect_string doesn't do what you want, which is presumably to return all of the non-comma characters in the string being parsed. I think the API of CSSParser is working against us here, and we should stop trying to use it in this code. Instead, we can use let sizes = input.iter().split(',').collect::<Vec<_>>() which will return a vector of strings and matches what you're attmpting to do in the following code.

As for the whitespace checks, we can use iterators to our advantage. The code looks like it should be equivalent to

let whitespace = unparsed_size.chars().rev().take_while(|c| util::str::char_is_whitespace(c)).count();
let trimmed = unparsed_size.chars().take(unparsed_size.chars().count() - whitespace).as_str();

trimmed will be a string that does not have any terminating whitespace after this.

[sneha1302]

The iterator was giving error , so included
use std::iter::Iterator;

let mut iter = Parser::new(input);
before iterating , but it gives error . What type should the iterator be , or look like?

[jdm]

We don't want to use Parser::new at all. We should be operating directly on the string, meaning the input argument to the function should be a DOMString value.

[sneha1302]

@jdm :
for the second step in the for each unparsed_size (i.e If the last component value in unparsed size is a valid non-negative ,) . How do we go about checking if the value is valid or not ? is there any existing function or method to do this check ? Your suggestions will be helpful

Thanks !

[jdm]

Yes. Here's where we can use the cssparser::Parser API effectively - Length::parse_non_negative accepts a reference to a Parser value, and returns a Result value that contains a Length if it was parsed correctly. This also correctly parses calc functions without permitting any others, based on my reading of Length::parse_internal. After parsing the value, you can use the expect_whitespace API, followed by Expression::parse for the remaining steps.

[sneha1302]

@jdm
let trimmed = unparsed_size.chars().take(unparsed_size.chars().count() - whitespace).as_str();
in the above code , as suggested . The as_str() method doesnot work as it says that "take" does not have that method . Any suggestion ?

[jdm]

My mistake for not trying it out in https://play.rust-lang.org/ first. This works:

let s = "foo bar   ".to_owned();
let whitespace = s.chars().rev().take_while(|c| *c == ' ').count();
let nonwhitespace: String = s.chars().take(s.chars().count() - whitespace).collect();
println!("|{}|", nonwhitespace);

[sneha1302]

@jdm
Could you check the code and suggest whats wrong and what all needs to be done .

Thanks ! sneha

[jdm]

I strongly recommend writing unit tests that put your code through its paces. Start with the case of a single length value, then try two comma-separated values where the first one has a media expression, then three. Also try the cases where there's a parse error expected, and trailing whitespace, etc. http://bitsofco.de/the-srcset-and-sizes-attributes/ has a great example that shows some sample values that can exist in the sizes attribute.

[jdm]

I'll point out that a media query is different than a media expression. We don't want to try to parse a whole media query here; I mentioned Expression::parse for a reason.

[srm09]

Length:parse_non_negative does not work for the value calc(100%-3em). Works for __em

[srm09]

Also, shouldn't the struct Size encapsulate an optional MediaQuery and Length making it

pub struct Size {
    query: Option<MediaQuery>,
    length: Length,
}

[srm09]

MediaQuery::parse does not recognize the following inputs:

( not (orientation: portrait) ) 300px,
( (orientation: landscape) or (min-width: 1000px) ) 50vw

[Ms2ger]

What happened here?

[srm09]

@Ms2ger This branch was under development. Since, it had some extra changes which had already been pushed to servo:master as part of #10767 , I removed that commit and reset all the new commits made on top of that one.