Remove nsITreeImageListener

RESOLVED FIXED in mozilla11

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
OS: Linux → All
Hardware: x86_64 → All
Assignee: nobody → sjohnson
Blocks: 666446
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
But we know it has to be an nsTreeImageListener (right?) so we can just statically cast to nsTreeImageListener.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Well, that's the question.  We talked through this on IRC and weren't convinced that it had to be ...
Another option is to add an IID to nsTreeImageListener and QI to that.
Posted patch Patch (v1) (obsolete) — Splinter Review
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!
Attachment #572998 - Flags: superreview?(bzbarsky)
Attachment #572998 - Flags: review?(hyatt)
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.
Attachment #572998 - Flags: superreview?(bzbarsky)
Attachment #572998 - Flags: superreview+
Attachment #572998 - Flags: review?(neil)
Attachment #572998 - Flags: review?(hyatt)
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).
Attachment #572998 - Flags: review?(neil) → review-
Posted patch Patch (v2) (obsolete) — Splinter Review
(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.
Attachment #572998 - Attachment is obsolete: true
Attachment #573331 - Flags: superreview+
Attachment #573331 - Flags: review?(neil)
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.)
Attachment #573331 - Flags: review?(neil) → review+
Posted patch b682077 (v3) (obsolete) — Splinter Review
(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.
Attachment #573331 - Attachment is obsolete: true
Attachment #574201 - Flags: superreview+
Attachment #574201 - Flags: review?(neil)
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. :)
Attachment #574201 - Flags: review?(neil) → review+
Posted patch b682077 (v4)Splinter Review
Changed style issues found in review.
Attachment #574201 - Attachment is obsolete: true
Attachment #574209 - Flags: superreview+
Attachment #574209 - Flags: review+
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/332d4787b430
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/332d4787b430
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Ok, I accidentally pushed too early. Had to backout again - Backout changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff07acd189de
https://hg.mozilla.org/mozilla-central/rev/3607dafccfac
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.