Closed Bug 1352025 Opened 3 years ago Closed 3 years ago

stylo: Stop passing url as string into Servo side

Categories

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

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(3 files, 3 obsolete files)

This is the step 2 of bug 1343964 comment 9.
Assignee: nobody → xidorn+moz
Priority: -- → P1
This patch set is based on the patch in bug 1351957.
Comment on attachment 8853639 [details]
Use final url for updating stylesheet from @import rule.

https://reviewboard.mozilla.org/r/125730/#review128268
Attachment #8853639 - Flags: review?(emilio+bugs) → review+
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?
Attachment #8853641 - Flags: review?(emilio+bugs) → review-
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.
Attachment #8853638 - Flags: review?(manishearth) → review+
Comment on attachment 8853643 [details]
Bug 1352025 part 3 - Update test expectations.

https://reviewboard.mozilla.org/r/125738/#review128274
Attachment #8853643 - Flags: review?(manishearth) → review+
Comment on attachment 8853640 [details]
Bug 1352025 part 2 - Stop passing in url as string for parsing.

https://reviewboard.mozilla.org/r/125732/#review128276
Attachment #8853640 - Flags: review?(manishearth) → review+
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
Attachment #8853642 - Flags: review?(manishearth) → review+
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
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 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.
> `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.
(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?
(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.
Depends on: 1352763
Attachment #8853639 - Attachment is obsolete: true
Attachment #8853641 - Attachment is obsolete: true
Attachment #8853642 - Attachment is obsolete: true
Pushed by xquan@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.