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.

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

WIP #17808

Closed
wants to merge 5 commits into from

Conversation

@Rakhisharma
Copy link
Contributor

commented Jul 20, 2017

Parse sizes attribute implementation.

  • ./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).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jul 20, 2017

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

@highfive

This comment has been minimized.

Copy link

commented Jul 20, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmlimageelement.rs
  • @KiChjang: components/script/dom/htmlimageelement.rs
@highfive

This comment has been minimized.

Copy link

commented Jul 20, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

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

@jdm jdm removed the S-awaiting-review label Jul 24, 2017
@jdm jdm assigned jdm and unassigned asajeffrey Jul 24, 2017
@Rakhisharma Rakhisharma force-pushed the Rakhisharma:sizes branch from 5fc23a3 to bc95d0a Jul 27, 2017
Copy link
Member

left a comment

Good start!

@@ -782,6 +797,71 @@ impl LayoutHTMLImageElementHelpers for LayoutJS<HTMLImageElement> {
}
}

pub fn parse_a_sizes_attribute(input: DOMString, width: Option<u32>) -> Result<Vec<Size>, String> {

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

Since we only return Ok from this method (and not Err), there is no reason to return a Result value.

let trimmed: String = unparsed_size.chars().take(unparsed_size.chars().count() - whitespace).collect();

if trimmed.is_empty() {
// return Err("reason".to_owned());

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

Please remove code instead of commenting it out.

},
}
}
if sizes.len() == 0 {

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

nit: use sizes.is_empty() instead.


#[test]
fn empty_vector() {
assert!(parse_a_sizes_attribute(DOMString::new(), None).is_ok());

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

We will want to assert that the result is an empty vector (as the test name implies ;))

This comment has been minimized.

Copy link
@creativcoder

creativcoder Jul 30, 2017

Contributor

@Rakhisharma As discussed, the test case depicts the situation where sizes is an empty string (DOMString::new()), so here we should also check if the vector contains a single Size with the value 100vw. (i.e., the default value returned from spec algorithm)

#[test]
fn test_whitespace() {
assert!(parse_a_sizes_attribute(DOMString::from(" (min-width: 500px)"),
None).is_ok());

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

Let's think about what this test is checking. (min-width: 500px) by itself is not a valid size; it is a media query, and the sizes algorithm then tries to parse a length that follows it. Since there is no length present, this string would be considered a parse error. That's fine, but we should also verify in a separate test that a valid size with extra whitespace parses correctly and gives us a real parsed size.

let media_query = media_query_parser.try(|i| MediaQuery::parse(&context, i));
match media_query {
Ok(query) => {
let length = media_query_parser.try(|i| Length::parse_non_negative(&context, i));

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

We can use Length::parse_non_negative directly instead of Parser::try: let length = Length::parse_non_negative(&context, &mut media_query_parser);

match media_query {
Ok(query) => {
let length = media_query_parser.try(|i| Length::parse_non_negative(&context, i));
if length.is_ok() {

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

Instead of using is_ok and unwrap, we can write if let Ok(length) = length {.

let mut media_query_parser = Parser::new(&mut input);
let media_query = media_query_parser.try(|i| MediaQuery::parse(&context, i));
match media_query {
Ok(query) => {

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

Since the Err branch of this match is empty except for continue, we can write if let Ok(query) = media_query { and omit it entirely.

@@ -66,6 +76,11 @@ enum State {
CompletelyAvailable,
Broken,
}
#[allow(dead_code)]

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

This probably is not considered dead code any more, so we can remove it.

@@ -3,6 +3,8 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use app_units::{Au, AU_PER_PX};
use core::ops::Deref;

This comment has been minimized.

Copy link
@jdm

jdm Jul 27, 2017

Member

This can be removed.

This comment has been minimized.

Copy link
@Rakhisharma

Rakhisharma Jul 29, 2017

Author Contributor

we are using this at line: 802: let unparsed_sizes = input.deref().split(',').collect::<Vec<_>>();

This comment has been minimized.

Copy link
@KiChjang

KiChjang Jul 29, 2017

Member

An explicit call to deref should not be necessary, if let unparsed_sizes = input.split(',').collect::<Vec<_>>(); does not work, try let unparsed_sizes = (*input).split(',').collect::<Vec<_>>();.

This comment has been minimized.

Copy link
@Rakhisharma

Rakhisharma Jul 30, 2017

Author Contributor

let unparsed_sizes = input.split(',').collect::<Vec<_>>(); worked! thanks.

None,
ParsingMode::empty(),
QuirksMode::NoQuirks);
let url = ServoUrl::parse("about:blank").unwrap();

This comment has been minimized.

Copy link
@creativcoder

creativcoder Jul 30, 2017

Contributor

You already have url declared in line 813. So you can remove this.

@@ -12,4 +12,6 @@ extern crate servo_url;
#[cfg(test)] mod textinput;
#[cfg(test)] mod headers;
#[cfg(test)] mod htmlareaelement;
#[cfg(test)] mod htmlimageelement;

This comment has been minimized.

Copy link
@creativcoder

creativcoder Jul 30, 2017

Contributor

nit: Remove the extra newline.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2017

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

Copy link
Member

left a comment

This is a good start! The tests can use a bit more refining, but I think we're close to having something that's ready to be merged!

None,
ParsingMode::empty(),
QuirksMode::NoQuirks);
let length = Parser::new(&mut input).try(|i| Length::parse_non_negative(&context, i));

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

We should create a separate parser object that we can reuse later.

This comment has been minimized.

Copy link
@Rakhisharma

Rakhisharma Sep 6, 2017

Author Contributor

How to create a separate parser object here? I am not clear if we can create a Parser object without any input string and reuse it later.

This comment has been minimized.

Copy link
@jdm

jdm Sep 6, 2017

Member
let mut parser = Parser::new(&mut input);
/* ... */
let length = parser.try(..);
/* .. */
let something_else = parser.foo(..).
query: None
}),
Err(_) => {
let mut media_query_parser = Parser::new(&mut input);

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

We can reuse the earlier parser instead.

let media_query = media_query_parser.try(|i| MediaQuery::parse(&context, i));
if let Ok(query) = media_query {
let length = Length::parse_non_negative(&context, &mut media_query_parser);
if length.is_ok() {

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

We can use if let Ok(length) = length { here and avoid calling unwrap below.

@@ -782,6 +798,63 @@ impl LayoutHTMLImageElementHelpers for LayoutJS<HTMLImageElement> {
}
}

pub fn parse_a_sizes_attribute(input: DOMString, width: Option<u32>) -> Vec<Size> {

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

Add a comment like // https://html.spec.whatwg.org/multipage/#parse-a-sizes-attribute so it's clear which algorithm this is based on.

make_getter!(Sizes, "sizes");
// https://html.spec.whatwg.org/multipage/#parse-a-sizes-attribute
make_setter!(SetSizes, "sizes");

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

nit: remove this extra newline.


#[test]
fn no_default_provided() {
assert!(parse_a_sizes_attribute(DOMString::new(), None).len() == 1);

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

Instead of asserting the length, we should be asserting the actual value that is returned in all of these tests.


#[test]
fn no_size() {
assert!(parse_a_sizes_attribute(DOMString::new(), None).len() == 1);

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

This is the same as no_default_provided.

let length = test_length(545f32);
let size = Size { query: Some(media_query), length: length };
a.push(size);
assert_eq!(parse_a_sizes_attribute(DOMString::from(" (max-width: 200px) 545px"), None), a);

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

It's confusing how this is called no_extra_whitespace but the test includes a leading space.

This comment has been minimized.

Copy link
@Rakhisharma

Rakhisharma Sep 6, 2017

Author Contributor

Here what you mean with the extra whitespace, "does the middle spaces also count as extra whitespace or only the leading and trailing spaces counts as extra whitespace ?"

This comment has been minimized.

Copy link
@jdm

jdm Sep 6, 2017

Member

Extra whitespace generally means any additional whitespace beyond what is necessary to separate two tokens, regardless of where the whitespace exists in the string.

(max-width: 900px) and (min-width: 400px) 50em,
100vw "),
None);
assert_eq!(result.len(), 3);

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

We should definitely be checking the actual values that are returned here.

let length = test_length(545f32);
let size = Size { query: Some(media_query), length: length };
a.push(size);
assert_eq!(parse_a_sizes_attribute(DOMString::from("(max-width: 320px) 280px, (max-width: 480px) 440px, 800px"), None), a);

This comment has been minimized.

Copy link
@jdm

jdm Aug 31, 2017

Member

Why does this test pass? We are parsing a string that contains three different sizes, but the expected results only contain one size that does not match any of them.

@KiChjang

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

Sorry, but which issue does this PR solve?

@creativcoder

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2017

This is part of #11416

@Rakhisharma Rakhisharma force-pushed the Rakhisharma:sizes branch from ee28de0 to 083bb37 Sep 21, 2017
Copy link
Member

left a comment

Nice work! This mostly looks good to me. Just a few nits, and we'll be good to go.

@@ -66,6 +74,12 @@ enum State {
CompletelyAvailable,
Broken,
}
#[derive(PartialEq)]

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Sep 24, 2017

Member

Nit: newline before types please

pub fn parse_a_sizes_attribute(input: DOMString, width: Option<u32>) -> Vec<Size> {
let mut sizes = Vec::<Size>::new();
let unparsed_sizes = input.split(',').collect::<Vec<_>>();
for unparsed_size in &unparsed_sizes {

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Sep 24, 2017

Member

Since unparsed_sizes is used only in the loop, let's iterate on input.split directly (which avoids an iteration and vector allocation).

This comment has been minimized.

Copy link
@Rakhisharma

Rakhisharma Sep 25, 2017

Author Contributor

Not sure what you want me to do here. Would you please elaborate little more. Thanks :)

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Sep 25, 2017

Member

I meant,

for unparsed_size in input.split(',') {

This way, we perform lazy iteration.

let mut media_query_parser = parser;
let media_query = media_query_parser.try(|i| MediaQuery::parse(&context, i));
if let Ok(query) = media_query {
let length = Length::parse_non_negative(&context, &mut media_query_parser);

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Sep 24, 2017

Member

Nit: Improper indentation (should be 4 spaces instead of 8).

let size = Size { query: None, length: length };
a.push(size);
assert_eq!(parse_a_sizes_attribute(DOMString::new(), None), a);
println!("{:?}", parse_a_sizes_attribute(DOMString::new(), None));

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Sep 24, 2017

Member

Nit: Please remove println! usage.

Note for the future: println! is not useful in the testsuite because the stdout is captured by cargo. Instead, we should panic! so that cargo prints the captured stdout (which also has the panic message).

let size = Size { query: None, length: length };
a.push(size);
assert_eq!(parse_a_sizes_attribute(DOMString::new(), Some(2)), a);
println!("{:?}", parse_a_sizes_attribute(DOMString::new(), Some(2)));

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Sep 24, 2017

Member

Nit: Ditto on println!

let mut a = vec![];
let length = test_length_for_default_provided(2f32);
let size = Size { query: None, length: length };
a.push(size);

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Sep 24, 2017

Member

Nit: If it's just a single size, then let's have let a = vec![size]; (here and everywhere below)

Rakhisharma added 2 commits Sep 25, 2017
@jdm jdm referenced this pull request Oct 2, 2017
4 of 4 tasks complete
@jdm

This comment has been minimized.

Copy link
Member

commented Oct 2, 2017

I went ahead and squashed these commits and opened a new PR in #18714.

@jdm jdm closed this Oct 2, 2017
bors-servo added a commit that referenced this pull request Oct 3, 2017
Parse sizes attribute values

Squashed version of #17808.

---
- [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

<!-- 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/18714)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.