The default bug view has changed. See this FAQ.

Remove nsITreeImageListener

RESOLVED FIXED in mozilla11

Status

()

Core
XUL
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

6 years ago
Assignee: nobody → sjohnson
(Assignee)

Updated

6 years ago
Blocks: 666446
(Assignee)

Comment 1

6 years ago
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
Last Resolved: 6 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.
(Assignee)

Comment 5

5 years ago
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!
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 7

5 years ago
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-
(Assignee)

Comment 8

5 years ago
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.
Attachment #572998 - Attachment is obsolete: true
Attachment #573331 - Flags: superreview+
Attachment #573331 - Flags: review?(neil)

Comment 9

5 years ago
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+
(Assignee)

Comment 10

5 years ago
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.
Attachment #573331 - Attachment is obsolete: true
Attachment #574201 - Flags: superreview+
Attachment #574201 - Flags: review?(neil)
(Assignee)

Comment 11

5 years ago
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. :)
(Assignee)

Updated

5 years ago
Attachment #574201 - Flags: review?(neil) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 574209 [details] [diff] [review]
b682077 (v4)

Changed style issues found in review.
Attachment #574201 - Attachment is obsolete: true
Attachment #574209 - Flags: superreview+
Attachment #574209 - Flags: review+
(Assignee)

Comment 13

5 years ago
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
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

5 years ago
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 → ---
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a123a96c50c7
(Assignee)

Comment 17

5 years ago
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
(Assignee)

Comment 18

5 years ago
Ok, I accidentally pushed too early. Had to backout again - Backout changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff07acd189de
(Assignee)

Comment 19

5 years ago
Landed on m-i again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3607dafccfac
https://hg.mozilla.org/mozilla-central/rev/3607dafccfac
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.