Closed
Bug 1457920
Opened 6 years ago
Closed 6 years ago
Merge ServoStyleSheet and StyleSheet.
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(11 files)
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
No description provided.
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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
mozreview-review |
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 13•6 years ago
|
||
mozreview-review |
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 14•6 years ago
|
||
mozreview-review |
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 15•6 years ago
|
||
mozreview-review |
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 16•6 years ago
|
||
mozreview-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/#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 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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•6 years ago
|
||
mozreview-review |
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 29•6 years ago
|
||
mozreview-review |
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 30•6 years ago
|
||
mozreview-review |
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 31•6 years ago
|
||
mozreview-review |
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 32•6 years ago
|
||
mozreview-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/#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]
Assignee | ||
Comment 33•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d356d330e463094613d2767c85b4ffff4ebfbf9f
Comment 34•6 years ago
|
||
mozreview-review |
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 35•6 years ago
|
||
mozreview-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 36•6 years ago
|
||
mozreview-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 37•6 years ago
|
||
mozreview-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 38•6 years ago
|
||
mozreview-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 39•6 years ago
|
||
mozreview-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 40•6 years ago
|
||
mozreview-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 41•6 years ago
|
||
mozreview-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 42•6 years ago
|
||
mozreview-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 43•6 years ago
|
||
mozreview-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 44•6 years ago
|
||
mozreview-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+
Comment 45•6 years ago
|
||
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
Assignee | ||
Comment 46•6 years ago
|
||
(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.
Comment 47•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e7b20e0b6727 https://hg.mozilla.org/mozilla-central/rev/eebbf9fbebc7 https://hg.mozilla.org/mozilla-central/rev/844f6c4c9356 https://hg.mozilla.org/mozilla-central/rev/424b39cb6769 https://hg.mozilla.org/mozilla-central/rev/39788dcbd008 https://hg.mozilla.org/mozilla-central/rev/7d3b204b45dc https://hg.mozilla.org/mozilla-central/rev/04bb8bf9a707 https://hg.mozilla.org/mozilla-central/rev/174b29e05978 https://hg.mozilla.org/mozilla-central/rev/2de7dbfe200c https://hg.mozilla.org/mozilla-central/rev/af58b003b734 https://hg.mozilla.org/mozilla-central/rev/f823923df3e6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•