1252611 - Hoist more shared machinery from CSSStyleSheet into StyleSheet

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

This will make more stuff work correctly in the servo case.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist mDocument into StyleSheet. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a method to get the StyleSheetInfo for a given StyleSheet. v1

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.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Hoist IsComplete/SetComplete into StyleSheet. v1

Comment 4

[Daniel Holbert [:dholbert]]

[Per IRC, I expect to get to these reviews early next week.]

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Bobby Holley (:bholley)]

(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!

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c6fd46a291e

Comment 9

[Daniel Holbert [:dholbert]]

Makes sense. Thanks!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/298abf6f10c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c49572965427
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0edeb440b7

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/298abf6f10c8
https://hg.mozilla.org/mozilla-central/rev/c49572965427
https://hg.mozilla.org/mozilla-central/rev/7d0edeb440b7