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

Reintroducing parse_a_sizes along with tests #21178

Closed
wants to merge 1 commit into from

Conversation

@paavininanda
Copy link
Contributor

paavininanda commented Jul 14, 2018


  • ./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

This change is Reviewable

@highfive
Copy link

highfive commented Jul 14, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/test.rs, components/script/dom/bindings/trace.rs, components/script/dom/htmlimageelement.rs
  • @KiChjang: components/script/test.rs, components/script/dom/bindings/trace.rs, components/script/dom/htmlimageelement.rs
  • @emilio: components/style/values/specified/source_size_list.rs
@paavininanda
Copy link
Contributor Author

paavininanda commented Jul 14, 2018

r? @jdm

@highfive highfive assigned jdm and unassigned avadacatavra Jul 14, 2018
@paavininanda paavininanda force-pushed the paavininanda:Parse_a_sizes branch from a890240 to e3f97a3 Jul 14, 2018
},
None => SourceSizeList {
source_sizes: vec![],
value: Some(Length::NoCalc(NoCalcLength::ViewportPercentage(ViewportPercentageLength::Vw(100.))))

This comment has been minimized.

Copy link
@emilio

emilio Jul 15, 2018

Member

Also note that this is not needed, SourceSizeList::evaluate already handles this correctly.

@nupurbaghel nupurbaghel mentioned this pull request Jul 15, 2018
4 of 4 tasks complete


#[test]
fn no_default_provided() {

This comment has been minimized.

Copy link
@emilio

emilio Jul 16, 2018

Member

I think I'd prefer to remove the unit tests and make the SourceSizeList and SourceSize fields private again, given there's a few tests that check all this in WPT: https://wpt.fyi/results/html/semantics/embedded-content/the-img-element/sizes

This comment has been minimized.

Copy link
@jdm

jdm Jul 16, 2018

Member

I would prefer to retain the unit tests for the time being until we finish implementing all the pieces that are required in order for the WPT tests to actually work in Servo.

This comment has been minimized.

Copy link
@emilio

emilio Jul 16, 2018

Member

That means that when I change SourceSizeList in Gecko I need to update all the tests, which is cumbersome given this is tested already there... :/

This comment has been minimized.

Copy link
@jdm

jdm Jul 17, 2018

Member

Ok, that's a reasonable argument against these changes. @paavininanda Let's undo the changes that were made to support re-adding the unit tests, and remove the unit tests. This PR can just focus on adding the parse_a_sizes_attribute function without using it anywhere.



#[test]
fn no_default_provided() {

This comment has been minimized.

Copy link
@jdm

jdm Jul 17, 2018

Member

Ok, that's a reasonable argument against these changes. @paavininanda Let's undo the changes that were made to support re-adding the unit tests, and remove the unit tests. This PR can just focus on adding the parse_a_sizes_attribute function without using it anywhere.

@paavininanda paavininanda force-pushed the paavininanda:Parse_a_sizes branch from cc30565 to 26e2241 Jul 17, 2018
@paavininanda paavininanda force-pushed the paavininanda:Parse_a_sizes branch from 26e2241 to d2f467c Jul 17, 2018
@jdm
Copy link
Member

jdm commented Jul 17, 2018

Since this is part of #21181 which makes use of it, let's just wait for that PR to merge.

@jdm jdm closed this Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.