1352025 - stylo: Stop passing url as string into Servo side

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

This is the step 2 of bug 1343964 comment 9.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1352025 part 1 - Use a UrlExtraData type alias to unify url handling logic.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Use final url for updating stylesheet from @import rule.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1352025 part 2 - Stop passing in url as string for parsing.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Use noop loader whenever loader is not provided for @import.

I don't really know what I should do here.

It seems that before the changes here, stylesheet.base_url.is_invalid() is
always true for geckolib? After these changes, base_url is replaced by
url_data, which is now valid url info for geckolib, so this "expect" panics.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Move DUMMY_URL_DATA to glue and use static to avoid shutdown leak.

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1352025 part 3 - Update test expectations.

As a side effect of unifying url handling in part 1, url values in
unparsed property value and variable now works, thus this change.

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

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

This patch set is based on the patch in bug 1351957.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8853639 [details]
Use final url for updating stylesheet from @import rule.

https://reviewboard.mozilla.org/r/125730/#review128268

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8853641 [details]
Use noop loader whenever loader is not provided for @import.

https://reviewboard.mozilla.org/r/125734/#review128270

::: servo/components/style/stylesheets.rs:873
(Diff revision 1)
>                      let specified_url = SpecifiedUrl::parse_from_string(url_string, &self.context)?;
>  
>                      let media = parse_media_query_list(input);
>  
>                      let noop_loader = NoOpLoader;
> -                    let loader = if !specified_url.is_invalid() {
> +                    let loader = self.loader.unwrap_or(&noop_loader);

This feels really wrong, and shouldn't be needed. We should always have a loader except for parsing some UA sheets where there are no imports.

Can you see when this is happening? Where are we giving no loader from Geckolib?

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8853638 [details]
Bug 1352025 part 1 - Use a UrlExtraData type alias to unify url handling logic.

https://reviewboard.mozilla.org/r/125728/#review128272

::: layout/style/ServoBindings.h:123
(Diff revision 1)
>                            const uint8_t* media_bytes,
>                            uint32_t media_length);
>  
> +// URLExtraData
> +// Create a new addrefed URLExtraData.
> +RawGeckoURLExtraData* Gecko_URLExtraData_CreateDummy();

Use `RawGeckoURLExtraDataStrong`? We can then make the bindings produce a RefPtr directly.

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8853643 [details]
Bug 1352025 part 3 - Update test expectations.

https://reviewboard.mozilla.org/r/125738/#review128274

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8853640 [details]
Bug 1352025 part 2 - Stop passing in url as string for parsing.

https://reviewboard.mozilla.org/r/125732/#review128276

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8853642 [details]
Move DUMMY_URL_DATA to glue and use static to avoid shutdown leak.

https://reviewboard.mozilla.org/r/125736/#review128278

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8853641 [details]
Use noop loader whenever loader is not provided for @import.

https://reviewboard.mozilla.org/r/125734/#review128270

> This feels really wrong, and shouldn't be needed. We should always have a loader except for parsing some UA sheets where there are no imports.
> 
> Can you see when this is happening? Where are we giving no loader from Geckolib?

Here: https://dxr.mozilla.org/mozilla-central/rev/7a3f514cf8490d271ee373a1d2999e4ea4dee2d7/servo/components/style/stylesheets.rs#358-365

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

So that function is called from CSSOM, when one tries to insert a new @import rule into the style sheet.

Without that change, test_rule_insertion.html crashes at the expect. See https://treeherder.mozilla.org/logviewer.html#?job_id=87735784&repo=try&lineNumber=2507 for example.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8853638 [details]
Bug 1352025 part 1 - Use a UrlExtraData type alias to unify url handling logic.

https://reviewboard.mozilla.org/r/125728/#review128272

> Use `RawGeckoURLExtraDataStrong`? We can then make the bindings produce a RefPtr directly.

`Strong` is only for Servo arc types, isn't it? It doesn't seem to me that we use this sugar for Gecko refcounted types.

Comment 17

[Manish Goregaokar [:manishearth]]

> `Strong` is only for Servo arc types, isn't it? It doesn't seem to me that we use this sugar for Gecko refcounted types.

Yes, but we could. Leave it for now, I'll try to write bindings for that later.

Comment 18

[Emilio Cobos Álvarez [:emilio]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #15)
> So that function is called from CSSOM, when one tries to insert a new
> @import rule into the style sheet.
> 
> Without that change, test_rule_insertion.html crashes at the expect. See
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=87735784&repo=try&lineNumber=2507 for example.

Seems to me we really need to pass a loader there, otherwise the @import'ed stylesheet would never be loaded, right? Or do we load them lazily later somehow?

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #18)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #15)
> > So that function is called from CSSOM, when one tries to insert a new
> > @import rule into the style sheet.
> > 
> > Without that change, test_rule_insertion.html crashes at the expect. See
> > https://treeherder.mozilla.org/logviewer.
> > html#?job_id=87735784&repo=try&lineNumber=2507 for example.
> 
> Seems to me we really need to pass a loader there, otherwise the @import'ed
> stylesheet would never be loaded, right? Or do we load them lazily later
> somehow?

No, I don't think we load them lazily somehow. If you don't think there is any, then there isn't any :) I don't think there is any magic here.

So, yeah, probably we should pass in a loader there. Actually CSSStyleSheet::InsertRuleInternal does pass in a loader to the parser. We should do the same for ServoStyleSheet.

If I want to avoid crash here, I should probably do it in a prerequisite bug, then.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8853638 [details]
Bug 1352025 part 1 - Use a UrlExtraData type alias to unify url handling logic.

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

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8853640 [details]
Bug 1352025 part 2 - Stop passing in url as string for parsing.

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

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8853643 [details]
Bug 1352025 part 3 - Update test expectations.

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

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

https://github.com/servo/servo/pull/16241

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5e5c568c04f
part 1 - Use a UrlExtraData type alias to unify url handling logic. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/3952d677963e
part 2 - Stop passing in url as string for parsing. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/07acdc80806e
part 3 - Update test expectations. r=manishearth

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5e5c568c04f
https://hg.mozilla.org/mozilla-central/rev/3952d677963e
https://hg.mozilla.org/mozilla-central/rev/07acdc80806e