Closed Bug 682077 Opened 13 years ago Closed 13 years ago

Remove nsITreeImageListener

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: jwir3, Assigned: jwir3)

References

Details

Attachments

(1 file, 3 obsolete files)

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: 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.
Attached 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-
Attached 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+
Attached 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+
Attached 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+
Target Milestone: --- → mozilla11
Status: REOPENED → RESOLVED
Closed: 13 years ago13 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
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: