Closed Bug 1189369 Opened 5 years ago Closed 5 years ago

Clean up nsMaybeWeakPtrArray

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

Details

Attachments

(9 files)

nsMaybeWeakPtrArray and nsMaybeWeakPtr are a little odd. This can make bugs like in bug 1074416 harder to find.
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.
This is only used in a few places, and it obscures what is happening.
Attachment #8641313 - Flags: review?(mak77)
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)
Also, fix up the CC template so it works.
Attachment #8641316 - Flags: review?(mak77)
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)
Attachment #8641312 - Flags: review?(mak77)
Attachment #8641312 - Flags: review?(mak77) → review+
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+
Attachment #8641315 - Flags: review?(mak77) → review+
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+
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+
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+
(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.