1325878 - stylo: Implement Servo-backed nsMediaList

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

This is required for feature-parity between Stylo and Gecko in @import and media queries.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Some initial refactoring in https://github.com/servo/servo/pull/14739

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Allow creating empty Servo MediaList.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Create less hardcoded nsMediaList instances.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Use dom::MediaList in the CSS Loader.

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Don't use nsMediaList for loading imports.

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8856326 [details]
Bug 1325878: Allow creating empty Servo MediaList.

https://reviewboard.mozilla.org/r/128230/#review130766

::: servo/ports/geckolib/glue.rs:1133
(Diff revision 1)
>  #[no_mangle]
> +pub extern "C" fn Servo_MediaList_Create() -> RawServoMediaListStrong {
> +
> +    let global_style_data = &*GLOBAL_STYLE_DATA;
> +    Arc::new(global_style_data.shared_lock.wrap(MediaList::default())).into_strong()
> +

Probably remove this empty line?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8856327 [details]
Bug 1325878: Create less hardcoded nsMediaList instances.

https://reviewboard.mozilla.org/r/128232/#review130768

::: dom/html/HTMLSourceElement.cpp:91
(Diff revision 1)
>    nsString mediaStr;
>    if (!aValue || (mediaStr = aValue->GetStringValue()).IsEmpty()) {
>      return;
>    }
>  
>    nsCSSParser cssParser;

This `cssParser` is unused now.

::: layout/style/MediaList.h:45
(Diff revision 1)
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(MediaList)
>  
>    /**
>     * Creates a MediaList backed by the StyleBackendType of the document.
>     */
> -  static already_AddRefed<MediaList> Create(nsIDocument* aDocument,
> +  static already_AddRefed<MediaList> Create(const nsIDocument& aDocument,

Per the discussion this morning, probably better keep using pointer rather than reference for `nsIDocument` here?

::: layout/style/MediaList.h:55
(Diff revision 1)
> +  virtual bool Matches(nsPresContext& aPresContext,
> +                       nsMediaQueryResultCacheKey* = nullptr) const = 0;

Also here for `nsPresContext`.

::: layout/style/MediaList.h:56
(Diff revision 1)
>    nsISupports* GetParentObject() const { return nullptr; }
>  
>    virtual void GetText(nsAString& aMediaText) = 0;
>    virtual void SetText(const nsAString& aMediaText) = 0;
> +  virtual bool Matches(nsPresContext& aPresContext,
> +                       nsMediaQueryResultCacheKey* = nullptr) const = 0;

Probably better having the virtual function without `nsMediaQueryResultCacheKey` and implement it as `Matchs(aPresContext, nullptr)` in `nsMediaList` rather than having the meaningless thing in `ServoMediaList`.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8856328 [details]
Bug 1325878: Use dom::MediaList in the CSS Loader.

https://reviewboard.mozilla.org/r/128226/#review130776

::: layout/style/Loader.cpp:1295
(Diff revision 1)
> -  RefPtr<nsMediaList> mediaList(aMediaList);
> +  RefPtr<MediaList> mediaList(aMediaList);
>  
>    if (!aMediaString.IsEmpty()) {
>      NS_ASSERTION(!aMediaList,
>                   "must not provide both aMediaString and aMediaList");
> -    mediaList = new nsMediaList();
> +    mediaList = MediaList::Create(GetStyleBackendType(), aMediaString);

You should be able to pass `mDocument` in rather than adding a new method I suppose.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8856329 [details]
Bug 1325878: Don't use nsMediaList for loading imports.

https://reviewboard.mozilla.org/r/128228/#review130780

::: servo/components/script/stylesheet_loader.rs:274
(Diff revision 1)
> -        media: MediaList,
> -        make_import: &mut FnMut(MediaList) -> ImportRule,
> +        media: Arc<Locked<MediaList>>,
> +        make_import: &mut FnMut(Arc<Locked<MediaList>>) -> ImportRule,

Probably `StyleLocked` rather than `Locked` given the return type of this function?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Support deep-cloning of ServoMediaLists.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856326 [details]
Bug 1325878: Allow creating empty Servo MediaList.

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856327 [details]
Bug 1325878: Create less hardcoded nsMediaList instances.

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856328 [details]
Bug 1325878: Use dom::MediaList in the CSS Loader.

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856329 [details]
Bug 1325878: Don't use nsMediaList for loading imports.

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

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8856363 [details]
Bug 1325878: Support deep-cloning of ServoMediaLists.

https://reviewboard.mozilla.org/r/128288/#review130812

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Only deep-clone StyleSheet::mMedia when the document changes.

This quite conveniently fixes a Servo crash when double-borrowing the style lock
while inserting an @import rule.

I could tweak the algorithm to avoid the double borrow, but I can also fix this
I guess.

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

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Don't hardode nsMediaList in dom::StyleSheet.

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

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Rewrite insert_rule to avoid lock re-entrancy problems.

In particular, you can insertRule("@import url(\"already-loaded.css\");") and we
would try to clone the stylesheet, which without this change would borrow the
lock for reading.

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

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856326 [details]
Bug 1325878: Allow creating empty Servo MediaList.

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

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856327 [details]
Bug 1325878: Create less hardcoded nsMediaList instances.

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

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856328 [details]
Bug 1325878: Use dom::MediaList in the CSS Loader.

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

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856329 [details]
Bug 1325878: Don't use nsMediaList for loading imports.

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

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856363 [details]
Bug 1325878: Support deep-cloning of ServoMediaLists.

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

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856906 [details]
Bug 1325878: Don't hardode nsMediaList in dom::StyleSheet.

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

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856907 [details]
Bug 1325878: Rewrite insert_rule to avoid lock re-entrancy problems.

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

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Pass the MediaList down to Servo, making <style media> work.

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

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856326 [details]
Bug 1325878: Allow creating empty Servo MediaList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128230/diff/3-4/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856327 [details]
Bug 1325878: Create less hardcoded nsMediaList instances.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128232/diff/3-4/

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856328 [details]
Bug 1325878: Use dom::MediaList in the CSS Loader.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128226/diff/3-4/

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856329 [details]
Bug 1325878: Don't use nsMediaList for loading imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128228/diff/3-4/

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856363 [details]
Bug 1325878: Support deep-cloning of ServoMediaLists.

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

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856906 [details]
Bug 1325878: Don't hardode nsMediaList in dom::StyleSheet.

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

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856907 [details]
Bug 1325878: Rewrite insert_rule to avoid lock re-entrancy problems.

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

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8856906 [details]
Bug 1325878: Don't hardode nsMediaList in dom::StyleSheet.

https://reviewboard.mozilla.org/r/128830/#review131784

::: layout/style/StyleSheet.cpp:694
(Diff revision 3)
>  
>  dom::MediaList*
>  StyleSheet::Media()
>  {
>    if (!mMedia) {
> -    mMedia = new nsMediaList();
> +    mMedia = dom::MediaList::Create(mType);

Given there is only one callsite, I would prefer you just do `MediaList::Create(mDocument, nsString())` here rather than creating a new function specific for this.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1325878: Simplify MediaList creation.

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

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856326 [details]
Bug 1325878: Allow creating empty Servo MediaList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128230/diff/4-5/

Comment 37

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856327 [details]
Bug 1325878: Create less hardcoded nsMediaList instances.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128232/diff/4-5/

Comment 38

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856328 [details]
Bug 1325878: Use dom::MediaList in the CSS Loader.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128226/diff/4-5/

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856329 [details]
Bug 1325878: Don't use nsMediaList for loading imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128228/diff/4-5/

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856363 [details]
Bug 1325878: Support deep-cloning of ServoMediaLists.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128288/diff/3-4/

Comment 41

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856906 [details]
Bug 1325878: Don't hardode nsMediaList in dom::StyleSheet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128830/diff/3-4/

Comment 42

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856907 [details]
Bug 1325878: Rewrite insert_rule to avoid lock re-entrancy problems.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128832/diff/3-4/

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8857236 [details]
Bug 1325878: Pass the MediaList down to Servo, making <style media> work.

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

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8856907 [details]
Bug 1325878: Rewrite insert_rule to avoid lock re-entrancy problems.

https://reviewboard.mozilla.org/r/128832/#review131822

This makes sense, but I guess you would need some further work for script component in Servo side?

Comment 45

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8857236 [details]
Bug 1325878: Pass the MediaList down to Servo, making <style media> work.

https://reviewboard.mozilla.org/r/129178/#review131830

::: layout/style/ServoBindingList.h:32
(Diff revision 2)
>  SERVO_BINDING_FUNC(Servo_StyleSheet_FromUTF8Bytes, RawServoStyleSheetStrong,
>                     mozilla::css::Loader* loader,
>                     mozilla::ServoStyleSheet* gecko_stylesheet,
>                     const nsACString* data,
>                     mozilla::css::SheetParsingMode parsing_mode,
> +                   const RawServoMediaList* media_list,

You probably should use `RawServoMediaListBorrowed` for better FFI safety.

::: servo/ports/geckolib/glue.rs:500
(Diff revision 2)
>  
>      let shared_lock = global_style_data.shared_lock.clone();
> +    let media = if media_list.is_null() {
> +        Arc::new(shared_lock.wrap(MediaList::empty()))
> +    } else {
> +        Locked::<MediaList>::as_arc(unsafe { &&*media_list }).clone()

With `Borrowed` I don't think you need unsafe code here. If you can pass in `nullptr`, probably `BorrowedOrNull`.

Comment 46

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8857329 [details]
Bug 1325878: Simplify MediaList creation.

https://reviewboard.mozilla.org/r/129306/#review131846

Comment 47

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8856906 [details]
Bug 1325878: Don't hardode nsMediaList in dom::StyleSheet.

https://reviewboard.mozilla.org/r/128830/#review131854

Given the simplifying commit, okay.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f360cf839c62
Allow creating empty Servo MediaList. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/73bc24a5bf25
Create less hardcoded nsMediaList instances. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/80f9f184eb40
Use dom::MediaList in the CSS Loader. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/2ea02e45d8fd
Don't use nsMediaList for loading imports. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/aaab36710a8b
Support deep-cloning of ServoMediaLists. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/ce813d02a0f1
Don't hardode nsMediaList in dom::StyleSheet. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/af24b703549a
Pass the MediaList down to Servo, making <style media> work. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/28a047476700
Simplify MediaList creation. r=xidorn

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04083449fbfe
Update test expectations. r=emilio

Comment 50

[Emilio Cobos Álvarez (:emilio)]

Still need to land as a followup two parts of comment 7.

Comment 51

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

https://hg.mozilla.org/mozilla-central/rev/f360cf839c62
https://hg.mozilla.org/mozilla-central/rev/73bc24a5bf25
https://hg.mozilla.org/mozilla-central/rev/80f9f184eb40
https://hg.mozilla.org/mozilla-central/rev/2ea02e45d8fd
https://hg.mozilla.org/mozilla-central/rev/aaab36710a8b
https://hg.mozilla.org/mozilla-central/rev/ce813d02a0f1
https://hg.mozilla.org/mozilla-central/rev/af24b703549a
https://hg.mozilla.org/mozilla-central/rev/28a047476700
https://hg.mozilla.org/mozilla-central/rev/04083449fbfe

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2d795ddbcc9
followup - cleanup MediaList::Matches callers. r=me

Comment 53

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d2d795ddbcc9