deCOMify style sheet classes

RESOLVED FIXED in mozilla1.9.3a5

Status

()

P4
normal
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: dbaron, Assigned: craig.topper)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.3a5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments, 12 obsolete attachments)

86.61 KB, patch
bryner
: review+
bryner
: superreview+
Details | Diff | Splinter Review
82.78 KB, patch
Details | Diff | Splinter Review
63.32 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
7.86 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.22 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
19.39 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
970 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.89 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
36.23 KB, patch
Details | Diff | Splinter Review
166.16 KB, patch
Details | Diff | Splinter Review
6.60 KB, patch
Details | Diff | Splinter Review
4.88 KB, patch
Details | Diff | Splinter Review
49.56 KB, patch
Details | Diff | Splinter Review
33.39 KB, patch
Details | Diff | Splinter Review
28.31 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
12.56 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8alpha
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.
Attachment #144983 - Flags: superreview?(bryner)
Attachment #144983 - Flags: superreview+
Attachment #144983 - Flags: review?(bryner)
Attachment #144983 - Flags: review+
Fix checked in to trunk, 2004-04-12 14:56 -0700.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
meant to leave this open for remaining work
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 150032 [details] [diff] [review]
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.
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.
Created attachment 155066 [details] [diff] [review]
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 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.
Attachment #155066 - Flags: superreview?(bzbarsky)
Attachment #155066 - Flags: review?(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
Attachment #155066 - Flags: superreview?(bzbarsky)
Attachment #155066 - Flags: superreview+
Attachment #155066 - Flags: review?(bzbarsky)
Attachment #155066 - Flags: review+
Attachment #155066 - Attachment description: patch to make nsCSSStyleSheet.h → patch to make nsCSSStyleSheet.h (checked in to trunk, 2004-08-03 20:27 -0700)
Assignee: dbaron → nobody
Priority: P1 → P4
Whiteboard: [patch]
Target Milestone: mozilla1.8alpha1 → ---
Created attachment 419167 [details] [diff] [review]
patch 1 for nsHTMLCSSStyleSheet: rename the class
Assignee: nobody → dbaron
Status: REOPENED → ASSIGNED
Attachment #419167 - Flags: review?(bzbarsky)
Created attachment 419170 [details] [diff] [review]
patch 3 for nsHTMLCSSStyleSheet: deCOMtaminate (main patch)

Oops, missed the necessary s/NS_IMETHODIMP/nsresult/, the lack of which I presume would break Windows.
Attachment #419169 - Attachment is obsolete: true
Attachment #419170 - Flags: review?(bzbarsky)
Attachment #419169 - Flags: review?(bzbarsky)
Comment on attachment 419167 [details] [diff] [review]
patch 1 for nsHTMLCSSStyleSheet: rename the class

r=bzbarsky
Attachment #419167 - Flags: review?(bzbarsky) → review+
Comment on attachment 419168 [details] [diff] [review]
patch 2 for nsHTMLCSSStyleSheet: give concrete class a header file

r=bzbarsky
Attachment #419168 - Flags: review?(bzbarsky) → review+
Comment on attachment 419170 [details] [diff] [review]
patch 3 for nsHTMLCSSStyleSheet: deCOMtaminate (main patch)

r=bzbarsky
Attachment #419170 - Flags: review?(bzbarsky) → review+
Created attachment 419673 [details] [diff] [review]
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.
Attachment #419673 - Flags: review?(bzbarsky)
Created attachment 419674 [details] [diff] [review]
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.)
Attachment #419674 - Flags: review?(bzbarsky)
Comment on attachment 419673 [details] [diff] [review]
make nsCSSStyleSheetInner mark members private

r=bzbarsky
Attachment #419673 - Flags: review?(bzbarsky) → review+
Comment on attachment 419674 [details] [diff] [review]
remove nsCSSStyleSheet from layout module

r=bzbarsky
Attachment #419674 - Flags: review?(bzbarsky) → review+

Updated

9 years ago
Blocks: 105431
David, would you like me to take this over?
(Assignee)

Comment 24

9 years ago
Created attachment 442929 [details] [diff] [review]
DeCOMtaminate CSSStyleSheet method signatures
Attachment #442929 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 years ago
Attachment #442929 - Attachment is obsolete: true
Attachment #442929 - Flags: review?(bzbarsky)
(Assignee)

Comment 25

9 years ago
Created attachment 443811 [details] [diff] [review]
DeCOMtaminate CSSStyleSheet method signatures
Attachment #443811 - Flags: review?(bzbarsky)
(Assignee)

Comment 26

9 years ago
Created attachment 443812 [details] [diff] [review]
Remove nsICSSStyleSheet and replace all instances with concrete class
Attachment #443812 - Flags: review?(bzbarsky)
(Assignee)

Comment 27

9 years ago
Created attachment 443813 [details] [diff] [review]
Inline a few methods in nsCSSStyleSheet
Attachment #443813 - Flags: review?(bzbarsky)
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 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.
Attachment #443811 - Flags: review?(bzbarsky) → review?(dbaron)
(Assignee)

Comment 31

9 years ago
I did not do a non-libxul build but I can.
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.
(Assignee)

Comment 33

9 years ago
Did one overnight. Didn't get a chance to update this morning. Build passed. I hadn't changed anything to non-virtual yet.
Ah, oops; I'd thought that was in the second patch, but I guess it's not.
(Assignee)

Comment 35

9 years ago
I am working on another patch to do the non-virtuals and will be sure to check a non-libxul build before submitting
(Assignee)

Comment 36

9 years ago
Created attachment 444035 [details] [diff] [review]
Make most of the methods in nsCSSStyleSheet non-virtual
Attachment #444035 - Flags: review?(dbaron)
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.
Attachment #443811 - Flags: review?(dbaron) → review+
(Assignee)

Comment 38

9 years ago
Created attachment 444341 [details] [diff] [review]
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.
Attachment #443811 - Attachment is obsolete: true
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
Attachment #443812 - Flags: review?(dbaron) → review+
(Assignee)

Comment 40

9 years ago
Created attachment 444357 [details] [diff] [review]
Remove nsICSSStyleSheet and replace all instances with concrete class addressing dbaron's comments
Attachment #443812 - Attachment is obsolete: true
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
Attachment #443813 - Flags: review?(dbaron) → review+
Comment on attachment 444035 [details] [diff] [review]
Make most of the methods in nsCSSStyleSheet non-virtual

r=dbaron
Attachment #444035 - Flags: review?(dbaron) → review+
(Assignee)

Comment 43

9 years ago
Created attachment 444551 [details] [diff] [review]
Innline a few methods in nsCSSStyleSheet addressing comments
Attachment #443813 - Attachment is obsolete: true
(Assignee)

Comment 44

9 years ago
Created attachment 444552 [details] [diff] [review]
Make most of the methods in nsCSSStyleSheet non-virtual updating because of changes in the other patches
Attachment #444035 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #444551 - Attachment description: 443813: Inline a few methods in nsCSSStyleSheet addressing comments → Innline a few methods in nsCSSStyleSheet addressing comments
(Assignee)

Updated

9 years ago
Attachment #444552 - Attachment description: 444035: Make most of the methods in nsCSSStyleSheet non-virtual updating because of changes in the other patches → Make most of the methods in nsCSSStyleSheet non-virtual updating because of changes in the other patches
(Assignee)

Comment 46

9 years ago
Yes please
(Assignee)

Comment 47

9 years ago
Created attachment 444611 [details] [diff] [review]
DeCOMtaminate nsIStyleSheet method signatures
Attachment #444611 - Flags: review?(dbaron)
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.
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 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.
Attachment #444611 - Flags: review?(dbaron) → review+
(Assignee)

Comment 52

9 years ago
Created attachment 444838 [details] [diff] [review]
DeCOMtaminate nsIStyleSheet method signatures addressing comments
Attachment #444611 - Attachment is obsolete: true
(Assignee)

Comment 54

9 years ago
Created attachment 445042 [details] [diff] [review]
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.
Attachment #445042 - Flags: review?(dbaron)
(Assignee)

Updated

9 years ago
Attachment #445042 - Attachment is obsolete: true
Attachment #445042 - Flags: review?(dbaron)
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.
(Assignee)

Comment 56

9 years ago
Created attachment 445063 [details]
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.
(Assignee)

Comment 57

9 years ago
Nevermind. I removed a QI but didn't put an AddRef in its place.
(Assignee)

Updated

9 years ago
Attachment #445063 - Attachment is obsolete: true
(Assignee)

Comment 58

9 years ago
Created attachment 445064 [details] [diff] [review]
 Rename GetApplicable and GetComplete. Addressing dbaron's comments and fixing a bug.
Attachment #445064 - Flags: review?(dbaron)
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
Attachment #445064 - Flags: review?(dbaron) → review+
(Assignee)

Comment 60

9 years ago
(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? :)
(Assignee)

Comment 61

9 years ago
Created attachment 445290 [details] [diff] [review]
Rename GetApplicable and GetComplete. Addressing comments
Attachment #445064 - Attachment is obsolete: true
Attachment #445290 - Flags: review?(dbaron)
(Assignee)

Comment 62

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

Oops shouldn't have put the review request.
Attachment #445290 - Flags: review?(dbaron)
(Assignee)

Comment 64

9 years ago
Created attachment 445521 [details] [diff] [review]
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.
Attachment #445521 - Flags: review?(dbaron)
(Assignee)

Comment 65

9 years ago
Created attachment 445577 [details] [diff] [review]
Convert some nsCOMPtrs to raw pointers on callers of some of the methods that no longer return already_Addef
Attachment #445577 - Flags: review?(dbaron)
(Assignee)

Comment 66

9 years ago
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 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.
Attachment #445521 - Flags: review?(dbaron) → review-
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
Attachment #445577 - Flags: review?(dbaron) → review+
(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.
(Assignee)

Comment 70

9 years ago
Created attachment 445903 [details] [diff] [review]
Change already_AddRef return types plus other changes addressing comments
Attachment #445521 - Attachment is obsolete: true
Attachment #445903 - Flags: review?(dbaron)
(Assignee)

Comment 71

9 years ago
Created 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
Attachment #445903 - Attachment is obsolete: true
Attachment #445904 - Flags: review?(dbaron)
Attachment #445903 - Flags: review?(dbaron)
(Assignee)

Updated

9 years ago
Attachment #445577 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #445903 - Attachment is obsolete: false
Attachment #445903 - Flags: review?(dbaron)
Comment on attachment 445903 [details] [diff] [review]
Change already_AddRef return types plus other changes addressing comments

r=dbaron
Attachment #445903 - Flags: review?(dbaron) → review+
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)
Attachment #445904 - Flags: review?(dbaron) → review+
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.
Assignee: dbaron → craig.topper
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5

Comment 75

9 years ago
Is it possible that those patches broke IGoogle page?
since this landed, css is not loading there
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.
You need to log in before you can comment on or make changes to this bug.