183999 - Modernize content

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Here's some cleanup I've been carrying around for a while. CallQI, do_QI,
do_GetAtom, CallGetService, etc.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Easy stuff

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: and some more

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: diff -uw nsRange.cpp (Alternative for part of attachment 108555)

Comment 4

[Boris Zbarsky [:bzbarsky]]

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!

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

+	 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)

Comment 6

[Peter Van der Beken [:peterv]]

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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

what!?! you lazy slacker! you won't rewrite content? r!=me ;-)

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 108555 [details] [diff] [review]
and some more

Ahh..  our fabulous nsRange code.  :-)	I'm reminded of  bug 156554 with this
patch.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.  :-)

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 13

[Peter Van der Beken [:peterv]]

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.

Comment 14

[Johnny Stenback (:jst)]

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_*(...).

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 :-)

Comment 16

[Johnny Stenback (:jst)]

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.

Comment 17

[Aleksey Nogin]

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

Comment 18

[Peter Van der Beken [:peterv]]

This commit did not change that variable in any way.
Fixed. Thanks everyone.

Comment 19

[kinmoz]

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;