1408308 - make ResponsiveImageSelector use Servo for parsing its source size list

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Cameron McCormack (:heycam)]

Currently it uses nsCSSParser to produce arrays of nsMediaQuery and nsCSSValue objects.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I'll take this one, I think.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/19126 introduces the CSS parsing and evaluation bits.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1408308: Integrate Servo SourceSizeList in ResponsiveImageSelector.

Review commit: https://reviewboard.mozilla.org/r/198170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198170/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)

This is a WIP. The wpt tests expect less strict grammar, so will work on that, but I didn't want to lose track of this.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8926931 [details]
Bug 1408308: Integrate Servo SourceSizeList in ResponsiveImageSelector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198170/diff/1-2/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8926931 [details]
Bug 1408308: Integrate Servo SourceSizeList in ResponsiveImageSelector.

Hmm, Xidorn and Cam are on PTO... Over to Manish I guess?

Note to self and reviewer: Need to update the spec links to reflect spec discussions, also probably add the HasBoxFFI boilerplate.

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8926931 [details]
Bug 1408308: Integrate Servo SourceSizeList in ResponsiveImageSelector.

https://reviewboard.mozilla.org/r/198170/#review203966

::: servo/components/style/values/specified/source_size_list.rs:99
(Diff revision 2)
> -        }).unwrap_or(vec![]);
>  
> -        let value = Length::parse_non_negative(context, input)?;
> +        loop {
> +            let result = input.parse_until_before(Delimiter::Comma, |input| {
> +                if let Ok(size) = input.try(|input| SourceSize::parse(context, input)) {
> +                    return Ok(SourceSizeOrLength::SourceSize(size));

could you structure this as an implicit return in an if let ... else block?

The explicit return makes it seem as if it returns out of the entire function, which is kinda confusing.

::: servo/ports/geckolib/glue.rs:4672
(Diff revision 2)
> +
> +    result.0
> +}
> +
> +#[no_mangle]
> +pub unsafe extern "C" fn Servo_SourceSizeList_Drop(list: *mut SourceSizeList) {

We don't bind to this or use it. Who handles the destructor?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Manish Goregaokar [:manishearth] from comment #7)
> Comment on attachment 8926931 [details]
> Bug 1408308: Integrate Servo SourceSizeList in ResponsiveImageSelector.
> 
> https://reviewboard.mozilla.org/r/198170/#review203966
> 
> ::: servo/components/style/values/specified/source_size_list.rs:99
> (Diff revision 2)
> > -        }).unwrap_or(vec![]);
> >  
> > -        let value = Length::parse_non_negative(context, input)?;
> > +        loop {
> > +            let result = input.parse_until_before(Delimiter::Comma, |input| {
> > +                if let Ok(size) = input.try(|input| SourceSize::parse(context, input)) {
> > +                    return Ok(SourceSizeOrLength::SourceSize(size));
> 
> could you structure this as an implicit return in an if let ... else block?
> 
> The explicit return makes it seem as if it returns out of the entire
> function, which is kinda confusing.

Sure, sounds ok.

> ::: servo/ports/geckolib/glue.rs:4672
> (Diff revision 2)
> > +
> > +    result.0
> > +}
> > +
> > +#[no_mangle]
> > +pub unsafe extern "C" fn Servo_SourceSizeList_Drop(list: *mut SourceSizeList) {
> 
> We don't bind to this or use it. Who handles the destructor?

The DEFINE_BOXED_TYPE thing. You wrote that :P

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b59ffea83e0a
Integrate Servo SourceSizeList in ResponsiveImageSelector. r=Manishearth

Comment 10

[Eliza Balazs [:ebalazs_]]

Backed out 1 changesets (bug 1408308) for bustage in [toolkit/library/target] r=backout on a CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b59ffea83e0abe345853fa76dba4bca551464858&selectedJob=144188189

https://hg.mozilla.org/integration/autoland/rev/168af2eb1faf085e06768dd11431175242afb5ee

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/136ee1bdf20d
Reland the bindings so the build isn't busted, but we don't need to backout the servo bits. r=me

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I think it's the mingw build and the asan fuzzing build... I suspect I'm missing includes or something. Will fix up tomorrow.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78d7ac2ce842
Integrate Servo SourceSizeList in ResponsiveImageSelector. r=Manishearth

Comment 14

[Tom Ritter [:tjr]]

Bug 1416756 is the MinGW build break. I'll put details there.

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/136ee1bdf20d
https://hg.mozilla.org/mozilla-central/rev/78d7ac2ce842

Comment 16

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.