Closed Bug 1457920 Opened 2 years ago Closed 2 years ago

Merge ServoStyleSheet and StyleSheet.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

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