Convert CSSStyleSheet to Paris bindings

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Reporter

Description

6 years ago
No description provided.
Andrew, are you doing this?  If not, I was planning to.
Reporter

Comment 2

6 years ago
I hadn't started on it or anything.  Either way is fine with me.
Assignee: nobody → bzbarsky
Attachment #720232 - Flags: review?(peterv) → review?(continuation)
This simple testcase:

  <style></style>
  <script>
    try {
      m = new WeakMap();
      var s = document.styleSheets[0];
      m.set(s, 5);
      alert(m.get(s));
    } catch (e) {
      alert(e);
    }
  </script>

seems to work fine with these patches.  Seems like we could have just mutated bug 846666... ;)
Whiteboard: [need review]
Reporter

Comment 7

6 years ago
Comment on attachment 720232 [details] [diff] [review]
part 1.  Make nsCSSStyleSheet wrappercached.

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

::: layout/style/nsCSSStyleSheet.cpp
@@ +1215,2 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsCSSStyleSheet)

You could use NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE here.

::: layout/style/nsCSSStyleSheet.h
@@ +106,5 @@
>  
>  class nsCSSStyleSheet MOZ_FINAL : public nsIStyleSheet,
>                                    public nsIDOMCSSStyleSheet,
> +                                  public nsICSSLoaderObserver,
> +                                  public nsWrapperCache

I guess it was only non-nsISupports things that must inherit from wrappercache first, right?
Attachment #720232 - Flags: review?(continuation) → review+
> You could use NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE here.

Done.

> I guess it was only non-nsISupports things that must inherit from wrappercache first

Correct.
Comment on attachment 720233 [details] [diff] [review]
part 2.  Add the WebIDL APIs for StyleSheet and CSSStyleSheet.

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

::: content/html/content/src/HTMLLinkElement.cpp
@@ +89,5 @@
>    if (!ss) {
>      return;
>    }
>  
> +  ss->SetDisabled(aDisabled);

I'd just do

  if (ss) {
    ss->SetDisabled(aDisabled);
  }

::: content/html/content/src/HTMLStyleElement.cpp
@@ +90,5 @@
>    if (!ss) {
>      return;
>    }
>  
> +  ss->SetDisabled(aDisabled);

Same here.
Attachment #720233 - Flags: review?(peterv) → review+
Attachment #720234 - Flags: review?(peterv) → review+
Comment on attachment 720233 [details] [diff] [review]
part 2.  Add the WebIDL APIs for StyleSheet and CSSStyleSheet.

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

::: dom/bindings/BindingDeclarations.h
@@ +399,5 @@
> +{
> +  return NULL;
> +}
> +
> +struct ParentObject {

{ on the next line

::: layout/style/nsCSSStyleSheet.h
@@ +257,5 @@
>  
> +  // WebIDL StyleSheet API
> +  // Our nsIStyleSheet::GetType is a const method, so it ends up
> +  // ambiguous with with the XPCOM version.  Just disambiguate.
> +  void GetType(nsString& aType) {

{ on the next line

@@ +265,5 @@
> +  nsINode* GetOwnerNode() const { return mOwningNode; }
> +  nsCSSStyleSheet* GetParentStyleSheet() const { return mParent; }
> +  // Our nsIStyleSheet::GetTitle is a const method, so it ends up
> +  // ambiguous with with the XPCOM version.  Just disambiguate.
> +  void GetTitle(nsString& aTitle) {

.

@@ +266,5 @@
> +  nsCSSStyleSheet* GetParentStyleSheet() const { return mParent; }
> +  // Our nsIStyleSheet::GetTitle is a const method, so it ends up
> +  // ambiguous with with the XPCOM version.  Just disambiguate.
> +  void GetTitle(nsString& aTitle) {
> +    const_cast<const nsCSSStyleSheet*>(this)->GetTitle(aTitle);

:/

@@ +279,5 @@
> +  // version.
> +  nsIDOMCSSRule* GetDOMOwnerRule() const;
> +  nsIDOMCSSRuleList* GetCssRules(mozilla::ErrorResult& aRv);
> +  uint32_t InsertRule(const nsAString& aRule, uint32_t aIndex,
> +                      mozilla::ErrorResult& aRv) {

.

@@ +284,5 @@
> +    uint32_t retval;
> +    aRv = InsertRule(aRule, aIndex, &retval);
> +    return retval;
> +  }
> +  void DeleteRule(uint32_t aIndex, mozilla::ErrorResult& aRv) {

.

@@ +297,5 @@
> +
> +    return mozilla::dom::ParentObject(static_cast<nsIStyleSheet*>(mParent),
> +                                      mParent);
> +  }
> +  

Trailing whitespace
(In reply to :Ms2ger from comment #10)
> Comment on attachment 720233 [details] [diff] [review]
> part 2.  Add the WebIDL APIs for StyleSheet and CSSStyleSheet.
> ::: layout/style/nsCSSStyleSheet.h
> @@ +257,5 @@
> >  
> > +  // WebIDL StyleSheet API
> > +  // Our nsIStyleSheet::GetType is a const method, so it ends up
> > +  // ambiguous with with the XPCOM version.  Just disambiguate.
> > +  void GetType(nsString& aType) {
> 
> { on the next line

Not when the definition is inside the class declaration.
> I'd just do

Done.

>{ on the next line

As dbaron said, file style is what I did here.

> Trailing whitespace

Fixed.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #11)
> (In reply to :Ms2ger from comment #10)
> > Comment on attachment 720233 [details] [diff] [review]
> > part 2.  Add the WebIDL APIs for StyleSheet and CSSStyleSheet.
> > ::: layout/style/nsCSSStyleSheet.h
> > @@ +257,5 @@
> > >  
> > > +  // WebIDL StyleSheet API
> > > +  // Our nsIStyleSheet::GetType is a const method, so it ends up
> > > +  // ambiguous with with the XPCOM version.  Just disambiguate.
> > > +  void GetType(nsString& aType) {
> > 
> > { on the next line
> 
> Not when the definition is inside the class declaration.

Unless it's

void SetScopeElement(mozilla::dom::Element* aScopeElement)
{

a couple of lines earlier, then.
Duplicate of this bug: 846666
Depends on: 995780
You need to log in before you can comment on or make changes to this bug.