stylo: Implement Servo-backed nsMediaList

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
(Assignee)

Description

6 months ago
This is required for feature-parity between Stylo and Gecko in @import and media queries.
(Assignee)

Comment 1

6 months ago
Some initial refactoring in https://github.com/servo/servo/pull/14739
(Assignee)

Updated

6 months ago
Blocks: 1290228
(Assignee)

Updated

5 months ago
Depends on: 1331213
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

3 months ago
mozreview-review
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?
Attachment #8856326 - Flags: review?(xidorn+moz) → review+

Comment 7

3 months ago
mozreview-review
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`.
Attachment #8856327 - Flags: review?(xidorn+moz) → review+

Comment 8

3 months ago
mozreview-review
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.
Attachment #8856328 - Flags: review?(xidorn+moz) → review+

Comment 9

3 months ago
mozreview-review
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?
Attachment #8856329 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

3 months ago
mozreview-review
Comment on attachment 8856363 [details]
Bug 1325878: Support deep-cloning of ServoMediaLists.

https://reviewboard.mozilla.org/r/128288/#review130812
Attachment #8856363 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8856905 - Attachment is obsolete: true
Attachment #8856905 - Flags: review?(cam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 34

2 months ago
mozreview-review
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.
Attachment #8856906 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 44

2 months ago
mozreview-review
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?
Attachment #8856907 - Flags: review?(xidorn+moz) → review+

Comment 45

2 months ago
mozreview-review
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`.
Attachment #8857236 - Flags: review?(xidorn+moz) → review+

Comment 46

2 months ago
mozreview-review
Comment on attachment 8857329 [details]
Bug 1325878: Simplify MediaList creation.

https://reviewboard.mozilla.org/r/129306/#review131846
Attachment #8857329 - Flags: review?(xidorn+moz) → review+

Comment 47

2 months ago
mozreview-review
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.
Attachment #8856906 - Flags: review?(xidorn+moz) → review+

Comment 48

2 months ago
Pushed by ecoal95@gmail.com:
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
(Assignee)

Updated

2 months ago
Depends on: 1355900

Comment 49

2 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/04083449fbfe
Update test expectations. r=emilio
(Assignee)

Comment 50

2 months ago
Still need to land as a followup two parts of comment 7.
Flags: needinfo?(emilio+bugs)

Comment 51

2 months ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 52

2 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d2d795ddbcc9
followup - cleanup MediaList::Matches callers. r=me
(Assignee)

Updated

2 months ago
Flags: needinfo?(emilio+bugs)
https://hg.mozilla.org/mozilla-central/rev/d2d795ddbcc9
You need to log in before you can comment on or make changes to this bug.