stylo: Set metadata on servo style sheets

RESOLVED FIXED in Firefox 53

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Bobby Holley (On Leave Until June 11th), Assigned: heycam)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(9 attachments)

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
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
in css::Loader::PrepareSheet, we currently bail out for servo style sheets, which causes us to lose the title, media string, etc. This presumably will cause observable (but minor) correctness issues, probably mostly when inspecting via CSSOM.
(Assignee)

Updated

a year ago
Blocks: 1323706
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 10

a year ago
mozreview-review
Comment on attachment 8823993 [details]
Bug 1290209 - Part 1: Move CSSStyleSheet::SetTitle up to StyleSheet.

https://reviewboard.mozilla.org/r/102478/#review102824

::: layout/style/Loader.cpp:1309
(Diff revision 1)
>      mediumParser.ParseMediaList(aMediaString, nullptr, 0, mediaList);
>    }
>  
>    sheet->SetMedia(mediaList);
>  
> -  sheet->SetTitle(aTitle);
> +  aSheet->SetTitle(aTitle);

This change doesn't seem to be necessary.
Attachment #8823993 - Flags: review?(xidorn+moz) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8823994 [details]
Bug 1290209 - Part 2: Remove unused friend declaration.

https://reviewboard.mozilla.org/r/102480/#review102826
Attachment #8823994 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8823993 [details]
Bug 1290209 - Part 1: Move CSSStyleSheet::SetTitle up to StyleSheet.

https://reviewboard.mozilla.org/r/102478/#review102824

> This change doesn't seem to be necessary.

By the end of the patch queue we should be doing most of these calls through aSheet, which is a StyleSheet*, and the ->AsGecko() is used only for the scope element thing.  So I thought I'd make these changes as I went.

Comment 13

a year ago
mozreview-review
Comment on attachment 8823995 [details]
Bug 1290209 - Part 3: Move media queries related class implementations from CSSStyleSheet.cpp to nsMediaList.cpp.

https://reviewboard.mozilla.org/r/102482/#review102832
Attachment #8823995 - Flags: review?(xidorn+moz) → review+

Comment 14

a year ago
mozreview-review
Comment on attachment 8823996 [details]
Bug 1290209 - Part 4: Delete trailing spaces in nsMediaList.cpp.

https://reviewboard.mozilla.org/r/102484/#review102834
Attachment #8823996 - Flags: review?(xidorn+moz) → review+

Comment 15

a year ago
mozreview-review
Comment on attachment 8823997 [details]
Bug 1290209 - Part 5: Change nsMediaList's style sheet pointer from a CSSStyleSheet to a StyleSheet.

https://reviewboard.mozilla.org/r/102486/#review102836
Attachment #8823997 - Flags: review?(xidorn+moz) → review+

Comment 16

a year ago
mozreview-review
Comment on attachment 8823999 [details]
Bug 1290209 - Part 7: Rename an argument.

https://reviewboard.mozilla.org/r/102490/#review102840
Attachment #8823999 - Flags: review?(xidorn+moz) → review+

Comment 17

a year ago
mozreview-review-reply
Comment on attachment 8823993 [details]
Bug 1290209 - Part 1: Move CSSStyleSheet::SetTitle up to StyleSheet.

https://reviewboard.mozilla.org/r/102478/#review102824

> By the end of the patch queue we should be doing most of these calls through aSheet, which is a StyleSheet*, and the ->AsGecko() is used only for the scope element thing.  So I thought I'd make these changes as I went.

OK.

Comment 18

a year ago
mozreview-review
Comment on attachment 8824000 [details]
Bug 1290209 - Part 8: Move CSSStyleSheet::SetEnabled up to StyleSheet.

https://reviewboard.mozilla.org/r/102492/#review102844
Attachment #8824000 - Flags: review?(xidorn+moz) → review+

Comment 19

a year ago
mozreview-review
Comment on attachment 8824001 [details]
Bug 1290209 - Part 9: Don't return early from Loader::PrepareSheet for a ServoStyleSheet.

https://reviewboard.mozilla.org/r/102494/#review102846
Attachment #8824001 - Flags: review?(xidorn+moz) → review+

Comment 20

a year ago
mozreview-review
Comment on attachment 8823998 [details]
Bug 1290209 - Part 6: Move CSSStyleSheet::mMedia up to StyleSheet.

https://reviewboard.mozilla.org/r/102488/#review102848

I'm not familiar with cycle collection... so redirect to bz.

::: layout/style/StyleSheet.cpp:64
(Diff revision 1)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(StyleSheet)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMedia)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(StyleSheet)
> +  tmp->DropMedia();
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(StyleSheet)
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END

It seems to me this is not an expansion of `NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0` with the new `mMedia` added. I don't quite have idea what does it mean.
Attachment #8823998 - Flags: review?(xidorn+moz)
Attachment #8823998 - Flags: review?(bzbarsky)
(Assignee)

Comment 21

a year ago
mozreview-review-reply
Comment on attachment 8823998 [details]
Bug 1290209 - Part 6: Move CSSStyleSheet::mMedia up to StyleSheet.

https://reviewboard.mozilla.org/r/102488/#review102848

> It seems to me this is not an expansion of `NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0` with the new `mMedia` added. I don't quite have idea what does it mean.

FWIW I just moved the unlinking behaviour that we had in CSSStyleSheet up here into StyleSheet.  DropMedia just clears the back pointer from the nsMediaList to the StyleSheet.
Comment on attachment 8823998 [details]
Bug 1290209 - Part 6: Move CSSStyleSheet::mMedia up to StyleSheet.

https://reviewboard.mozilla.org/r/102488/#review103264

r=me

::: layout/style/StyleSheet.cpp:66
(Diff revision 1)
>  
> -NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(StyleSheet)
> +NS_IMPL_CYCLE_COLLECTION_CLASS(StyleSheet)
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(StyleSheet)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMedia)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS isn't a thing on inbound/m-c anymore.  It got removed in bug 1326507.  So if you plan to land this on inbound, remove that line.

::: layout/style/StyleSheet.cpp:74
(Diff revision 1)
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(StyleSheet)
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END

You can replace these three lines with:

    NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(StyleSheet)
Attachment #8823998 - Flags: review?(bzbarsky) → review+

Comment 23

a year ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6bbf6f71474
Part 1: Move CSSStyleSheet::SetTitle up to StyleSheet. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a7defa6ed44
Part 2: Remove unused friend declaration. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ccf8287f2c9
Part 3: Move media queries related class implementations from CSSStyleSheet.cpp to nsMediaList.cpp. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5f91da5250f
Part 4: Delete trailing spaces in nsMediaList.cpp. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/91929e676b48
Part 5: Change nsMediaList's style sheet pointer from a CSSStyleSheet to a StyleSheet. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffafd1ed4e05
Part 6: Move CSSStyleSheet::mMedia up to StyleSheet. r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/8639e5953152
Part 7: Rename an argument. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/19fcced7d097
Part 8: Move CSSStyleSheet::SetEnabled up to StyleSheet. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/613c5d1ec7b9
Part 9: Don't return early from Loader::PrepareSheet for a ServoStyleSheet. r=xidorn
Assignee: nobody → cam
You need to log in before you can comment on or make changes to this bug.