Closed
Bug 239008
Opened 20 years ago
Closed 14 years ago
deCOMify style sheet classes
Categories
(Core :: CSS Parsing and Computation, defect, P4)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: dbaron, Assigned: craig.topper)
References
(Blocks 1 open bug)
Details
Attachments
(16 files, 12 obsolete files)
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 |
Reporter | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8alpha
Reporter | ||
Comment 1•20 years ago
|
||
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.
Reporter | ||
Comment 2•20 years ago
|
||
Reporter | ||
Updated•20 years ago
|
Attachment #144983 -
Flags: superreview?(bryner)
Attachment #144983 -
Flags: review?(bryner)
Updated•20 years ago
|
Attachment #144983 -
Flags: superreview?(bryner)
Attachment #144983 -
Flags: superreview+
Attachment #144983 -
Flags: review?(bryner)
Attachment #144983 -
Flags: review+
Reporter | ||
Comment 3•20 years ago
|
||
Fix checked in to trunk, 2004-04-12 14:56 -0700.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 4•20 years ago
|
||
meant to leave this open for remaining work
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 5•20 years ago
|
||
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.
Reporter | ||
Comment 6•20 years ago
|
||
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.
Reporter | ||
Comment 7•20 years ago
|
||
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.
Reporter | ||
Comment 8•20 years ago
|
||
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 9•20 years ago
|
||
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+
Reporter | ||
Updated•20 years ago
|
Attachment #155066 -
Attachment description: patch to make nsCSSStyleSheet.h → patch to make nsCSSStyleSheet.h (checked in to trunk, 2004-08-03 20:27 -0700)
Reporter | ||
Updated•17 years ago
|
QA Contact: ian → style-system
Reporter | ||
Updated•15 years ago
|
Assignee: dbaron → nobody
Priority: P1 → P4
Whiteboard: [patch]
Target Milestone: mozilla1.8alpha1 → ---
Reporter | ||
Comment 10•14 years ago
|
||
Reporter | ||
Comment 11•14 years ago
|
||
Attachment #419168 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 12•14 years ago
|
||
Attachment #419169 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 13•14 years ago
|
||
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 14•14 years ago
|
||
Comment on attachment 419167 [details] [diff] [review] patch 1 for nsHTMLCSSStyleSheet: rename the class r=bzbarsky
Attachment #419167 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 15•14 years ago
|
||
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 16•14 years ago
|
||
Comment on attachment 419170 [details] [diff] [review] patch 3 for nsHTMLCSSStyleSheet: deCOMtaminate (main patch) r=bzbarsky
Attachment #419170 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 17•14 years ago
|
||
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)
Reporter | ||
Comment 18•14 years ago
|
||
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)
Reporter | ||
Comment 19•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 419673 [details] [diff] [review] make nsCSSStyleSheetInner mark members private r=bzbarsky
Attachment #419673 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 21•14 years ago
|
||
Comment on attachment 419674 [details] [diff] [review] remove nsCSSStyleSheet from layout module r=bzbarsky
Attachment #419674 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 22•14 years ago
|
||
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•14 years ago
|
||
David, would you like me to take this over?
Assignee | ||
Comment 24•14 years ago
|
||
Attachment #442929 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #442929 -
Attachment is obsolete: true
Attachment #442929 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 25•14 years ago
|
||
Attachment #443811 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 26•14 years ago
|
||
Attachment #443812 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 27•14 years ago
|
||
Attachment #443813 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 28•14 years ago
|
||
Did you test that this compiles in a non-libxul build?
Comment 29•14 years ago
|
||
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.
Reporter | ||
Comment 30•14 years ago
|
||
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)
Reporter | ||
Updated•14 years ago
|
Attachment #443812 -
Flags: review?(bzbarsky) → review?(dbaron)
Reporter | ||
Updated•14 years ago
|
Attachment #443813 -
Flags: review?(bzbarsky) → review?(dbaron)
Assignee | ||
Comment 31•14 years ago
|
||
I did not do a non-libxul build but I can.
Reporter | ||
Comment 32•14 years ago
|
||
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•14 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.
Reporter | ||
Comment 34•14 years ago
|
||
Ah, oops; I'd thought that was in the second patch, but I guess it's not.
Assignee | ||
Comment 35•14 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•14 years ago
|
||
Attachment #444035 -
Flags: review?(dbaron)
Reporter | ||
Comment 37•14 years ago
|
||
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•14 years ago
|
||
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
Reporter | ||
Comment 39•14 years ago
|
||
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•14 years ago
|
||
Attachment #443812 -
Attachment is obsolete: true
Reporter | ||
Comment 41•14 years ago
|
||
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+
Reporter | ||
Comment 42•14 years ago
|
||
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•14 years ago
|
||
Attachment #443813 -
Attachment is obsolete: true
Assignee | ||
Comment 44•14 years ago
|
||
Attachment #444035 -
Attachment is obsolete: true
Assignee | ||
Updated•14 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•14 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
Reporter | ||
Comment 45•14 years ago
|
||
Do you need someone to land these for you?
Assignee | ||
Comment 46•14 years ago
|
||
Yes please
Assignee | ||
Comment 47•14 years ago
|
||
Attachment #444611 -
Flags: review?(dbaron)
Reporter | ||
Comment 48•14 years ago
|
||
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.
Reporter | ||
Comment 49•14 years ago
|
||
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.
Reporter | ||
Comment 50•14 years ago
|
||
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
Reporter | ||
Comment 51•14 years ago
|
||
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•14 years ago
|
||
Attachment #444611 -
Attachment is obsolete: true
Reporter | ||
Comment 53•14 years ago
|
||
nsIStyleSheet patch landed: http://hg.mozilla.org/mozilla-central/rev/911cc89e3cc5
Assignee | ||
Comment 54•14 years ago
|
||
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•14 years ago
|
Attachment #445042 -
Attachment is obsolete: true
Attachment #445042 -
Flags: review?(dbaron)
Reporter | ||
Comment 55•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
Nevermind. I removed a QI but didn't put an AddRef in its place.
Assignee | ||
Updated•14 years ago
|
Attachment #445063 -
Attachment is obsolete: true
Assignee | ||
Comment 58•14 years ago
|
||
Attachment #445064 -
Flags: review?(dbaron)
Reporter | ||
Comment 59•14 years ago
|
||
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•14 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•14 years ago
|
||
Attachment #445064 -
Attachment is obsolete: true
Attachment #445290 -
Flags: review?(dbaron)
Assignee | ||
Comment 62•14 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)
Reporter | ||
Comment 63•14 years ago
|
||
Landed the GetApplicable and GetComplete rename: http://hg.mozilla.org/mozilla-central/rev/7501fc5a3bc1 Thanks for the patch.
Assignee | ||
Comment 64•14 years ago
|
||
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•14 years ago
|
||
Attachment #445577 -
Flags: review?(dbaron)
Assignee | ||
Comment 66•14 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.
Reporter | ||
Comment 67•14 years ago
|
||
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-
Reporter | ||
Comment 68•14 years ago
|
||
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+
Reporter | ||
Comment 69•14 years ago
|
||
(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•14 years ago
|
||
Attachment #445521 -
Attachment is obsolete: true
Attachment #445903 -
Flags: review?(dbaron)
Assignee | ||
Comment 71•14 years ago
|
||
Attachment #445903 -
Attachment is obsolete: true
Attachment #445904 -
Flags: review?(dbaron)
Attachment #445903 -
Flags: review?(dbaron)
Assignee | ||
Updated•14 years ago
|
Attachment #445577 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #445903 -
Attachment is obsolete: false
Attachment #445903 -
Flags: review?(dbaron)
Reporter | ||
Comment 72•14 years ago
|
||
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+
Reporter | ||
Comment 73•14 years ago
|
||
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+
Reporter | ||
Comment 74•14 years ago
|
||
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
Closed: 20 years ago → 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Comment 75•14 years ago
|
||
Is it possible that those patches broke IGoogle page? since this landed, css is not loading there
Comment 76•14 years ago
|
||
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.
Description
•