Closed Bug 1457920 Opened 7 years ago Closed 7 years ago

Merge ServoStyleSheet and StyleSheet.

Categories

(Core :: CSS Parsing and Computation, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(11 files)

Bug 1457920: Merge ServoStyleSheet and StyleSheet.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Remove FORWARD_INTERNAL.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Remove EnabledStateChanged.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Remove DidDirty.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Remove StyleSheet::AsServo.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Move stuff from ServoStyleSheet.cpp to StyleSheet.cpp.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Remove ServoStyleSheet usage.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Remove ServoStyleSheet.{h,cpp}.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Trivially inline a bunch of methods that had no reason for not being inlined.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Cleanup a useless argument in Loader.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457920: Loader::ParseSheet is not really fallible.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
No description provided.
Comment on attachment 8972075 [details] Bug 1457920: Merge ServoStyleSheet and StyleSheet. https://reviewboard.mozilla.org/r/240798/#review246524 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 1) > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > - virtual ~StyleSheet(); > > + virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972077 [details] Bug 1457920: Remove EnabledStateChanged. https://reviewboard.mozilla.org/r/240802/#review246526 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 1) > StyleSheet* aParentToUse, > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > > virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972078 [details] Bug 1457920: Remove DidDirty. https://reviewboard.mozilla.org/r/240804/#review246528 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 1) > StyleSheet* aParentToUse, > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > > virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972079 [details] Bug 1457920: Remove StyleSheet::AsServo. https://reviewboard.mozilla.org/r/240806/#review246532 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 1) > StyleSheet* aParentToUse, > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > > virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972083 [details] Bug 1457920: Trivially inline a bunch of methods that had no reason for not being inlined. https://reviewboard.mozilla.org/r/240814/#review246536 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 1) > StyleSheet* aParentToUse, > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > > virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972075 [details] Bug 1457920: Merge ServoStyleSheet and StyleSheet. https://reviewboard.mozilla.org/r/240798/#review246578 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 2) > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > - virtual ~StyleSheet(); > > + virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972077 [details] Bug 1457920: Remove EnabledStateChanged. https://reviewboard.mozilla.org/r/240802/#review246580 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 2) > StyleSheet* aParentToUse, > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > > virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972078 [details] Bug 1457920: Remove DidDirty. https://reviewboard.mozilla.org/r/240804/#review246582 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 2) > StyleSheet* aParentToUse, > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > > virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972079 [details] Bug 1457920: Remove StyleSheet::AsServo. https://reviewboard.mozilla.org/r/240806/#review246586 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 2) > StyleSheet* aParentToUse, > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > > virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972083 [details] Bug 1457920: Trivially inline a bunch of methods that had no reason for not being inlined. https://reviewboard.mozilla.org/r/240814/#review246588 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/StyleSheet.h:64 (Diff revision 2) > StyleSheet* aParentToUse, > dom::CSSImportRule* aOwnerRuleToUse, > nsIDocument* aDocumentToUse, > nsINode* aOwningNodeToUse); > > virtual ~StyleSheet() final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]
Comment on attachment 8972075 [details] Bug 1457920: Merge ServoStyleSheet and StyleSheet. https://reviewboard.mozilla.org/r/240798/#review246644 ::: layout/style/StyleSheetInfo.h:48 (Diff revision 2) > + // FIXME(emilio): most of this struct should be const, then we can remove the > + // UrlExtraData member and such. I think the direction is the reverse, actually. We need to pass in `URLExtraData` for parsing, and some value needs to hold that struct, so we probably want to remove the URI member rather than the `URLExtraData` member, unless you want everything which is currently holding `URLExtraData` to instead hold three separate refcounted objects.
Attachment #8972075 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972076 [details] Bug 1457920: Remove FORWARD_INTERNAL. https://reviewboard.mozilla.org/r/240800/#review246852 ::: layout/style/StyleSheet.cpp:468 (Diff revision 2) > ErrorResult& aRv) > { > if (!AreRulesAvailable(aSubjectPrincipal, aRv)) { > return nullptr; > } > - FORWARD_INTERNAL(GetCssRulesInternal, ()) > + return GetCssRulesInternal(); I guess we should probably merge the internal functions into these directly.
Attachment #8972076 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972077 [details] Bug 1457920: Remove EnabledStateChanged. https://reviewboard.mozilla.org/r/240802/#review246854
Attachment #8972077 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972078 [details] Bug 1457920: Remove DidDirty. https://reviewboard.mozilla.org/r/240804/#review246856
Attachment #8972078 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972079 [details] Bug 1457920: Remove StyleSheet::AsServo. https://reviewboard.mozilla.org/r/240806/#review246858
Attachment #8972079 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972080 [details] Bug 1457920: Move stuff from ServoStyleSheet.cpp to StyleSheet.cpp. https://reviewboard.mozilla.org/r/240808/#review246860
Attachment #8972080 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972081 [details] Bug 1457920: Remove ServoStyleSheet usage. https://reviewboard.mozilla.org/r/240810/#review246862
Attachment #8972081 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972082 [details] Bug 1457920: Remove ServoStyleSheet.{h,cpp}. https://reviewboard.mozilla.org/r/240812/#review246864
Attachment #8972082 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972083 [details] Bug 1457920: Trivially inline a bunch of methods that had no reason for not being inlined. https://reviewboard.mozilla.org/r/240814/#review246866 ::: layout/style/StyleSheet.h (Diff revision 2) > - nsINode* GetOwnerNode() const { return mOwningNode; } > - inline StyleSheet* GetParentSheet() const { return mParent; } > > + nsINode* GetOwnerNode() const > + { > + return mOwningNode; > + } > + > + StyleSheet* GetParentSheet() const > + { > + return mParent; > + } I believe we have code style guidelines which explicitly allow single line method definition inside class definitions. That's more compact and shouldn't affect readability, so it is unnecessary to expand it. ::: layout/style/StyleSheet.cpp:10 (Diff revision 2) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "mozilla/StyleSheet.h" > > #include "mozilla/ComputedStyleInlines.h" > +#include "mozilla/css/ErrorReporter.h" It's not clear to me why do you need to add this include. It doesn't seem to me there is any change here requires this. ::: layout/style/StyleSheet.cpp:20 (Diff revision 2) > #include "mozilla/dom/MediaList.h" > #include "mozilla/dom/ShadowRoot.h" > #include "mozilla/dom/ShadowRootBinding.h" > #include "mozilla/ServoCSSRuleList.h" > #include "mozilla/ServoStyleSet.h" > +#include "mozilla/StaticPrefs.h" Ditto.
Attachment #8972083 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972084 [details] Bug 1457920: Cleanup a useless argument in Loader. https://reviewboard.mozilla.org/r/240816/#review246868
Attachment #8972084 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8972085 [details] Bug 1457920: Loader::ParseSheet is not really fallible. https://reviewboard.mozilla.org/r/240818/#review246870
Attachment #8972085 - Flags: review?(xidorn+moz) → review+
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b20e0b6727 Merge ServoStyleSheet and StyleSheet. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/eebbf9fbebc7 Remove FORWARD_INTERNAL. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/844f6c4c9356 Remove EnabledStateChanged. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/424b39cb6769 Remove DidDirty. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/39788dcbd008 Remove StyleSheet::AsServo. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3b204b45dc Move stuff from ServoStyleSheet.cpp to StyleSheet.cpp. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/04bb8bf9a707 Remove ServoStyleSheet usage. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/174b29e05978 Remove ServoStyleSheet.{h,cpp}. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/2de7dbfe200c Trivially inline a bunch of methods that had no reason for not being inlined. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/af58b003b734 Cleanup a useless argument in Loader. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/f823923df3e6 Loader::ParseSheet is not really fallible. r=xidorn
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #34) > Comment on attachment 8972075 [details] > Bug 1457920: Merge ServoStyleSheet and StyleSheet. > > https://reviewboard.mozilla.org/r/240798/#review246644 > > ::: layout/style/StyleSheetInfo.h:48 > (Diff revision 2) > > + // FIXME(emilio): most of this struct should be const, then we can remove the > > + // UrlExtraData member and such. > > I think the direction is the reverse, actually. We need to pass in > `URLExtraData` for parsing, and some value needs to hold that struct, so we > probably want to remove the URI member rather than the `URLExtraData` > member, unless you want everything which is currently holding `URLExtraData` > to instead hold three separate refcounted objects. Yeah, agreed, reworded. (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #42) > Comment on attachment 8972083 [details] > Bug 1457920: Trivially inline a bunch of methods that had no reason for not > being inlined. > > https://reviewboard.mozilla.org/r/240814/#review246866 > > ::: layout/style/StyleSheet.h > (Diff revision 2) > > - nsINode* GetOwnerNode() const { return mOwningNode; } > > - inline StyleSheet* GetParentSheet() const { return mParent; } > > > > + nsINode* GetOwnerNode() const > > + { > > + return mOwningNode; > > + } > > + > > + StyleSheet* GetParentSheet() const > > + { > > + return mParent; > > + } > > I believe we have code style guidelines which explicitly allow single line > method definition inside class definitions. That's more compact and > shouldn't affect readability, so it is unnecessary to expand it. > > ::: layout/style/StyleSheet.cpp:10 > (Diff revision 2) > > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > #include "mozilla/StyleSheet.h" > > > > #include "mozilla/ComputedStyleInlines.h" > > +#include "mozilla/css/ErrorReporter.h" > > It's not clear to me why do you need to add this include. It doesn't seem to > me there is any change here requires this. > > ::: layout/style/StyleSheet.cpp:20 > (Diff revision 2) > > #include "mozilla/dom/MediaList.h" > > #include "mozilla/dom/ShadowRoot.h" > > #include "mozilla/dom/ShadowRootBinding.h" > > #include "mozilla/ServoCSSRuleList.h" > > #include "mozilla/ServoStyleSet.h" > > +#include "mozilla/StaticPrefs.h" > > Ditto. These are unified build bustage from removing the header / cpp files. I'll move them to the appropriate commit.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: