1290209 - stylo: Set metadata on servo style sheets

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1290209 - Part 1: Move CSSStyleSheet::SetTitle up to StyleSheet.

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

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1290209 - Part 2: Remove unused friend declaration.

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

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1290209 - Part 3: Move media queries related class implementations from CSSStyleSheet.cpp to nsMediaList.cpp.

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

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1290209 - Part 4: Delete trailing spaces in nsMediaList.cpp.

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Bug 1290209 - Part 5: Change nsMediaList's style sheet pointer from a CSSStyleSheet to a StyleSheet.

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

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Bug 1290209 - Part 6: Move CSSStyleSheet::mMedia up to StyleSheet.

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

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1290209 - Part 7: Rename an argument.

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

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1290209 - Part 8: Move CSSStyleSheet::SetEnabled up to StyleSheet.

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

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1290209 - Part 9: Don't return early from Loader::PrepareSheet for a ServoStyleSheet.

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8823994 [details]
Bug 1290209 - Part 2: Remove unused friend declaration.

https://reviewboard.mozilla.org/r/102480/#review102826

Comment 12

[Cameron McCormack (:heycam)]

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

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

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

https://reviewboard.mozilla.org/r/102484/#review102834

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8823999 [details]
Bug 1290209 - Part 7: Rename an argument.

https://reviewboard.mozilla.org/r/102490/#review102840

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

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

[Xidorn Quan [:xidorn] UTC+10]

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

https://reviewboard.mozilla.org/r/102492/#review102844

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 21

[Cameron McCormack (:heycam)]

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 22

[Boris Zbarsky [:bzbarsky]]

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)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/e6bbf6f71474
https://hg.mozilla.org/mozilla-central/rev/6a7defa6ed44
https://hg.mozilla.org/mozilla-central/rev/5ccf8287f2c9
https://hg.mozilla.org/mozilla-central/rev/d5f91da5250f
https://hg.mozilla.org/mozilla-central/rev/91929e676b48
https://hg.mozilla.org/mozilla-central/rev/ffafd1ed4e05
https://hg.mozilla.org/mozilla-central/rev/8639e5953152
https://hg.mozilla.org/mozilla-central/rev/19fcced7d097
https://hg.mozilla.org/mozilla-central/rev/613c5d1ec7b9