Closed Bug 1290209 Opened 7 years ago Closed 7 years ago

stylo: Set metadata on servo style sheets

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: bholley, Assigned: heycam)

References

Details

Attachments

(9 files)

Bug 1290209 - Part 1: Move CSSStyleSheet::SetTitle up to StyleSheet.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1290209 - Part 2: Remove unused friend declaration.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1290209 - Part 3: Move media queries related class implementations from CSSStyleSheet.cpp to nsMediaList.cpp.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1290209 - Part 4: Delete trailing spaces in nsMediaList.cpp.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1290209 - Part 5: Change nsMediaList's style sheet pointer from a CSSStyleSheet to a StyleSheet.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1290209 - Part 6: Move CSSStyleSheet::mMedia up to StyleSheet.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1290209 - Part 7: Rename an argument.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1290209 - Part 8: Move CSSStyleSheet::SetEnabled up to StyleSheet.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1290209 - Part 9: Don't return early from Loader::PrepareSheet for a ServoStyleSheet.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
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.
Blocks: 1323706
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 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+
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 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 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 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 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 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 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 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 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)
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+
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
Depends on: 1336223
You need to log in before you can comment on or make changes to this bug.