Use fallible allocation and move semantics for push events

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Push Notifications
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment)

Created attachment 8744664 [details] [diff] [review]
fallibleAlloc.patch

Some trivial refactoring to occupy myself on the train. It looks like we can use fallible allocation and a move when passing push message data to the `PushMessageEvent` constructor. I also de-duped the nsTArray conversion methods while I was there.
Attachment #8744664 - Flags: review?(wchen)
Comment on attachment 8744664 [details] [diff] [review]
fallibleAlloc.patch

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

::: dom/push/PushUtil.cpp
@@ +10,5 @@
>  /* static */ bool
>  PushUtil::CopyArrayBufferToArray(const ArrayBuffer& aBuffer,
>                                   nsTArray<uint8_t>& aArray)
>  {
> +  MOZ_ASSERT(aArray.IsEmpty());

This function and the next one correctly handles non-empty arrays so the assertion isn't really valid.

If you want to make it a requirement that aArray is empty you can use InsertElementAt instead of SetLength + ReplaceElementAt similar to what ExtractBytesFromArrayBuffer is doing. That way we save calling the destructor on all the elements in the array before copying the data (although it might be optimized away since the array contains a fundamental data type).
Attachment #8744664 - Flags: review?(wchen) → review+
(In reply to William Chen [:wchen] from comment #1)
> If you want to make it a requirement that aArray is empty you can use
> InsertElementAt instead of SetLength + ReplaceElementAt similar to what
> ExtractBytesFromArrayBuffer is doing. That way we save calling the
> destructor on all the elements in the array before copying the data
> (although it might be optimized away since the array contains a fundamental
> data type).

Thanks for catching that! Changed here and in `PushManager::CopySubscriptionKeyToArray`.

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/662a8ccb9a3e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

11 months ago
Blocks: 1355213
You need to log in before you can comment on or make changes to this bug.