239008 - deCOMify style sheet classes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsIHTMLStyleSheet, nsICSSStyleSheet, and nsIHTMLCSSStyleSheet are unnecessary --
the class definitions of their only implementations should instead be accessible
to other classes that need them, with appropriate use of public and private.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for HTMLStyleSheet

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2004-04-12 14:56 -0700.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

meant to leave this open for remaining work

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: WIP for nsCSSStyleSheet etc.

This is work-in-progress, and it doesn't compile.  The followup patch I
describe at the end of bug 234861 comment 31 would help, I think.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The patch in bug 252578 splits nsCSSRuleProcessor into its own header file.  I
plan to split the .cpp file contents as well either when doing or after doing
this change.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to make nsCSSStyleSheet.h (checked in to trunk, 2004-08-03 20:27 -0700)

This puts stuff into nsCSSStyleSheet.h.  I'm not going to deCOMify at this
point, but I plan to split nsCSSRuleProcessor into its own file after this.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 155066 [details] [diff] [review]
patch to make nsCSSStyleSheet.h (checked in to trunk, 2004-08-03 20:27 -0700)

This:
 1) Renames CSSStyleSheetInner to nsCSSStyleSheetInner and CSSStyleSheetImpl to
nsCSSStyleSheet
 2) Moves the class definitions of those two into nsCSSStyleSheet.h
 3) Removes unneeded (in C++) "(void)" parameter lists.
 4) Reorganizes the functions in the definition of nsCSSStyleSheet.h to be by
interface, and moves one comment to nsICSSStyleSheet.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155066 [details] [diff] [review]
patch to make nsCSSStyleSheet.h (checked in to trunk, 2004-08-03 20:27 -0700)

r+sr=bzbarsky

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 for nsHTMLCSSStyleSheet: rename the class

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 for nsHTMLCSSStyleSheet: give concrete class a header file

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 for nsHTMLCSSStyleSheet: deCOMtaminate (main patch)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 for nsHTMLCSSStyleSheet: deCOMtaminate (main patch)

Oops, missed the necessary s/NS_IMETHODIMP/nsresult/, the lack of which I presume would break Windows.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419167 [details] [diff] [review]
patch 1 for nsHTMLCSSStyleSheet: rename the class

r=bzbarsky

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419168 [details] [diff] [review]
patch 2 for nsHTMLCSSStyleSheet: give concrete class a header file

r=bzbarsky

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419170 [details] [diff] [review]
patch 3 for nsHTMLCSSStyleSheet: deCOMtaminate (main patch)

r=bzbarsky

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: make nsCSSStyleSheetInner mark members private

I did some work towards nsCSSStyleSheet but sort of gave up; however, I have two pieces that could land.

First, if we're going to #include nsCSSStyleSheet.h in more places, we should make the inner class private.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: remove nsCSSStyleSheet from layout module

And, also, we can remove nsCSSStyleSheet from the layout module and remove its CID.

(And in the nsHTMLCSSStyleSheet patch above, I actually forgot the change to remove the CID, but now have it in my tree.)

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

HTMLCSSStyleSheet patches landed:
http://hg.mozilla.org/mozilla-central/rev/37c441117f70
http://hg.mozilla.org/mozilla-central/rev/133cc168d23d
http://hg.mozilla.org/mozilla-central/rev/5dc1687eaa75

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419673 [details] [diff] [review]
make nsCSSStyleSheetInner mark members private

r=bzbarsky

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419674 [details] [diff] [review]
remove nsCSSStyleSheet from layout module

r=bzbarsky

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed those two preliminary CSS style sheet patches:
http://hg.mozilla.org/mozilla-central/rev/b6e7a482da96
http://hg.mozilla.org/mozilla-central/rev/dae78df81792

Comment 23

[Zack Weinberg (:zwol)]

David, would you like me to take this over?

Comment 24

[Craig Topper]

Attachment: DeCOMtaminate CSSStyleSheet method signatures

Comment 25

[Craig Topper]

Attachment: DeCOMtaminate CSSStyleSheet method signatures

Comment 26

[Craig Topper]

Attachment: Remove nsICSSStyleSheet and replace all instances with concrete class

Comment 27

[Craig Topper]

Attachment: Inline a few methods in nsCSSStyleSheet

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you test that this compiles in a non-libxul build?

Comment 29

[Zack Weinberg (:zwol)]

To anyone working on this bug: please do not touch any of the classes with 'Declaration', 'DataBlock', or 'StyleRule' in their names until bug 553456 and bug 558235 have both been closed.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 443811 [details] [diff] [review]
DeCOMtaminate CSSStyleSheet method signatures

Boris's review queue is very overloaded right now; I should be able to look at these a good bit faster, hopefully this week.

Comment 31

[Craig Topper]

I did not do a non-libxul build but I can.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If you can easily, could you?  Otherwise, I could.  It's one of things that's relatively easy to break with patches like this, and it's easiest to check by actually trying it.

Comment 33

[Craig Topper]

Did one overnight. Didn't get a chance to update this morning. Build passed. I hadn't changed anything to non-virtual yet.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, oops; I'd thought that was in the second patch, but I guess it's not.

Comment 35

[Craig Topper]

I am working on another patch to do the non-virtuals and will be sure to check a non-libxul build before submitting

Comment 36

[Craig Topper]

Attachment: Make most of the methods in nsCSSStyleSheet non-virtual

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 443811 [details] [diff] [review]
DeCOMtaminate CSSStyleSheet method signatures

I don't like the difference between the method signatures between
AppendStyleSheet and InsertStyleSheetAt.  (Prior to your patch, nobody
checked the result of either, but one of them returned a non-NS_OK
result in some cases.)  Since we're moving to infalliable malloc, I
think the best solution is to make both of them return void.  That is,
please change InsertStyleSheetAt the same way you changed
AppendStyleSheet, so it returns void.

The same for AppendStyleRule and ReplaceStyleRule.


If it makes it easier to address these comments with an additional patch
on top rather then editing the patches underneath, feel free to do it
that way (although editing the lower patch is better if it's equally
easy).

Some additional changes that would probably be worth making in a later
patch (but not a revision of this one, please) are making
DeleteRuleFromGroup, InsertRuleIntoGroup, ReplaceRuleInGroup,
AddRuleProcessor and DropRuleProcessor return void, and making
GetStyleRuleAt return already_AddRefed<nsICSSRule> or nsICSSRule*.


Now, comments on the changes to callers:

>-            nsCOMPtr<nsICSSStyleSheet> clonedSheet;
>-            sheet->Clone(nsnull, nsnull, clonedDoc, nsnull,
>-                         getter_AddRefs(clonedSheet));
>+            nsCOMPtr<nsICSSStyleSheet> clonedSheet = sheet->Clone(nsnull, nsnull, clonedDoc, nsnull);

>-            nsCOMPtr<nsICSSStyleSheet> clonedSheet;
>-            sheet->Clone(nsnull, nsnull, clonedDoc, nsnull,
>-                         getter_AddRefs(clonedSheet));
>+            nsCOMPtr<nsICSSStyleSheet> clonedSheet = sheet->Clone(nsnull, nsnull, clonedDoc, nsnull);

Please wrap these at less than 80 characters.  Probably the best way to
wrap is to wrap after the "=" and indent "sheet" two characters more
than "nsCOMPtr".

>-      NS_FAILED(nsLayoutStylesheetCache::QuirkSheet()->
>-                Clone(nsnull, nsnull, nsnull, nsnull,
>-                      getter_AddRefs(quirkClone))) ||
>+      !(quirkClone = nsLayoutStylesheetCache::QuirkSheet()->Clone(nsnull, nsnull, nsnull, nsnull)) ||

Again, please wrap before 80 characters.


Given that InsertRuleInternal still returns nsresult, nsPresShell.cpp
should probably still check its result.

r=dbaron with that.  Good work on the patch.



If you weren't de-virtualizing these methods in a later patch, I'd
ask you to write "/* virtual */" as a comment before the type, i.e.:
  /* virtual */ nsresult
  nsCSSStyleSheet::...
but no need to bother given the later patch.

Comment 38

[Craig Topper]

Attachment: DeCOMtaminate CSSStyleSheet method signatures with dbaron's comments addressed

The other patches won't apply after the changes here, but they're still valid for review. I'll upload new versions after they are reviewed unless you want them now.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 443812 [details] [diff] [review]
Remove nsICSSStyleSheet and replace all instances with concrete class

nsXBLPrototypeResources.cpp:

>   PRInt32 i;
>-  PRInt32 count = oldSheets.Count();
>+  PRInt32 count = oldSheets.Length();
>   for (i = 0; i < count; i++) {

Given the switch to nsTArray these should now be PRUint32.

That said, it might even be preferable to have, in the header (within
the class):
  typedef nsTArray<nsRefPtr<nsCSSStyleSheet> > sheet_array_type;
  sheet_array_type mStyleSheetList;
which would then make it reasonable to type:
  for (sheet_array_type::size_type i = 0, count = oldSheets.Length();
       i < count; ++i) {
(there's three changes there:  move both variables into the for loop,
use sheet_array_type::size_type instead of PRUint32, and use ++i instead
of i++).

(And perhaps the same typedef in nsXBLPrototypeBinding?)

nsHTMLEditor.cpp:

>-  nsCOMPtr<nsIDOMStyleSheet> domSheet(do_QueryInterface(sheet));
>+  nsCOMPtr<nsIDOMStyleSheet> domSheet(do_QueryObject(sheet));

(occurs twice)

You don't need any Query-ing here.  You can just call SetDisabled
directly.  (And if you needed an nsIDOMStyleSheet*, you can just assign
an nsCSSStyleSheet* into one.)

That said, if you'd needed Query-ing, you'd want do_QueryInterface and
not do_QueryObject.  (see below, comments on nsCSSStyleSheet.cpp;
I reviewed the patch out-of-order but my comments are in-order).

>-      nsCOMPtr<nsIStyleSheet> sheet = do_QueryInterface(aSheet);
>+      nsCOMPtr<nsIStyleSheet> sheet = do_QueryObject(aSheet);

No need for any Query-ing; just call GetSheetURI on aSheet.

nsCSSLoader.cpp:

>-    rv = NS_NewCSSStyleSheet(aSheet);
>+    nsCSSStyleSheet* sheet;
>+    rv = NS_NewCSSStyleSheet(&sheet);
>     NS_ENSURE_SUCCESS(rv, rv);
>+    *aSheet = static_cast<nsCSSStyleSheet*>(sheet);

You don't need to make any changes here; it's better left as it was.

>-    nsCOMPtr<nsIDOMStyleSheet> nextParentSheet(do_QueryInterface(aParentSheet));
>+    nsCOMPtr<nsIDOMStyleSheet> nextParentSheet(do_QueryObject(aParentSheet));

Again, there shouldn't be any need for any query-ing at all (and if
there were, you'd still want do_QueryInterface).  You can just use:
  nsCOMPtr<nsIDOMStyleSheet> nextParentSheet(aParentSheet);

>-    observer = do_QueryInterface(aParentSheet);
>+    observer = do_QueryObject(aParentSheet);

observer = aParentSheet;

(again, change incorrect, but QueryInterface no longer needed)

nsCSSRuleProcessor.cpp:

>-        nsCOMPtr<nsICSSStyleSheet> cssSheet = do_QueryInterface(sheet);
>+        nsRefPtr<nsCSSStyleSheet> cssSheet = do_QueryInterface(sheet);

This, however, should be do_QueryObject.

(And I imagine there might be other occurrences of this pattern
that I missed in the patch... but then again, it doesn't really
matter, especially if we rename do_QueryObject to do_QueryInterface).
See comments below on nsCSSStyleSheet.cpp.

I think class nsCSSRuleProcessor could definitely use, within the class:
  typedef nsTArray<nsRefPtr<nsCSSStyleSheet> > sheet_array_type;
(use as described above for XBL code).


>-  for (PRInt32 i = mSheets.Count() - 1; i >= 0; --i)
>+  for (PRUint32 i = mSheets.Length(); i != 0; ) {
>+    --i;
>     mSheets[i]->AddRuleProcessor(this);
>+  }

I slightly prefer:
  for (PRUint32 i = mSheets.Length(); i-- != 0; ) {
    mSheets[i]->AddRuleProcessor(this);
  }
but it's ugly either way.

(twice, but the second time is DropRuleProcessor)

>-      if (!mSheets.EnumerateForwards(CascadeSheetEnumFunc, &data))
>-        return; /* out of memory */
>+
>+      for (PRUint32 index = 0; index < mSheets.Length(); ++index) {
>+        if (!CascadeSheetEnumFunc(mSheets.ElementAt(index), &data))
>+          return; /* out of memory */
>+      }

Could you use i instead of index, and mSheets[i] instead of
mSheets.ElementAt(i)?

Also, while you're here, could you:
 (a) rename CascadeSheetEnumFunc to CascadeSheet, and
 (b) make its second parameter have type RuleCascadeData*.

And then, actually, eliminate the |sheet| and |data| variables in it
(or just rename the parameters to sheet and data).  (I should have
noticed that above.)

nsCSSRules.cpp:

>-  nsCOMPtr<nsIDOMStyleSheet> sheet(do_QueryInterface(mChildSheet, &rv));
>+  nsCOMPtr<nsIDOMStyleSheet> sheet(do_QueryObject(mChildSheet, &rv));

No Querying needed; the |sheet| variable should be dropped.  Just
call mChildSheet->GetMedia().

nsCSSStyleSheet.cpp:

>   nsCOMPtr<nsIDOMStyleSheet> domSheet =
>-    do_QueryInterface(static_cast<nsICSSStyleSheet*>(mStyleSheet));
>+    do_QueryObject(static_cast<nsCSSStyleSheet*>(mStyleSheet));

This should be do_QueryInterface, not do_QueryObject.  The distinction
between the two was intended as what you're asking for ("query
interface" == "ask if an interface is supported; "query object" == "ask
if this is a particular class"), not what type of object you're starting
with.  That said, I think we should rename do_QueryObject to
do_QueryInterface since it just calls QueryInterface underneath and the
distinction will just cause confusion.

-    aRule->SetStyleSheet((nsICSSStyleSheet*)aSheet);
+    aRule->SetStyleSheet((nsCSSStyleSheet*)aSheet);

Could you change this cast to static_cast while you're here?

>-  : nsICSSStyleSheet(),
>+  : nsIStyleSheet(),

Just drop this line and start the initializer list with ": mRefCnt(0)".

(twice)

>-  nsCOMPtr<nsIStyleSheet> styleSheet(do_QueryInterface(aSheet));
>+  nsCOMPtr<nsIStyleSheet> styleSheet(do_QueryObject(aSheet));

This shouldn't use do_QueryObject.  As above, If you needed
QueryInterface to get an interface you should use do_QueryInterface.
However, what you really want here is just (to replace the whole #ifdef
DEBUG block):
   nsCOMPtr<nsIStyleSheet> parentSheet;
   aSheet->GetParentSheet(*getter_AddRefs(parentSheet));
   NS_ASSERTION(this == parentSheet, "We are being notified of a sheet load for a sheet that is not our child!\n");

nsCSSStyleSheet.h:

>-  virtual already_AddRefed<nsICSSStyleSheet> Clone(nsICSSStyleSheet* aCloneParent,
>+  virtual already_AddRefed<nsCSSStyleSheet> Clone(nsCSSStyleSheet* aCloneParent,
>                                                    nsICSSImportRule* aCloneOwnerRule,
>                                                    nsIDocument* aCloneDocument,
>                                                    nsIDOMNode* aCloneOwningNode) const;

Please fix indent.



You also need to rev IIDs for nsIDocument, nsICSSRule, nsICSSImportRule,
nsICSSStyleRule, nsICSSGroupRule, nsICSSImportRule, nsICSSNameSpaceRule,
nsICSSLoaderObserver.

(Since nsICSSRule changed, all interfaces derived from it need IID
revs even though only some of them changed independently.)

r=dbaron with those issues fixed

Comment 40

[Craig Topper]

Attachment: Remove nsICSSStyleSheet and replace all instances with concrete class addressing dbaron's comments

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 443813 [details] [diff] [review]
Inline a few methods in nsCSSStyleSheet

There were comments in the .cpp file saying that these owned weak references ("not ref counted").  Could you add comments in the .h to replace them?

>+  void SetOwningNode(nsIDOMNode* aOwningNode) { mOwningNode = aOwningNode; }
>+  void SetOwnerRule(nsICSSImportRule* aOwnerRule) { mOwnerRule = aOwnerRule; }

r=dbaron with that

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 444035 [details] [diff] [review]
Make most of the methods in nsCSSStyleSheet non-virtual

r=dbaron

Comment 43

[Craig Topper]

Attachment: Innline a few methods in nsCSSStyleSheet addressing comments

Comment 44

[Craig Topper]

Attachment: Make most of the methods in nsCSSStyleSheet non-virtual updating because of changes in the other patches

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do you need someone to land these for you?

Comment 46

[Craig Topper]

Yes please

Comment 47

[Craig Topper]

Attachment: DeCOMtaminate nsIStyleSheet method signatures

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I made some very minor tweaks to the patches:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches-clean/rev/64cee4da40d8
(change an NS_ASSERTION to NS_WARN_IF_FALSE, and two whitespace changes).

I'll land them shortly.

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed those 4 patches:
http://hg.mozilla.org/mozilla-central/rev/c660b23286f5
http://hg.mozilla.org/mozilla-central/rev/5329c2d9c968
http://hg.mozilla.org/mozilla-central/rev/779fd4bb356e
http://hg.mozilla.org/mozilla-central/rev/a1b729d5a933

Thanks for the patches.


I'll try to get to that remaining review later today, but I have some other things I need to do first.

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

While merging with my tree, I noticed another issue in the second of the patches above, which I fixed in:
http://hg.mozilla.org/mozilla-central/rev/b08683fb91ec

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 444611 [details] [diff] [review]
DeCOMtaminate nsIStyleSheet method signatures

nsCSSLoader.cpp:

>   nsIURI* uri = nsnull;
>   if (mParentData)
>-    mParentData->mSheet->GetSheetURI(&uri);
>+    uri = mParentData->mSheet->GetSheetURI().get();
>   if (!uri && mLoader->mDocument)
>     NS_IF_ADDREF(uri = mLoader->mDocument->GetDocumentURI());
>   return uri;

This would be much cleaner using nsCOMPtr (no .get() or NS_IF_ADDREF):

  nsCOMPtr<nsIURI> uri;
  if (mParentData)
    uri = mParentData->mSheet->GetSheetURI();
  if (!uri && mLoader->mDocument)
    uri = mLoader->mDocument->GetDocumentURI();
  return uri.forget();

nsCSSStyleSheet.cpp:

>-    rv = sheet->GetOwningDocument(*getter_AddRefs(doc));       \
>-    NS_ENSURE_SUCCESS(rv, rv);                                 \
>+    doc = sheet->GetOwningDocument();       \

Please fix the \ to line up with the rest.

>+  nsIURI* sheetURI = mInner->mSheetURI.get();

No .get() needed.

(same in GetBaseURI)

nsIStyleSheet.h:

>+template<class E> class already_AddRefed;

could you make this:
  template<class T> struct already_AddRefed;
for consistency with the way it's defined (T, struct, not E, class)

You need to rev the IID.


nsStyleSet.cpp:

>+  NS_ASSERTION(aSheet->GetApplicable(), "Inapplicable sheet being placed in style set"); \

(three times)
 - drop the \ at the end
 - wrap to 2 lines so it's under 80 chars (wrap at the comma, line
   up both arguments to the macro)

>+  NS_ASSERTION(aSheet->GetComplete(), "Incomplete sheet being removed from style set");

also wrap this, please


r=dbaron with those changes


I also wonder whether GetApplicable and GetComplete should be called
IsApplicable and IsComplete now that we don't have the XPCOM convention.
I tend do think they probably should, but that should also be a separate
patch (so I don't have to review this one again with that change mixed
in).

It's also possible a bunch of the methods that return
already_AddRefed<T> could just return T*, but again, that should be a
separate patch.

Comment 52

[Craig Topper]

Attachment: DeCOMtaminate nsIStyleSheet method signatures addressing comments

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsIStyleSheet patch landed:
http://hg.mozilla.org/mozilla-central/rev/911cc89e3cc5

Comment 54

[Craig Topper]

Attachment: Rename GetApplicable and GetComplete as dbaron suggested. Some other random cleanup too

Does the rename dbaron suggested. Removes a QI and some useless typecasts. Going to investigate removing the already_AddRef returns in another patch to follow.

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 445042 [details] [diff] [review]
Rename GetApplicable and GetComplete as dbaron suggested. Some other random cleanup too

I had started looking at this before you obsoleted it.  The things I'd noticed were:

 * You made some methods const methods (i.e., they're allowed to be called on a const object) that really shouldn't be.  I don't think you want to make any of those methods const.

 * I know I'd asked you before to add "/* virtual */" comments... and then when reviewing a later patch, I was thinking of asking you the same thing, and decided against it, because I couldn't justify asking you to do the work to add them.  I think I came to the conclusion that while they're the kind of thing that would be useful if they were present reliably, they're not present reliably, and therefore not particularly useful.  (Then again, one could say the same thing about writing the |virtual| in the header when a base class already makes the method virtual.  That said, that one is present *mostly* reliably.)

So I don't think you need to take out the "/* virtual */", but I'm also not sure that it's worth spending a lot of effort to put it in.

Comment 56

[Craig Topper]

Attachment: Stack trace of a failure on the most recent patch

I'm getting a failure when I try to run some css reftests on this patch. Seems to be crashing in an nsRefPtr destructor. I've looked over the patch several times and nothing jumps out at me as being wrong.

Comment 57

[Craig Topper]

Nevermind. I removed a QI but didn't put an AddRef in its place.

Comment 58

[Craig Topper]

Attachment: Rename GetApplicable and GetComplete. Addressing dbaron's comments and fixing a bug.

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 445064 [details] [diff] [review]
 Rename GetApplicable and GetComplete. Addressing dbaron's comments and fixing a bug.

nsCSSLoader.cpp:

> #ifdef DEBUG
>       // This sheet came from the XUL cache or our per-document hashtable; it
>       // better be a complete sheet.
>-      PRBool complete = sheet->GetComplete();
>-      NS_ASSERTION(complete,
>+      NS_ASSERTION(sheet->IsComplete(),
>                    "Sheet thinks it's not complete while we think it is");
> #endif

You can now drop the #ifdef DEBUG and the #endif since NS_ASSERTION
compiles away for non-DEBUG.

(It's probably good to leave a blank line in place of the #endif.)

(And you could probably do the same to the second set of assertions
you're changing if you just call sheet->IsComplete() twice, which is
fine in assertions.)

nsHTMLStyleSheet.cpp:

>-void
>+/* virtual */  void
> nsHTMLStyleSheet::GetTitle(nsString& aTitle) const

Only need one space between /* virtual */ and void


r=dbaron with that

Comment 60

[Craig Topper]

(In reply to comment #59)

> >-void
> >+/* virtual */  void
> > nsHTMLStyleSheet::GetTitle(nsString& aTitle) const
> 
> Only need one space between /* virtual */ and void

Couldn't let it go with only one comment? :)

Comment 61

[Craig Topper]

Attachment: Rename GetApplicable and GetComplete. Addressing comments

Comment 62

[Craig Topper]

Comment on attachment 445290 [details] [diff] [review]
Rename GetApplicable and GetComplete. Addressing comments

Oops shouldn't have put the review request.

Comment 63

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed the GetApplicable and GetComplete rename:
http://hg.mozilla.org/mozilla-central/rev/7501fc5a3bc1

Thanks for the patch.

Comment 64

[Craig Topper]

Attachment: Change already_AddRef return types plus other changes

This patch contains several things I noticed while working on the three style sheets classes

-Converted the already_AddRef return types to raw pointers as dbaron suggested. Future patch coming to convert some of the nsCOMPtr/nsRefPtr callers to raw pointers.
-Inlined some of the converted methods
-Converted some class members to nsRefPtr or nsCOMPtr and removed manual reference counting code.
-Removed some unnecessary initializations from some constructors. Mainly mRefCnt since it resets to 0 on its own.
-Remove nsresult return from Reset() on two of the stylesheet classes.

Comment 65

[Craig Topper]

Attachment: Convert some nsCOMPtrs to raw pointers on callers of some of the methods that no longer return already_Addef

Comment 66

[Craig Topper]

Comment on attachment 445521 [details] [diff] [review]
Change already_AddRef return types plus other changes

I forgot to change IID on nsIStyleSheet, but I'll fix it when I fix any review comments.

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 445521 [details] [diff] [review]
Change already_AddRef return types plus other changes

nsCSSStyleSheet::Clone should still return
already_AddRefed<nsCSSStyleSheet>.  It's bad to pass around objects with
0 reference count:  somebody might AddRef and Release them along the way.

(That means reverting the changes in nsCSSLoader.cpp too)

Other than that, this looks fine, so r=dbaron with that change.

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 445577 [details] [diff] [review]
Convert some nsCOMPtrs to raw pointers on callers of some of the methods that no longer return already_Addef

To sync with the requested changes in the previous patch, remove the
diffs in nsIDocument::CreateStaticClone and nsCSSRules.cpp

In Loader::LoadChildSheet, could you just remove the |owningDoc| variable 
entirely?  

Could you also undo the change in 
DOMCSSDeclarationImpl::DeclarationChanged ?  I'm just worried it might 
introduce a crash somehow.

Same for the three nsMediaList methods (and the comment above).

In nsCSSStyleSheet::StyleSheetLoaded, could you wrap the assertion
text to a new line (at the comma) and remove the "\n" at the end?

r=dbaron with that

Comment 69

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #67)
> Other than that, this looks fine, so r=dbaron with that change.

... and the one in comment 66, of course.

Comment 70

[Craig Topper]

Attachment: Change already_AddRef return types plus other changes addressing comments

Comment 71

[Craig Topper]

Attachment: Convert some nsCOMPtrs to raw pointers on callers of some of the methods that no longer return already_Addef addressing comments

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 445903 [details] [diff] [review]
Change already_AddRef return types plus other changes addressing comments

r=dbaron

Comment 73

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 445904 [details] [diff] [review]
Convert some nsCOMPtrs to raw pointers on callers of some of the methods that no longer return already_Addef addressing comments

r=dbaron with the nsCSSLoader.cpp comment addressed (which I did locally in my tree, so no need for a new patch)

Comment 74

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/acc89be54f3c
http://hg.mozilla.org/mozilla-central/rev/ce21b8d89bb0

(I noticed one more problem in the first patch; in the nsDocument changes, rv now needed to be initialized.  I fixed that before landing.)


I think this bug, as originally described, is now fixed, and we should probably put any additional work in new bugs.  (Well, really, we probably passed that point a while ago.)

Thanks for all your work on this, Craig.

Comment 75

[u336495]

Is it possible that those patches broke IGoogle page?
since this landed, css is not loading there

Comment 76

[Daniel Holbert [:dholbert]]

RooD: iGoogle looks the same for me, comparing a 4/29 nightly to a 5/20 nightly (w/ fresh profile in both cases).  So the landings in comment 74 didn't affect its rendering on my machine.  If your results are different, please file a new bug on it.