Closed Bug 183999 Opened 23 years ago Closed 23 years ago

Modernize content

Categories

(Core :: DOM: Core & HTML, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla1.3beta

People

(Reporter: peterv, Assigned: peterv)

Details

Attachments

(3 files)

Here's some cleanup I've been carrying around for a while. CallQI, do_QI, do_GetAtom, CallGetService, etc.
Attached patch Easy stuffSplinter Review
Attachment #108549 - Flags: superreview?(bzbarsky)
Attachment #108549 - Flags: review?(caillon)
Attached patch and some moreSplinter Review
Attachment #108555 - Flags: superreview?(bzbarsky)
Attachment #108555 - Flags: review?(caillon)
Comment on attachment 108549 [details] [diff] [review] Easy stuff > nsSelection.cpp: > + if (mIndex >= 0 && mIndex < (PRInt32)cnt) { > + return CallQueryInterface(mDomSelection->mRangeArray->ElementAt(mIndex), aItem); This leaks, since ElementAt addrefs. See also bug 184022, where this code is smacked... > + while (closestView && (closestView != scrolledView)) { Overparenthesized. > nsStyleSet.cpp: > nsCOMPtr<nsIAtom> tag; > - tag = getter_AddRefs(NS_NewAtom("StyleSet")); > + tag = do_GetAtom("StyleSet"); just make that one line, ok? > nsHTMLBodyElement.cpp: + if (NS_OK == CallQueryInterface(doc, &htmlContainer)) { How about some nsCOMPtr love here too? > nsWyciwygChannel.cpp Why that change? > nsCSSParser.cpp > } // else, no delcared namespaces Fix that while you're here? ;) (all three places) > nsXMLDocument.cpp > nsXMLDocument::GetAttributeStyleSheet + NS_ENSURE_ARG(aResult); NS_ENSURE_ARG_POINTER, please. sr=bzbarsky with those minor tweaks. Nice cleanup!
Attachment #108549 - Flags: superreview?(bzbarsky) → superreview+
Attachment #108555 - Flags: superreview?(bzbarsky) → superreview+
+ if (NS_OK == CallQueryInterface(doc, &htmlContainer)) { any reason not to use NS_SUCCEEDED here? (same for other places near the places you were touching, as visible in your patch)
Small note to (potential) reviewers: I'm not going to rewrite content completely ;-). I realize there's more stuff I can change, but please let's not turn this into an endless battle. That being said, I'll try to do the NS_SUCCEEDED changes. Boris: do you want me to hold off on the nsSelection changes until you landed your fix for bug 184022 then?
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: --- → mozilla1.3beta
Yeah, sorry if I got a bit carried away with nsCOMPtr love. ;) iirc, your patch to nsSelection does not involve too many spots with mRangeArray... you could just take those out; I've modernized all that code already. ;) I guess, yes it may be easier if you held off on that part.
what!?! you lazy slacker! you won't rewrite content? r!=me ;-)
Comment on attachment 108555 [details] [diff] [review] and some more Ahh.. our fabulous nsRange code. :-) I'm reminded of bug 156554 with this patch.
Attachment #108555 - Flags: review?(caillon) → review+
Comment on attachment 108549 [details] [diff] [review] Easy stuff r=caillon with boris's fixes, and these nits which I just had to throw in. ;-) >Index: base/src/nsGeneratedIterator.cpp >=================================================================== In nsresult nsGeneratedContentIterator::CurrentNode(nsIContent **aNode) > if (mGenIter) > return mGenIter->CurrentNode(aNode); This just stuck out since you are adding parens around ifs before and after this method. So do the same here. > Index: base/src/nsSelection.cpp >=================================================================== >@@ -5834,7 +5807,7 @@ > nsIView *scrolledView = 0; > nsIView *view = 0; Could you change these to nsnull? > >- result = scrollableView->QueryInterface(NS_GET_IID(nsIView), (void**)&view); >+ CallQueryInterface(scrollableView, &view); > > if (view) > { >@@ -5894,13 +5867,11 @@ > // > > view = 0; And this too? >@@ -7731,14 +7702,9 @@ > // > > nsIView *view = 0; Last one. Sorry. :-)
Attachment #108549 - Flags: review?(caillon) → review+
There are quite a few parts like this in the patch > StyleSetImpl *it = new StyleSetImpl(); >- if (nsnull == it) { >+ if (!it) { > return NS_ERROR_OUT_OF_MEMORY; > } I would much rather see that they were done as NS_ENSURE_TRUE instead. Let me know if you want a more detailed list
Yeah I had a lot more nits to pick, too. But the patch is large and I wanted to keep it to just what I saw in the patch. We can make peterv do round 2 later.
Yeah, and since these should all be more consistent after I land it'll be a much easier task to search and replace them, for someone else :-P.
There's absolutely nothing wrong with: it = new foo(); if (!it) { return NS_ERROR_FAILURE; } IMNSHO you should not use NS_ENSURE_TRUE() in a case like this, only use the NS_ENSURE_* macros where you're ensuring that *code* worked the way it was supposed to work, NS_ENSURE_* is identical to the if, the only difference is that it asserts, and there's no reason to assert if malloc fails, that's normal behavior in low memory situations, nothing special about that. (admittedly we're pretty much guaranteed to crash if that ever happens, but that's beside the point here). Please don't pollute changes like this by changing good valid if (...) return ... to NS_ENSURE_*(...).
I disagree :-), for a number of reason 1. NS_ENSURE_* doesn't assert but only warns. Granted, I don't know in what situations we are supposed to warn so my oppinion on it might differ from everybody elses, but I usually think of warnings as: "something didn't work as expected, if you're debugging you might wanna have a look here. However it's not fatal and shouldn't cause a crash." Assertions are for fatal (and only for fatal) errors in our codebase, so i agree that we shouldn't assert on out-of-memory. 2. In my definition of warnings above i don't restrict "debugging" to debugging the mozilla codebase. It could also be for when investigating why a page doesn't work in mozilla while it works in IE for example. Which is why something like NS_ENSURE_TRUE(aNode, NS_ERROR_DOM_NOT_SUPPORTED) is something that is good to put at the top of the implementation of some DOM-function. 3. Even if you avoid warning after a memory allocation it is very likly that the caller of the function that did the allocation will use ENSURE to make sure that that call worked. Or that the caller of *that* function will use ENSURE. Shortly, it is very likly that somewhere up the callstack we will warn anyway so another warning or two won't really matter. If we want to avoid warning at all for out-of-memory we pretty much can't use ENSURE at all, anywhere, since most functions can fail with an out-of-memory error. 4. When we run out of memory when doing a small allocation like this it is farily likly that it is a problem with our code. Some recursion/loop running out of control or we were accidently allocating far too much memory. The exception to this rule would be if we somewhere allocate some really big chunk of memory and it wouldn't be unexpected for the allocation to fail. 5. If the browser starts acting up because we're running short of memory (and we actually manage to not crash) it is probably good to be able to see in the console that the reason it is acting up is because of low memory. That will make it easier to take the appropriate action; Either free up memory if you are in fact not giving moz much memory to play with, or start debugging what is eating so freakin' much memory. Instead of going after random other problems that just happen to arrise because some random small memory allocation failed. If you still don't think we should warn for out-of-memory then i think we should look into if we really want ENSURE to warn. IMHO ENSURE makes the code a lot cleaner and i'd hate to uglyfy the code just becuase of this. peterv: feel free to still land the patch as is. My reason for arguing this is not this patch but that i want the argument that we shouldn't ENSURE after memoryallocation to go away :-)
True, it warns, it doesn't assert, but that's essentially the same thing, only a different level. And yeah, you're right, some level above the malloc point will end up throwing the warning anyways in our code, which is IMO even more reason *not* to throw the warning for OOM. All the NS_ENSURE_* macros are bogus to start with, they don't ensure anything, they just check things, they don't make things better in any way, they just don't ensure. What we should really have is NS_RETURN_* macro's that matched the NS_ENSURE_* ones w/o the silly warnings, and if we want the warnings we could have NS_RETURN_AND_WARN_* or somesuch macro's. Don't get me wrong here, I'm not against the idea of having easily readable macros like NS_ENSURE_* (but IMO that name should change), but I'm strongly against mis-using the NS_ENSURE_* macros in cases where you're just checking for normal failure cases that we *know* can and will happen (eventhough they may not be easily triggerable). I stand by my comment, using NS_ENSURE_TRUE() to check for malloc failure is IMO wrong.
This have added a warning on brad Tbox: +content/base/src/nsSelection.cpp:1860 + `class nsIFrame * thisBlock' might be used uninitialized in this function (not that I understand why this commit made a difference). P.S. Per bug 179819 the "uninitialized" warnings ought to be treated as errors. P.P.S. The new warning is similar to already existing one content/base/src/nsSelection.cpp:2313 `class nsIFrame * thisBlock' might be used uninitialized in this function
This commit did not change that variable in any way. Fixed. Thanks everyone.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
This change to nsDOMEvent::GetView() seems to have broken Drag & Drop of selections (Bug 185033). That is, the CallQueryInterface() fails because nsWebShell doesn't implement nsIDOMAbstractView so a Drag is never started: @@ -483,14 +484,10 @@ rv = mPresContext->GetContainer(getter_AddRefs(container)); NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && container, rv); - nsCOMPtr<nsIInterfaceRequestor> ifrq(do_QueryInterface(container)); - NS_ENSURE_TRUE(ifrq, NS_OK); - - nsCOMPtr<nsIDOMWindowInternal> window; - ifrq->GetInterface(NS_GET_IID(nsIDOMWindowInternal), getter_AddRefs(window)); + nsCOMPtr<nsIDOMWindowInternal> window = do_GetInterface(container); NS_ENSURE_TRUE(window, NS_OK); - - window->QueryInterface(NS_GET_IID(nsIDOMAbstractView), (void **)aView); + + CallQueryInterface(container, aView); } return rv;
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: