Get rid of StyleSheetHandle and use StyleSheet* directly with branch dispatch

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(11 attachments)

58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
Assignee

Description

3 years ago
No description provided.
Assignee

Updated

3 years ago
Assignee: nobody → 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)
Assignee

Comment 12

3 years ago
Hmmm, I may need to move #include "mozilla/StyleSheetInfo.h" from StyleSheet.h into StyleSheetInlines.h in addition.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

3 years ago
mozreview-review
Comment on attachment 8794080 [details]
Bug 1304302 part 6 - Add cycle collecting support to pointer of StyleSheet.

https://reviewboard.mozilla.org/r/80650/#review79422

So, since I don't quite understand why this is needed, and looks very error prone, r-.

::: layout/style/StyleSheetInlines.h:202
(Diff revision 1)
>  #undef FORWARD
>  #undef FORWARD_CONCRETE
>  
> +inline void
> +ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& aCallback,
> +                            StyleSheet*& aField,

This looks scary. Why to traverse raw pointer? What is the lifetime management here?
Attachment #8794080 - Flags: review?(bugs) → review-

Comment 19

3 years ago
mozreview-review
Comment on attachment 8794081 [details]
Bug 1304302 part 7 - Break cycle reference between SRIMetadata.h and SRICheck.h.

https://reviewboard.mozilla.org/r/80652/#review79428
Attachment #8794081 - Flags: review?(bugs) → review+
Assignee

Comment 20

3 years ago
mozreview-review-reply
Comment on attachment 8794080 [details]
Bug 1304302 part 6 - Add cycle collecting support to pointer of StyleSheet.

https://reviewboard.mozilla.org/r/80650/#review79422

> This looks scary. Why to traverse raw pointer? What is the lifetime management here?

Hmmm, I'm replacing StyleSheetHandle with StyleSheet*, and StyleSheetHandle doesn't seem to be a smart pointer itself either... I guess it is exposing an existing issue :/

I can try whether things work without this part... Would you be fine if only the RefPtr part left?
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 27

3 years ago
mozreview-review
Comment on attachment 8794075 [details]
Bug 1304302 part 1 - Add const version of AsGecko/AsServo to StyleSheet.

https://reviewboard.mozilla.org/r/80640/#review79606
Attachment #8794075 - Flags: review?(cam) → review+

Comment 28

3 years ago
mozreview-review
Comment on attachment 8794076 [details]
Bug 1304302 part 2 - Some small fixes.

https://reviewboard.mozilla.org/r/80642/#review79608

::: layout/style/ServoStyleSheet.cpp:35
(Diff revision 1)
>  }
>  
>  bool
>  ServoStyleSheet::HasRules() const
>  {
> -  return Servo_StyleSheet_HasRules(RawSheet());
> +  return mSheet && Servo_StyleSheet_HasRules(RawSheet());

It's a bit weird to use mSheet in one part of the expression and RawSheet() in the other.  How about we just use mSheet in both places.
Attachment #8794076 - Flags: review?(cam) → review+

Comment 29

3 years ago
mozreview-review
Comment on attachment 8794077 [details]
Bug 1304302 part 3 - Use ServoStyleSheet* instead of Handle in ServoStyleSheet.

https://reviewboard.mozilla.org/r/80644/#review79610
Attachment #8794077 - Flags: review?(cam) → review+

Comment 30

3 years ago
mozreview-review
Comment on attachment 8794078 [details]
Bug 1304302 part 4 - Add all methods StyleSheetHandle needs to StyleSheet.

https://reviewboard.mozilla.org/r/80646/#review79612

::: layout/style/CSSStyleSheet.h:238
(Diff revision 1)
>    // ambiguous with with the XPCOM version.  Just disambiguate.
>    void GetType(nsString& aType) {
>      const_cast<const CSSStyleSheet*>(this)->GetType(aType);
>    }
>    // Our XPCOM GetHref is fine for WebIDL
> -  nsINode* GetOwnerNode() const { return mOwningNode; }
> +  using StyleSheet::GetOwnerNode;

Why do we need this using statement?
Attachment #8794078 - Flags: review?(cam) → review+

Comment 31

3 years ago
mozreview-review
Comment on attachment 8794079 [details]
Bug 1304302 part 5 - Make StyleSheet::As{Gecko,Servo} return pointer instead of reference.

https://reviewboard.mozilla.org/r/80648/#review79620
Attachment #8794079 - Flags: review?(cam) → review+
Assignee

Comment 32

3 years ago
mozreview-review-reply
Comment on attachment 8794078 [details]
Bug 1304302 part 4 - Add all methods StyleSheetHandle needs to StyleSheet.

https://reviewboard.mozilla.org/r/80646/#review79612

> Why do we need this using statement?

Because there is another GetOwnerNode method defined in this class (should be the XPCOM one), so if you do not add this using statement, the WebIDL GetOwnerNode from its base class would not involve overload resolution.

This using statement will likely be removed in bug 1292432.

Comment 33

3 years ago
mozreview-review
Comment on attachment 8794082 [details]
Bug 1304302 part 8 - Change include of {CSS,Servo}StyleSheet.h to StyleSheetInlines.h.

https://reviewboard.mozilla.org/r/80654/#review79624
Attachment #8794082 - Flags: review?(cam) → review+

Comment 34

3 years ago
mozreview-review
Comment on attachment 8794083 [details]
Bug 1304302 part 9 - Make StyleSheet::SheetInfo inline.

https://reviewboard.mozilla.org/r/80656/#review79626
Attachment #8794083 - Flags: review?(cam) → review+

Comment 35

3 years ago
mozreview-review
Comment on attachment 8794084 [details]
Bug 1304302 part 10 - Replace all uses of StyleSheetHandle.

https://reviewboard.mozilla.org/r/80658/#review79628

::: dom/base/nsDocument.cpp:4330
(Diff revision 3)
>  }
>  
> -StyleSheetHandle
> +StyleSheet*
>  nsDocument::GetFirstAdditionalAuthorSheet()
>  {
> -  return mAdditionalSheets[eAuthorSheet].SafeElementAt(0, StyleSheetHandle());
> +  return mAdditionalSheets[eAuthorSheet].SafeElementAt(0, nullptr);

If you want you could rewrite this to leave out the nullptr argument, since that's the default for pointer types.

::: dom/base/nsIDocumentObserver.h:189
(Diff revision 3)
>  #define NS_DECL_NSIDOCUMENTOBSERVER_DOCUMENTSTATESCHANGED                    \
>      virtual void DocumentStatesChanged(nsIDocument* aDocument,               \
>                                         mozilla::EventStates aStateMask) override;
>  
>  #define NS_DECL_NSIDOCUMENTOBSERVER_STYLESHEETADDED                          \
> -    virtual void StyleSheetAdded(mozilla::StyleSheetHandle aStyleSheet,      \
> +    virtual void StyleSheetAdded(mozilla::StyleSheet* aStyleSheet,      \

Nit: feel free to fix the backslash alignment up manually in this patch.
Attachment #8794084 - Flags: review?(cam) → review+

Comment 36

3 years ago
mozreview-review
Comment on attachment 8794085 [details]
Bug 1304302 part 11 - Remove StyleSheetHandle as well as other places reference it.

https://reviewboard.mozilla.org/r/80660/#review79630
Attachment #8794085 - Flags: review?(cam) → review+

Comment 37

3 years ago
mozreview-review
Comment on attachment 8794080 [details]
Bug 1304302 part 6 - Add cycle collecting support to pointer of StyleSheet.

https://reviewboard.mozilla.org/r/80650/#review79684
Attachment #8794080 - Flags: review?(bugs) → review+

Comment 38

3 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf6782170cd
part 1 - Add const version of AsGecko/AsServo to StyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/12e6d7c5a65f
part 2 - Some small fixes. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/de7d09b0a5a5
part 3 - Use ServoStyleSheet* instead of Handle in ServoStyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c390d4c86a2
part 4 - Add all methods StyleSheetHandle needs to StyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec5b85b4507c
part 5 - Make StyleSheet::As{Gecko,Servo} return pointer instead of reference. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/203b90fcacdf
part 6 - Add cycle collecting support to pointer of StyleSheet. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/43bd9a61757d
part 7 - Break cycle reference between SRIMetadata.h and SRICheck.h. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/59bdda2c019e
part 8 - Change include of {CSS,Servo}StyleSheet.h to StyleSheetInlines.h. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfbef1f0933b
part 9 - Make StyleSheet::SheetInfo inline. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/0938bc1e608f
part 10 - Replace all uses of StyleSheetHandle. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fec8d313c49
part 11 - Remove StyleSheetHandle as well as other places reference it. r=heycam
Assignee

Updated

3 years ago
Blocks: 1306212
You need to log in before you can comment on or make changes to this bug.