Last Comment Bug 682077 - Remove nsITreeImageListener
: Remove nsITreeImageListener
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Scott Johnson (:jwir3)
:
Mentors:
Depends on:
Blocks: 666446
  Show dependency treegraph
 
Reported: 2011-08-25 13:23 PDT by Scott Johnson (:jwir3)
Modified: 2012-02-01 13:58 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.35 KB, patch)
2011-11-08 14:11 PST, Scott Johnson (:jwir3)
neil: review-
bzbarsky: superreview+
Details | Diff | Review
Patch (v2) (5.07 KB, patch)
2011-11-09 14:37 PST, Scott Johnson (:jwir3)
neil: review+
jaywir3: superreview+
Details | Diff | Review
b682077 (v3) (7.45 KB, patch)
2011-11-13 16:52 PST, Scott Johnson (:jwir3)
jaywir3: review+
jaywir3: superreview+
Details | Diff | Review
b682077 (v4) (7.54 KB, patch)
2011-11-13 17:16 PST, Scott Johnson (:jwir3)
jaywir3: review+
jaywir3: superreview+
Details | Diff | Review

Description Scott Johnson (:jwir3) 2011-08-25 13:23:53 PDT
The XUL tree class nsTreeImageListener derives from nsITreeImageListener, but this interface is only used in nsTreeImageListener and nsTreeBodyFrame. It's unnecessary interface, and should be removed, and making nsTreeImageListener derive from nsISupports directly.
Comment 1 Scott Johnson (:jwir3) 2011-08-31 09:22:36 PDT
I don't think this is going to be possible, because of the following code in nsTreeBodyFrame:2146:

> nsCOMPtr<nsITreeImageListener> listener(do_QueryInterface(obs));

Resolving as invalid.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-31 16:22:10 PDT
But we know it has to be an nsTreeImageListener (right?) so we can just statically cast to nsTreeImageListener.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-31 16:24:38 PDT
Well, that's the question.  We talked through this on IRC and weren't convinced that it had to be ...
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-31 17:01:13 PDT
Another option is to add an IID to nsTreeImageListener and QI to that.
Comment 5 Scott Johnson (:jwir3) 2011-11-08 14:11:22 PST
Created attachment 572998 [details] [diff] [review]
Patch (v1)

Hi Dave and Boris:

Since this is removing an interface, I think it necessitates a superreview. Since you're both module peers for layout/xul, I decided to request review from you both. Please let me know if I should request review from another person.

Thanks!
Comment 6 Boris Zbarsky [:bz] 2011-11-08 19:17:52 PST
Comment on attachment 572998 [details] [diff] [review]
Patch (v1)

hyatt isn't likely to review this. Let's try Neil instead.

You want to use nsRefPtr<nsTreeImageListener>.  sr=me with that.
Comment 7 neil@parkwaycc.co.uk 2011-11-09 14:10:41 PST
Comment on attachment 572998 [details] [diff] [review]
Patch (v1)

>       nsCOMPtr<imgIDecoderObserver> obs;
>       imgReq->GetDecoderObserver(getter_AddRefs(obs));
>-      nsCOMPtr<nsITreeImageListener> listener(do_QueryInterface(obs));
>+      nsCOMPtr<nsTreeImageListener> listener =
>+        static_cast<nsTreeImageListener*>(obs.get());
>+
>       if (listener)
>         listener->AddCell(aRowIndex, aCol);
Since we're static_casting rather than do_QueryInterfacing the null check is wrong here. I'm not sure whether obs needs to be null checked instead.
You can also eliminate the temporary and call AddCell directly on the result of the cast.

>-  NS_IMETHOD AddCell(PRInt32 aIndex, nsITreeColumn* aCol) = 0;
AddCell needs to be turned into a normal class method (with void result).
Comment 8 Scott Johnson (:jwir3) 2011-11-09 14:37:15 PST
Created attachment 573331 [details] [diff] [review]
Patch (v2)

(In reply to neil@parkwaycc.co.uk from comment #7)
> Since we're static_casting rather than do_QueryInterfacing the null check is
> wrong here. I'm not sure whether obs needs to be null checked instead.

Since mListener inside of imgRequestProxy could by nsnull, and GetDecoderObserver() on an imgRequestProxy just dereferences the parameter and assigns it mListener, I think this needs a null check. I've modified it in the latest version of the patch.
 
> You can also eliminate the temporary and call AddCell directly on the result
> of the cast.

Done.

> AddCell needs to be turned into a normal class method (with void result).

Done.
Comment 9 neil@parkwaycc.co.uk 2011-11-10 11:52:07 PST
Comment on attachment 573331 [details] [diff] [review]
Patch (v2)

Some minor style nits:

>+        (static_cast<nsTreeImageListener*> (obs.get()))->AddCell(aRowIndex,
>+                                                                 aCol);
I didn't think you needed quite so many ()s, unless they're for readability.
Also, no space necessary between static_cast<> and its (argument).

>+  void AddCell(PRInt32 aIndex, nsITreeColumn* aCol);
>  
>   friend class nsTreeBodyFrame;
> 
> protected:
>   void UnsuppressInvalidation() { mInvalidationSuppressed = false; }
Since nsTreeBodyFrame is a friend, we could theoretically protect AddCell.
(I don't know whether we prefer public methods or friends.)
Comment 10 Scott Johnson (:jwir3) 2011-11-13 16:52:52 PST
Created attachment 574201 [details] [diff] [review]
b682077 (v3)

(In reply to neil@parkwaycc.co.uk from comment #7)
> Since we're static_casting rather than do_QueryInterfacing the null check is
> wrong here. I'm not sure whether obs needs to be null checked instead.

Yes, I think that obs needs to be null checked. The implementation of GetDecoderObserver in imgRequestProxy just sets the parameter to mListener, which can be nsnull. I've modified this in the current patch.
 
> You can also eliminate the temporary and call AddCell directly on the result
> of the cast.

I've changed this in the current version.

> AddCell needs to be turned into a normal class method (with void result).
Done.
Comment 11 Scott Johnson (:jwir3) 2011-11-13 17:00:55 PST
hm... sorry about this, I apparently had two tabs open with this bug, and one of them hadn't been updated. I will fix the items described in comment 9. :)
Comment 12 Scott Johnson (:jwir3) 2011-11-13 17:16:37 PST
Created attachment 574209 [details] [diff] [review]
b682077 (v4)

Changed style issues found in review.
Comment 13 Scott Johnson (:jwir3) 2011-11-14 19:24:32 PST
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/332d4787b430
Comment 14 Ed Morley [:emorley] 2011-11-15 11:53:51 PST
https://hg.mozilla.org/mozilla-central/rev/332d4787b430
Comment 15 Scott Johnson (:jwir3) 2011-11-23 11:41:00 PST
This bug seems to have had an effect on bug 703133. It doesn't appear to be the cause, because there is a crash in FF 8.0 with the same signature and stack trace (), but it seems like this bug may have made it more likely that the crash occurs. 

I backed this out in order to see if it has an effect on the frequency of the crash.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a123a96c50c7
Comment 16 Ed Morley [:emorley] 2011-11-24 08:12:14 PST
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a123a96c50c7
Comment 17 Scott Johnson (:jwir3) 2011-11-28 10:42:34 PST
It doesn't seem like this bug affects bug 703133, so I'm repushing to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/068e3078ced6
Comment 18 Scott Johnson (:jwir3) 2011-11-28 11:13:18 PST
Ok, I accidentally pushed too early. Had to backout again - Backout changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff07acd189de
Comment 19 Scott Johnson (:jwir3) 2011-11-28 12:12:01 PST
Landed on m-i again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3607dafccfac
Comment 20 Marco Bonardo [::mak] 2011-11-29 04:54:47 PST
https://hg.mozilla.org/mozilla-central/rev/3607dafccfac

Note You need to log in before you can comment on or make changes to this bug.