All users were logged out of Bugzilla on October 13th, 2018

Clean up nsMaybeWeakPtrArray

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(9 attachments)

(Assignee)

Description

3 years ago
nsMaybeWeakPtrArray and nsMaybeWeakPtr are a little odd. This can make bugs like in bug 1074416 harder to find.
(Assignee)

Comment 1

3 years ago
Created attachment 8641312 [details] [diff] [review]
part 1 - Remove trailing whitespace from some nsNavHistory* files.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81069a460c55

There's a good bit of intermittent BC orange, but hopefully I just got unlucky.
(Assignee)

Comment 2

3 years ago
Created attachment 8641313 [details] [diff] [review]
part 2 - Don't implicitly convert nsMaybeWeakPtr to an nsCOMPtr.

This is only used in a few places, and it obscures what is happening.
Attachment #8641313 - Flags: review?(mak77)
(Assignee)

Comment 3

3 years ago
Created attachment 8641314 [details] [diff] [review]
part 3 - Inline nsMaybeWeakPtr_base.

nsMaybePtr<> is only ever instantiated with 3 different classes, so it
doesn't seem like it worth the contortions to save a little code.
Attachment #8641314 - Flags: review?(mak77)
(Assignee)

Comment 4

3 years ago
Created attachment 8641315 [details] [diff] [review]
part 4 - Get rid of some old style QIs from nsMaybeWeakPtrArray.
Attachment #8641315 - Flags: review?(mak77)
(Assignee)

Comment 5

3 years ago
Created attachment 8641316 [details] [diff] [review]
part 5 - Use the nice cycle collector macro template for nsNavHistoryResult.

Also, fix up the CC template so it works.
Attachment #8641316 - Flags: review?(mak77)
(Assignee)

Comment 6

3 years ago
Created attachment 8641317 [details] [diff] [review]
part 6 - Inline AppendWeakElement and RemoveWeakElement.

This class is only instantiated three times, so it doesn't seem worth
the contortions to save a bit of code.
Attachment #8641317 - Flags: review?(mak77)
(Assignee)

Comment 7

3 years ago
Created attachment 8641318 [details] [diff] [review]
part 7 - Use nicer nsTArray functions in nsMaybeWeakPtrArray.
Attachment #8641318 - Flags: review?(mak77)
(Assignee)

Comment 8

3 years ago
Created attachment 8641319 [details] [diff] [review]
part 8 - Clean up some minor style issues in nsMaybeWeakPtr.h.
Attachment #8641319 - Flags: review?(mak77)
(Assignee)

Updated

3 years ago
Attachment #8641312 - Flags: review?(mak77)

Updated

3 years ago
Attachment #8641312 - Flags: review?(mak77) → review+

Updated

3 years ago
Attachment #8641313 - Flags: review?(mak77) → review+
Comment on attachment 8641314 [details] [diff] [review]
part 3 - Inline nsMaybeWeakPtr_base.

Review of attachment 8641314 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsMaybeWeakPtr.h
@@ +74,5 @@
> +      return ref;
> +    }
> +  }
> +
> +  nsCOMPtr<nsIWeakReference> weakRef = do_QueryInterface(mPtr);

I wonder if, as a micro optimization, we could make if (mPtr) wrap both paths so we never try to do_QueryInterface(nullptr) here.
Attachment #8641314 - Flags: review?(mak77) → review+

Updated

3 years ago
Attachment #8641315 - Flags: review?(mak77) → review+

Updated

3 years ago
Attachment #8641316 - Flags: review?(mak77) → review+
Comment on attachment 8641317 [details] [diff] [review]
part 6 - Inline AppendWeakElement and RemoveWeakElement.

Review of attachment 8641317 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsMaybeWeakPtr.h
@@ +42,4 @@
>  template<class T>
>  class nsMaybeWeakPtrArray : public nsTArray< nsMaybeWeakPtr<T> >
>  {
> +  typedef nsTArray<nsMaybeWeakPtr<T>> MaybeArray;

MaybeWeakArray (since this is always an array the name would be confusing)
Attachment #8641317 - Flags: review?(mak77) → review+

Updated

3 years ago
Attachment #8641318 - Flags: review?(mak77) → review+
Comment on attachment 8641319 [details] [diff] [review]
part 8 - Clean up some minor style issues in nsMaybeWeakPtr.h.

Review of attachment 8641319 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for doing this nice cleanup and thank you for the separate patches that made review easier.
You can now merge all the changes, if you wish.
Attachment #8641319 - Flags: review?(mak77) → review+
(Assignee)

Comment 12

3 years ago
Created attachment 8643101 [details] [diff] [review]
part 9 - Use early return in nsMaybeWeakPtr<T>::GetValue().

This implements your suggestion in comment 9, plus does a little bit of other minor cleanup. Good idea.
Attachment #8643101 - Flags: review?(mak77)
Comment on attachment 8643101 [details] [diff] [review]
part 9 - Use early return in nsMaybeWeakPtr<T>::GetValue().

Review of attachment 8643101 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsMaybeWeakPtr.h
@@ +111,1 @@
>    }

nit: I think for this second "if" I prefer the old form, mostly cause the benefit of an early return here is very small and the code was more compact. but whatever, it's likely personal preference.

The other changes above look great!
Attachment #8643101 - Flags: review?(mak77) → review+
(Assignee)

Comment 14

3 years ago
(In reply to Marco Bonardo [::mak] from comment #13)
> nit: I think for this second "if" I prefer the old form, mostly cause the
> benefit of an early return here is very small and the code was more compact.
> but whatever, it's likely personal preference.

I reverted that part of the change.

Thanks for the reviews.
You need to log in before you can comment on or make changes to this bug.