MediaGalleriesCustomBindings: Ensure blobs aren't GC'd when getMetadata runs.

MediaGalleriesCustomBindings: Ensure blobs aren't GC'd when getMetadata runs.

BUG=415792
Committed: https://crrev.com/ca5a56f98d44564885036e571b10562c61973771
Cr-Commit-Position: refs/heads/master@{#297444}

[Oren Blasberg, 2014-09-29 21:44:52]

orenb@chromium.org changed reviewers:
+ tommycli@chromium.org

[tommycli, 2014-09-29 21:50:00]

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
(right):

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:11: var
mediaFilesMetadata = {};
This makes it sound like you're storing the metadata of the media file rather
than a reference to a Blob you need to hold onto because of a pending
getMetadata request.

Off the top of my head, 'blobsWithPendingMetadataRequests'? Ugh, I hope you can
think of a better name than that one...

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:105: //
Store the blob in a global object to keep its refcount nonzero -- this
Perfect comment :)

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:127: var
uuid = request.args[0];
Maybe a short one-line comment since this is not super obvious.

And don't worry, there's precedent for doing this:
https://code.google.com/p/chromium/codesearch/#search/&q=args%20custom_bindin...

[Oren Blasberg, 2014-09-29 22:05:49]

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
(right):

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:11: var
mediaFilesMetadata = {};
On 2014/09/29 21:50:00, tommycli wrote:
> This makes it sound like you're storing the metadata of the media file rather
> than a reference to a Blob you need to hold onto because of a pending
> getMetadata request.
> 
> Off the top of my head, 'blobsWithPendingMetadataRequests'? Ugh, I hope you
can
> think of a better name than that one...

Done.

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:105: //
Store the blob in a global object to keep its refcount nonzero -- this
On 2014/09/29 21:50:00, tommycli wrote:
> Perfect comment :)
Thanks Tommy!

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:127: var
uuid = request.args[0];
On 2014/09/29 21:50:00, tommycli wrote:
> Maybe a short one-line comment since this is not super obvious.
> 
> And don't worry, there's precedent for doing this:
>
https://code.google.com/p/chromium/codesearch/#search/&q=args%20custom_bindin...

What sort of comment would be reasonable here? Remember, I'm still not sure how
args[0] ends up being the uuid after all.

[tommycli, 2014-09-29 22:09:25]

lgtm

baller variable name choice.

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/media_galleries_custom_bindings.js
(right):

https://codereview.chromium.org/615633003/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/media_galleries_custom_bindings.js:127: var
uuid = request.args[0];
On 2014/09/29 22:05:48, Oren Blasberg wrote:
> On 2014/09/29 21:50:00, tommycli wrote:
> > Maybe a short one-line comment since this is not super obvious.
> > 
> > And don't worry, there's precedent for doing this:
> >
>
https://code.google.com/p/chromium/codesearch/#search/&q=args%20custom_bindin...
> 
> What sort of comment would be reasonable here? Remember, I'm still not sure
how
> args[0] ends up being the uuid after all.

It's the uuid because of line 109: return [blobUuid, options, callback].

Maybe this doesn't even need a comment. I don't have a strong feeling about
this. You're the maintainer, you can make a judgement call.

[Oren Blasberg, 2014-09-29 22:14:42]

orenb@chromium.org changed reviewers:
+ yoz@chromium.org

[Yoyo Zhou, 2014-09-29 22:21:11]

LGTM

[Oren Blasberg, 2014-09-30 16:31:00]

The CQ bit was checked by orenb@chromium.org

[commit-bot: I haz the power, 2014-09-30 16:32:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615633003/60001

[commit-bot: I haz the power, 2014-09-30 17:37:31]

Committed patchset #4 (id:60001) as 1a284b102907baf6f815b44fbd2ffa1c8bee7751

[commit-bot: I haz the power, 2014-09-30 17:38:13]

Patchset 4 (id:??) landed as
https://crrev.com/ca5a56f98d44564885036e571b10562c61973771
Cr-Commit-Position: refs/heads/master@{#297444}