Closed Bug 1292432 Opened 8 years ago Closed 8 years ago

Stylo: Implement CSSStyleSheet interface

Categories

(Core :: DOM: CSS Object Model, defect)

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(10 files)

Bug 1292432 part 1 - Make StyleSheet.mType be const.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292432 part 2 - Make StyleSheet inherit nsIDOMCSSStyleSheet.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292432 part 3 - Add WillDirty and DidDirty to StyleSheet.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292432 part 4 - Move SubjectSubsumesInnerPrincipal to StyleSheet.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292432 part 5 - Unify completeness check and security check to StyleSheet.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292432 part 6 - Make StyleSheet implement GetCssRules/InsertRule/DeleteRule.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292432 part 7 - Move other WebIDL methods to StyleSheet.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292432 part 8 - Move XPCOM IDL methods which just call WebIDL methods to StyleSheet.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292432 part 9 - Change WebIDL interface of CSSStyleSheet to StyleSheet.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292432 part 10 - Make style/link elements return StyleSheet.
58 bytes, text/x-review-board-request
heycam
: review+
Details 
      No description provided.
Assignee: nobody → xidorn+moz
Depends on: 1304302
Blocks: 1307357
To actually implement the methods, we would need to implement interfaces for CSS rules, which would be done in bug 1307357.
Hmmm, still some oranges and compiler failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f9fdf7f3e5b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc7eb392e218
Comment on attachment 8797546 [details]
Bug 1292432 part 1 - Make StyleSheet.mType be const.

https://reviewboard.mozilla.org/r/83008/#review84042
Attachment #8797546 - Flags: review?(cam) → review+
Comment on attachment 8797547 [details]
Bug 1292432 part 2 - Make StyleSheet inherit nsIDOMCSSStyleSheet.

https://reviewboard.mozilla.org/r/83010/#review84046

::: layout/style/StyleSheet.h:140
(Diff revision 2)
> +  // The XPCOM GetType is fine for WebIDL.
> +  // The XPCOM GetHref is fine for WebIDL
> +  // GetOwnerNode is defined above.
> +  // The XPCOM GetTitle is fine for WebIDL.
> +  bool Disabled() const { return mDisabled; }
> +  // The XPCOM SetDisabled is fine for WebIDL

Nit: "." at the end of the comment?

Or feel free to join those four XPCOM comments into one sentence.

::: layout/style/StyleSheet.cpp:104
(Diff revision 2)
> +  // DOM method, so handle BeginUpdate/EndUpdate
> +  MOZ_AUTO_DOC_UPDATE(mDocument, UPDATE_STYLE, true);
> +  if (IsGecko()) {
> +    AsGecko()->SetEnabled(!aDisabled);
> +  } else {
> +    MOZ_CRASH("Unimplemented SetEnabled for stylo");

Nit: "stylo: unimplemented SetEnabled" for consistency with other stylo-specific crashes/assertions/warnings.
Attachment #8797547 - Flags: review?(cam) → review+
Comment on attachment 8797548 [details]
Bug 1292432 part 3 - Add WillDirty and DidDirty to StyleSheet.

https://reviewboard.mozilla.org/r/83012/#review84050
Attachment #8797548 - Flags: review?(cam) → review+
Comment on attachment 8797549 [details]
Bug 1292432 part 4 - Move SubjectSubsumesInnerPrincipal to StyleSheet.

https://reviewboard.mozilla.org/r/83014/#review84052
Attachment #8797549 - Flags: review?(cam) → review+
Comment on attachment 8797550 [details]
Bug 1292432 part 5 - Unify completeness check and security check to StyleSheet.

https://reviewboard.mozilla.org/r/83016/#review84054

::: layout/style/StyleSheet.h:163
(Diff revision 2)
> +  // It does the security check as well as whether the rules have been
> +  // completely loaded.
> +  bool AreRulesAvailable(const Maybe<nsIPrincipal*>& aSubjectPrincipal,

Maybe mention in the comment here that the ErrorResult will have an exception set on it if this function returns false.
Attachment #8797550 - Flags: review?(cam) → review+
Comment on attachment 8797551 [details]
Bug 1292432 part 6 - Make StyleSheet implement GetCssRules/InsertRule/DeleteRule.

https://reviewboard.mozilla.org/r/83018/#review84056

::: layout/style/StyleSheet.h:29
(Diff revision 2)
>  class ServoStyleSheet;
>  class StyleSheetInfo;
>  
>  namespace dom {
>  class SRIMetadata;
> +class CSSRuleList;

Nit: one line up to keep this sorted.
Attachment #8797551 - Flags: review?(cam) → review+
Comment on attachment 8797552 [details]
Bug 1292432 part 7 - Move other WebIDL methods to StyleSheet.

https://reviewboard.mozilla.org/r/83020/#review84062

::: layout/style/StyleSheet.h:140
(Diff revision 3)
>  
>    // WebIDL StyleSheet API
>    // The XPCOM GetType is fine for WebIDL.
>    // The XPCOM GetHref is fine for WebIDL
>    // GetOwnerNode is defined above.
> +  inline StyleSheet* GetParentStyleSheet() const;

Since the body of this function is so simple you might want to just define it inline here.

::: layout/style/StyleSheet.h:147
(Diff revision 3)
> +  virtual nsMediaList* Media() = 0;
>    bool Disabled() const { return mDisabled; }
>    // The XPCOM SetDisabled is fine for WebIDL
>  
>    // WebIDL CSSStyleSheet API
> +  virtual nsIDOMCSSRule* GetDOMOwnerRule() const = 0;

I wonder if we should move the binary name definition from Bindings.conf to a [BinaryName=...] in the .webidl file.  I forgot for a moment it could be in Bindings.conf too, and got a bit confused...
Attachment #8797552 - Flags: review?(cam) → review+
Comment on attachment 8797553 [details]
Bug 1292432 part 8 - Move XPCOM IDL methods which just call WebIDL methods to StyleSheet.

https://reviewboard.mozilla.org/r/83022/#review84066

::: layout/style/StyleSheet.cpp:14
(Diff revision 3)
>  #include "mozilla/dom/ShadowRoot.h"
>  #include "mozilla/ServoStyleSheet.h"
>  #include "mozilla/StyleSheetInlines.h"
>  #include "mozilla/CSSStyleSheet.h"
>  
> +#include "mozilla/dom/CSSRuleList.h"

Nit: Put this up in the block above?
Attachment #8797553 - Flags: review?(cam) → review+
Comment on attachment 8797554 [details]
Bug 1292432 part 9 - Change WebIDL interface of CSSStyleSheet to StyleSheet.

https://reviewboard.mozilla.org/r/83024/#review84068

::: layout/style/CSSStyleSheet.cpp:1247
(Diff revision 3)
> -NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(CSSStyleSheet)
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(CSSStyleSheet)
> -  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> -  NS_INTERFACE_MAP_ENTRY(nsIDOMStyleSheet)
> -  NS_INTERFACE_MAP_ENTRY(nsIDOMCSSStyleSheet)
>    NS_INTERFACE_MAP_ENTRY(nsICSSLoaderObserver)
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMCSSStyleSheet)

I wonder if it's better to write this as:

  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, StyleSheet)

since that's kind of the minimal casting required to get from CSSStyleSheet to a type that is unambiguously castable to nsISupports.

::: layout/style/ServoStyleSheet.h:16
(Diff revision 3)
>  #include "mozilla/RefPtr.h"
>  #include "mozilla/ServoBindingHelpers.h"
>  #include "mozilla/StyleSheet.h"
>  #include "mozilla/StyleSheetInfo.h"
>  #include "nsStringFwd.h"
> +#include "nsCycleCollectionParticipant.h"

If we do need this, I guess it can go in the .cpp file, but...

::: layout/style/ServoStyleSheet.h:32
(Diff revision 3)
>    ServoStyleSheet(css::SheetParsingMode aParsingMode,
>                    CORSMode aCORSMode,
>                    net::ReferrerPolicy aReferrerPolicy,
>                    const dom::SRIMetadata& aIntegrity);
>  
> -  NS_DECL_ISUPPORTS
> +  NS_DECL_ISUPPORTS_INHERITED

...do we even need this and the macros in ServoStyleSheet.cpp now that the superclass is the thing that implements all of the XPCOM interfaces?
Attachment #8797554 - Flags: review?(cam) → review+
Comment on attachment 8797555 [details]
Bug 1292432 part 10 - Make style/link elements return StyleSheet.

https://reviewboard.mozilla.org/r/83238/#review84082
Attachment #8797555 - Flags: review?(cam) → review+
Comment on attachment 8797552 [details]
Bug 1292432 part 7 - Move other WebIDL methods to StyleSheet.

https://reviewboard.mozilla.org/r/83020/#review84062

> Since the body of this function is so simple you might want to just define it inline here.

You can't, because some compilers would complain that it is used but never defined for files which include StyleSheet.h but not StyleSheetInlines.h.
Comment on attachment 8797554 [details]
Bug 1292432 part 9 - Change WebIDL interface of CSSStyleSheet to StyleSheet.

https://reviewboard.mozilla.org/r/83024/#review84068

> ...do we even need this and the macros in ServoStyleSheet.cpp now that the superclass is the thing that implements all of the XPCOM interfaces?

Not strictly necessary. This can help tracking addref/release on this particular type. But I guess removing it is also fine.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25a49da5b2af
part 1 - Make StyleSheet.mType be const. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e8b25c9e07
part 2 - Make StyleSheet inherit nsIDOMCSSStyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f810ce05dc
part 3 - Add WillDirty and DidDirty to StyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba26eae57658
part 4 - Move SubjectSubsumesInnerPrincipal to StyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/0366529b94f1
part 5 - Unify completeness check and security check to StyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8cac9478593
part 6 - Make StyleSheet implement GetCssRules/InsertRule/DeleteRule. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2617bfd8c4d4
part 7 - Move other WebIDL methods to StyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/5deaf3486070
part 8 - Move XPCOM IDL methods which just call WebIDL methods to StyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7fceba0c1b
part 9 - Change WebIDL interface of CSSStyleSheet to StyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ddd42da3c43
part 10 - Make style/link elements return StyleSheet. r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: