Closed
Bug 682077
Opened 13 years ago
Closed 13 years ago
Remove nsITreeImageListener
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: jwir3, Assigned: jwir3)
References
Details
Attachments
(1 file, 3 obsolete files)
7.54 KB,
patch
|
jwir3
:
review+
jwir3
:
superreview+
|
Details | Diff | Splinter Review |
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•13 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → sjohnson
Assignee | ||
Comment 1•13 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
Closed: 13 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•13 years ago
|
||
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 6•13 years ago
|
||
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•13 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•13 years ago
|
||
(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•13 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•13 years ago
|
||
(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•13 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•13 years ago
|
Attachment #574201 -
Flags: review?(neil) → review+
Assignee | ||
Comment 12•13 years ago
|
||
Changed style issues found in review.
Attachment #574201 -
Attachment is obsolete: true
Attachment #574209 -
Flags: superreview+
Attachment #574209 -
Flags: review+
Assignee | ||
Comment 13•13 years ago
|
||
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/332d4787b430
Target Milestone: --- → mozilla11
Comment 14•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 15•13 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 → ---
Comment 16•13 years ago
|
||
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a123a96c50c7
Assignee | ||
Comment 17•13 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•13 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•13 years ago
|
||
Landed on m-i again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3607dafccfac
Comment 20•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•