Closed Bug 1252611 Opened 4 years ago Closed 4 years ago

Hoist more shared machinery from CSSStyleSheet into StyleSheet

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(3 files)

This will make more stuff work correctly in the servo case.
One of the annoying things about sharing algorithms on the superclass here is that
certain members live on StyleSheet and others live on StyleSheetInfo (a situation
resulting from the split between CSSStyleSheet and CSSStyleSheetInner). This allows
us to write general algorithms on StyleSheet that touch members on StyleSheetInfo
without paying the price of virtual dispatch.
Attachment #8725939 - Flags: review?(dholbert)
[Per IRC, I expect to get to these reviews early next week.]
Attachment #8725938 - Flags: review?(dholbert) → review+
Comment on attachment 8725939 [details] [diff] [review]
Part 2 - Add a method to get the StyleSheetInfo for a given StyleSheet. v1

Review of attachment 8725939 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/StyleSheet.cpp
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "mozilla/ServoStyleSheet.h"
> +#include "mozilla/StyleSheet.h"

Swap the order of these two includes.

This is StyleSheet.cpp, so it should include StyleSheet.h as the very first thing (possibly followed by a blank line, to more clearly separate it from the other includes).

@@ +6,5 @@
>  
> +#include "mozilla/ServoStyleSheet.h"
> +#include "mozilla/StyleSheet.h"
> +#include "mozilla/StyleSheetInlines.h"
> +#include "CSSStyleSheet.h"

Please add a "mozilla/" prefix here, as we do elsewhere when including CSSStyleSheet.h (and for consistency with your other includes here).

::: layout/style/StyleSheet.h
@@ +24,5 @@
>   */
>  class StyleSheet
>  {
>  public:
> +  StyleSheet(StyleBackendType aType);

This constructor needs to be marked as "protected".  We only want it to be able to be invoked from a subclass -- we don't want just anybody to be able to do "new StyleSheet(StyleBackendType::Gecko)".  If they could, then AsGecko()/AsServo() calls on the resulting object would be very bad. (it'd involve a bogus static_cast)

So: consider replacing the "public:" above this with "protected:", and then sticking a "public:" just below this constructor.

@@ +45,5 @@
>    // The document this style sheet is associated with.  May be null
>    nsIDocument* GetDocument() const { return mDocument; }
>  
> +  // Get a handle to the various stylesheet bits which live on the 'inner' for
> +  // GeckoStyleSheets and live on the StyleSheet for Servo StyleSheets.

s/GeckoStyleSheets/Gecko stylesheets/

(Or something like that. "GeckoStyleSheet" sounds like an actual type, but it's not.)

@@ +60,5 @@
> +#endif
> +  }
> +
> +  inline CSSStyleSheet& AsGecko();
> +  inline ServoStyleSheet& AsServo();

Please add a line of documentation for these methods. (In particular, stating that these are only safe to call if the caller is sure that this stylesheet is of the correct type.)

::: layout/style/StyleSheetInlines.h
@@ +7,5 @@
> +#ifndef mozilla_StyleSheetInlines_h
> +#define mozilla_StyleSheetInlines_h
> +
> +#include "mozilla/ServoStyleSheet.h"
> +#include "CSSStyleSheet.h"

Please add a "mozilla/" prefix -- so, this should be:
#include "mozilla/CSSStyleSheet.h"

(for consistency with the line before it, and also because that seems to be the way we include this file everywhere else, too).
Attachment #8725939 - Flags: review?(dholbert) → review+
Comment on attachment 8725941 [details] [diff] [review]
Part 3 - Hoist IsComplete/SetComplete into StyleSheet. v1

Review of attachment 8725941 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, three nits:

::: layout/style/StyleSheet.cpp
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "mozilla/dom/ShadowRoot.h"

As noted in comment 5, this .cpp file's own header (StyleSheet.h) should remain first here, so please make sure this new #include goes below that header.

::: layout/style/StyleSheetHandle.h
@@ +57,5 @@
>        return false;
>  #endif
>      }
>  
> +    inline StyleSheet* AsStyleSheet();

Two things here:

THING #1: Could we just include the impl here in the .h file, rather than abstracting it away to the inlines file?

I'd prefer that, since:
 (a) this can be a one-liner, & hence is easily-includable here, like so:
>    StyleSheet* AsStyleSheet() { return IsServo() ? AsServo() : AsGecko(); }

 (b) this function's friends AsGecko & AsServo each include their impl here, so it's nice for AsStylesheet to be here as well, for consistency.

 (c) This function is qualitatively different from all the other functions in the "inlines" file. (Those other functions all use FORWARD, or are cycle-collection related.  All other StyleSheetHandle method-impls just live directly in this .h file.)

===

THING #2: Perhaps we should add a "const StyleSheet* AsStyleSheet() const" accessor, for completeness? (We have that const accessors for AsGecko & AsServo -- so, seems like AsStyleSheet should have a const version as well, alongside those.)  It could be a copypasted version of the const AsGecko() accessor, but with the assertion removed and with s/Gecko/StyleSheet/.
Attachment #8725941 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #6)
> THING #1: Could we just include the impl here in the .h file, rather than
> abstracting it away to the inlines file?

Unfortunately not, because the knowledge that both ServoStyleSheet and CSSStyleSheet implicitly convert to StyleSheet depends on including both CSSStyleStyleSheet.h and ServoStyleSheet.h, which we do in *Inlines.h but not the regular header.

> I'd prefer that, since:
>  (a) this can be a one-liner, & hence is easily-includable here, like so:
> >    StyleSheet* AsStyleSheet() { return IsServo() ? AsServo() : AsGecko(); }

It can't quite, because the ternary operator doesn't work with types that aren't implicitly convertible to each other, hence the |if|.

> THING #2: Perhaps we should add a "const StyleSheet* AsStyleSheet() const"
> accessor, for completeness? (We have that const accessors for AsGecko &
> AsServo -- so, seems like AsStyleSheet should have a const version as well,
> alongside those.) 

It costs more lines given the above, but I can do it anyway.

Addressed all other comments. Thanks for the reviews!
Makes sense. Thanks!
You need to log in before you can comment on or make changes to this bug.