Closed Bug 1304302 Opened 8 years ago Closed 8 years ago

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

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla52

Tracking Flags:

Tracking

Status

firefox52

---

fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(11 files)

Bug 1304302 part 1 - Add const version of AsGecko/AsServo to StyleSheet. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1304302 part 2 - Some small fixes. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1304302 part 3 - Use ServoStyleSheet* instead of Handle in ServoStyleSheet. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1304302 part 4 - Add all methods StyleSheetHandle needs to StyleSheet. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1304302 part 5 - Make StyleSheet::As{Gecko,Servo} return pointer instead of reference. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1304302 part 6 - Add cycle collecting support to pointer of StyleSheet. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	smaug : review+	Details
Bug 1304302 part 7 - Break cycle reference between SRIMetadata.h and SRICheck.h. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	smaug : review+	Details
Bug 1304302 part 8 - Change include of {CSS,Servo}StyleSheet.h to StyleSheetInlines.h. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1304302 part 9 - Make StyleSheet::SheetInfo inline. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1304302 part 10 - Replace all uses of StyleSheetHandle. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1304302 part 11 - Remove StyleSheetHandle as well as other places reference it. 8 years ago Xidorn Quan [:xidorn] UTC+10 58 bytes, text/x-review-board-request	heycam : review+	Details

Xidorn Quan [:xidorn] UTC+10

Assignee

Description

•

8 years ago

      No description provided.

Xidorn Quan [:xidorn] UTC+10

Assignee

Updated

•

8 years ago

Assignee: nobody → xidorn+moz

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

The methods are written in the same order as in StyleSheetHandle::Ptr.

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

Comment hidden (mozreview-request)

To match the behavior of StyleSheetHandle so that we can simply replace
uses of StyleSheetHandle with StyleSheet* in later patch.

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

Comment hidden (mozreview-request)

This is mostly copied from StyleSheetHandle which is being removed in
later patches. This is a temporary workaround to keep things working.
It should be removed soon when StyleSheet itself becomes a subclass
of nsISupports (which is expected to happen in bug 1292432).

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

It is a preparation for later patch which moves functions from those
classes into StyleSheet. Some of the functions are better defined in
StyleSheetInlines.h.

This commit is generated by the following command:

find . \( -name '*.h' -or -name '*.cpp' \) -not -name '*StyleSheet*' -exec sed -i -b \
  -e '/^#include/ s_/\(CSS\|Servo\)StyleSheet\.h_/StyleSheetInlines.h_' \
  -e '1,\_^#include "mozilla/StyleSheetInlines.h"_ ! { \_^#include "mozilla/StyleSheetInlines.h"_d }' {} +

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

This commit is generated by the following commands:

find . \( -name '*.h' -or -name '*.cpp' \) -not -path './layout/style/StyleSheet*' -exec sed -i -b \
  -e '/^\(#include\|using\)/s/StyleSheetHandle/StyleSheet/g' \
  -e 's/\(mozilla::\)\?StyleSheetHandle::RefPtr/RefPtr<\1StyleSheet>/g' \
  -e 's/StyleSheetHandle()/nullptr/g' \
  -e 's/->AsStyleSheet()//g' \
  -e 's/StyleSheetHandle/StyleSheet*/g' {} +
sed -i -b 's/sheet->AsVoidPtr()/sheet.get()/' layout/style/Loader.cpp
sed -i -b 's/AsHandle()/this/' layout/style/StyleSheet.cpp

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

Comment hidden (mozreview-request)

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

Xidorn Quan [:xidorn] UTC+10

Assignee

Comment 12

•

8 years ago

Hmmm, I may need to move #include "mozilla/StyleSheetInfo.h" from StyleSheet.h into StyleSheetInlines.h in addition.

Xidorn Quan [:xidorn] UTC+10

Assignee

Comment 13

•

8 years ago

This is the try push based on m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af5db07ee087

Comment hidden (mozreview-request)

Comment on attachment 8794083 [details]
Bug 1304302 part 9 - Make StyleSheet::SheetInfo inline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80656/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8794084 [details]
Bug 1304302 part 10 - Replace all uses of StyleSheetHandle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80658/diff/1-2/

Comment hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80660/diff/1-2/

Xidorn Quan [:xidorn] UTC+10

Assignee

Comment 17

•

8 years ago

All builds green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21c8876d55e3

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 18

•

8 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-

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 19

•

8 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+

Xidorn Quan [:xidorn] UTC+10

Assignee

Comment 20

•

8 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 on attachment 8794080 [details]
Bug 1304302 part 6 - Add cycle collecting support to pointer of StyleSheet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80650/diff/1-2/

Comment hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80652/diff/1-2/

Comment hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80654/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8794083 [details]
Bug 1304302 part 9 - Make StyleSheet::SheetInfo inline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80656/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8794084 [details]
Bug 1304302 part 10 - Replace all uses of StyleSheetHandle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80658/diff/2-3/

Comment hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80660/diff/2-3/

Cameron McCormack (:heycam)

Comment 27

•

8 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+

Cameron McCormack (:heycam)

Comment 28

•

8 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+

Cameron McCormack (:heycam)

Comment 29

•

8 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+

Cameron McCormack (:heycam)

Comment 30

•

8 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+

Cameron McCormack (:heycam)

Comment 31

•

8 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+

Xidorn Quan [:xidorn] UTC+10

Assignee

Comment 32

•

8 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.

Cameron McCormack (:heycam)

Comment 33

•

8 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+

Cameron McCormack (:heycam)

Comment 34

•

8 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+

Cameron McCormack (:heycam)

Comment 35

•

8 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+

Cameron McCormack (:heycam)

Comment 36

•

8 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+

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 37

•

8 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+

Pulsebot

Comment 38

•

8 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

Carsten Book [:Tomcat]

Comment 39

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/ddf6782170cd
https://hg.mozilla.org/mozilla-central/rev/12e6d7c5a65f
https://hg.mozilla.org/mozilla-central/rev/de7d09b0a5a5
https://hg.mozilla.org/mozilla-central/rev/3c390d4c86a2
https://hg.mozilla.org/mozilla-central/rev/ec5b85b4507c
https://hg.mozilla.org/mozilla-central/rev/203b90fcacdf
https://hg.mozilla.org/mozilla-central/rev/43bd9a61757d
https://hg.mozilla.org/mozilla-central/rev/59bdda2c019e
https://hg.mozilla.org/mozilla-central/rev/cfbef1f0933b
https://hg.mozilla.org/mozilla-central/rev/0938bc1e608f
https://hg.mozilla.org/mozilla-central/rev/9fec8d313c49

Status: NEW → RESOLVED

Closed: 8 years ago

status-firefox52: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla52

Xidorn Quan [:xidorn] UTC+10

Assignee

Updated

•

8 years ago

Blocks: 1306212

You need to log in before you can comment on or make changes to this bug.