Media Galleries API Metadata: SafeMediaMetadataParser and IPCTunnelDataReader.

Media Galleries API Metadata: SafeMediaMetadataParser and IPCTunnelDataReader.

BUG=318450
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244310

[tommycli, 2013-12-04 01:30:38]

https://codereview.chromium.org/103283003/diff/40001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/103283003/diff/40001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.h:134:
metadata::IPCTunnelDataReader* ipc_tunnel_data_reader_;
This is kind of janky. It's here because ChromeContentUtilityClient needs to
deliver some bytes over to the data reader and MediaMetadataParser takes
ownership of its data reader.

Maybe MediaMetadataParser should not take ownership and they can be siblings?

[tommycli, 2014-01-06 20:25:11]

vandebo: I fixed the ownership issue (finally). Let me know what you think.

https://codereview.chromium.org/103283003/diff/40001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/103283003/diff/40001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.h:134:
metadata::IPCTunnelDataReader* ipc_tunnel_data_reader_;
On 2013/12/04 01:30:38, tommycli wrote:
> This is kind of janky. It's here because ChromeContentUtilityClient needs to
> deliver some bytes over to the data reader and MediaMetadataParser takes
> ownership of its data reader.
> 
> Maybe MediaMetadataParser should not take ownership and they can be siblings?

Done.

[vandebo (ex-Chrome), 2014-01-07 18:34:58]

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:67:
MediaMetadata::FromValue(metadata_dictionary));
Aren't we going to need to turn this back into Values later?  If so, why not
just keep it in that format?

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:72:
base::Bind(callback_, parse_success, base::Passed(metadata.Pass())));
You said callback_ is only access on the UI thread.  Probably it should only be
accessed on the IO thread.

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:87:
base::Unretained(this),
This concerns me.  What if the SafeMediaMetadataParser owner doesn't want and
cleans up?  What if the utility process crashes?  A weak pointer might be an
option.  You could also avoid trampolining through SafeMediaMetadataParser,
using a local function with bound parameters to call through directly to the
utility process.

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:106: 
This should set the state.

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h
(right):

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:74: Profile*
profile_;
Which thread are these accessed on?

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:75:
std::string blob_uuid_;
Can these be const ?

https://codereview.chromium.org/103283003/diff/130001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:239: std::string /* bytes */)
Maybe this should be char* instead of std::string? The data isn't a string and
base::File::Read() uses char* to return data.

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:790: void
ChromeContentUtilityClient::OnParseMediaMetadataRequestBlobBytesFinished(
I suggest making IPCDataSource a message Listener and adding it to |handlers_|. 
That way it will both send and receive message instead of only sending them. 
However, you'd need a mechanism to remove it from handlers_ as well and that may
not be worth the effort...

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
File chrome/utility/media_galleries/ipc_tunnel_data_source.h (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_tunnel_data_source.h:15: class
IPCTunnelDataSource : public media::DataSource {
IPCDataSource is probably a sufficient name.

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
File chrome/utility/media_galleries/media_metadata_parser.h (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.h:28: // Does not take
ownership of |source|. The MIME type is sniffed in the
You should say more explicitly that the caller is responsible for ensuring that
source outlives MediaMetadataParser

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.h:29: // browser process,
as it's faster (and safe enough). When the user wants
I think you can omit the stuff about mime type - this API doesn't care about
how/why it is parsed by the user of the API.

[tommycli, 2014-01-07 20:23:29]

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:67:
MediaMetadata::FromValue(metadata_dictionary));
On 2014/01/07 18:34:59, vandebo wrote:
> Aren't we going to need to turn this back into Values later?  If so, why not
> just keep it in that format?

Done.

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:72:
base::Bind(callback_, parse_success, base::Passed(metadata.Pass())));
On 2014/01/07 18:34:59, vandebo wrote:
> You said callback_ is only access on the UI thread.  Probably it should only
be
> accessed on the IO thread.

I updated the .h comment to read:
// Assigned on the UI thread, then accessed on the IO thread only.

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:87:
base::Unretained(this),
On 2014/01/07 18:34:59, vandebo wrote:
> This concerns me.  What if the SafeMediaMetadataParser owner doesn't want and
> cleans up?  What if the utility process crashes?  A weak pointer might be an
> option.  You could also avoid trampolining through SafeMediaMetadataParser,
> using a local function with bound parameters to call through directly to the
> utility process.

Done.

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:106: 
On 2014/01/07 18:34:59, vandebo wrote:
> This should set the state.

Done.

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h
(right):

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:74: Profile*
profile_;
On 2014/01/07 18:34:59, vandebo wrote:
> Which thread are these accessed on?

Done.

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:75:
std::string blob_uuid_;
On 2014/01/07 18:34:59, vandebo wrote:
> Can these be const ?

Done.

https://codereview.chromium.org/103283003/diff/130001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:239: std::string /* bytes */)
On 2014/01/07 18:34:59, vandebo wrote:
> Maybe this should be char* instead of std::string? The data isn't a string and
> base::File::Read() uses char* to return data.

I don't see any other pointers in IPC messages. I do see std::vector<unsigned
char>, but that seems worse. I need the length in any case...

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:790: void
ChromeContentUtilityClient::OnParseMediaMetadataRequestBlobBytesFinished(
On 2014/01/07 18:34:59, vandebo wrote:
> I suggest making IPCDataSource a message Listener and adding it to
|handlers_|. 
> That way it will both send and receive message instead of only sending them. 
> However, you'd need a mechanism to remove it from handlers_ as well and that
may
> not be worth the effort...

Done. This is better, but I have one concern left:

What if in a different function, another IPCDataSource is created and added to
handlers_? In that case, the newly created IPCDataSource will never receive any
messages.

I think whether or not this will become a problem - and the solution, will
become apparent in further CLs though, so I'm okay with this for right now...

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
File chrome/utility/media_galleries/ipc_tunnel_data_source.h (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_tunnel_data_source.h:15: class
IPCTunnelDataSource : public media::DataSource {
On 2014/01/07 18:34:59, vandebo wrote:
> IPCDataSource is probably a sufficient name.

Done.

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
File chrome/utility/media_galleries/media_metadata_parser.h (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.h:28: // Does not take
ownership of |source|. The MIME type is sniffed in the
On 2014/01/07 18:34:59, vandebo wrote:
> You should say more explicitly that the caller is responsible for ensuring
that
> source outlives MediaMetadataParser

Done.

https://codereview.chromium.org/103283003/diff/130001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.h:29: // browser process,
as it's faster (and safe enough). When the user wants
On 2014/01/07 18:34:59, vandebo wrote:
> I think you can omit the stuff about mime type - this API doesn't care about
> how/why it is parsed by the user of the API.
> 

Done.

[vandebo (ex-Chrome), 2014-01-08 00:26:17]

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:87:
base::Unretained(this),
On 2014/01/07 20:23:31, tommycli wrote:
> On 2014/01/07 18:34:59, vandebo wrote:
> > This concerns me.  What if the SafeMediaMetadataParser owner doesn't want
and
> > cleans up?  What if the utility process crashes?  A weak pointer might be an
> > option.  You could also avoid trampolining through SafeMediaMetadataParser,
> > using a local function with bound parameters to call through directly to the
> > utility process.
> 
> Done.

I think bound parameters would be preferred...

base::Bind(DoBlobReaderDone, utility_process_host_, request_id)

void DoBlobReaderDone(WeakPtr<> utility_process_host_, request_id,
scoped_ptr<std::string> data) {

if (utility_process_host_.IsValid()) {
  ...
}

}

https://codereview.chromium.org/103283003/diff/130001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:239: std::string /* bytes */)
On 2014/01/07 20:23:31, tommycli wrote:
> On 2014/01/07 18:34:59, vandebo wrote:
> > Maybe this should be char* instead of std::string? The data isn't a string
and
> > base::File::Read() uses char* to return data.
> 
> I don't see any other pointers in IPC messages. I do see std::vector<unsigned
> char>, but that seems worse. I need the length in any case...

I think it's preferred to pass std::vector<unsigned char> and length. 
std::string should be used for strings.

https://codereview.chromium.org/103283003/diff/450001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h
(right):

https://codereview.chromium.org/103283003/diff/450001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:81: //
Assigned on the UI thread, then accessed on the IO thread only.
I think you can just assign this on the IO thread.

[tommycli, 2014-01-08 01:17:09]

vandebo: thanks! When you look at the interpatchset delta, there may be some
extra lines, since I also merged https://codereview.chromium.org/121383002/ into
the branch.

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/130001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:87:
base::Unretained(this),
On 2014/01/08 00:26:18, vandebo wrote:
> On 2014/01/07 20:23:31, tommycli wrote:
> > On 2014/01/07 18:34:59, vandebo wrote:
> > > This concerns me.  What if the SafeMediaMetadataParser owner doesn't want
> and
> > > cleans up?  What if the utility process crashes?  A weak pointer might be
an
> > > option.  You could also avoid trampolining through
SafeMediaMetadataParser,
> > > using a local function with bound parameters to call through directly to
the
> > > utility process.
> > 
> > Done.
> 
> I think bound parameters would be preferred...
> 
> base::Bind(DoBlobReaderDone, utility_process_host_, request_id)
> 
> void DoBlobReaderDone(WeakPtr<> utility_process_host_, request_id,
> scoped_ptr<std::string> data) {
> 
> if (utility_process_host_.IsValid()) {
>   ...
> }
> 
> }

Done.

https://codereview.chromium.org/103283003/diff/130001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/103283003/diff/130001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:239: std::string /* bytes */)
On 2014/01/08 00:26:18, vandebo wrote:
> On 2014/01/07 20:23:31, tommycli wrote:
> > On 2014/01/07 18:34:59, vandebo wrote:
> > > Maybe this should be char* instead of std::string? The data isn't a string
> and
> > > base::File::Read() uses char* to return data.
> > 
> > I don't see any other pointers in IPC messages. I do see
std::vector<unsigned
> > char>, but that seems worse. I need the length in any case...
> 
> I think it's preferred to pass std::vector<unsigned char> and length. 
> std::string should be used for strings.

Done.

https://codereview.chromium.org/103283003/diff/450001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h
(right):

https://codereview.chromium.org/103283003/diff/450001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:81: //
Assigned on the UI thread, then accessed on the IO thread only.
On 2014/01/08 00:26:18, vandebo wrote:
> I think you can just assign this on the IO thread.

Done.

[vandebo (ex-Chrome), 2014-01-08 17:48:06]

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:26:
scoped_ptr<std::string> data) {
Sorry, I didn't know that BlobReader returns std::string.  I think that's wrong,
but in that case, there's no reason to copy it again and you should use
std::string throughout.

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:53:
DCHECK(!callback.is_null());
nit: move to StartWokOnIOThread

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:99: int64
request_id,
nit: args fit on one line.

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h
(right):

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:30: typedef
extensions::api::media_galleries::MediaMetadata MediaMetadata;
Remove

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:73: // Only
accessed on the IO thread.
All member variables are only accessed on the IO thread, maybe change this
comment to say that.

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:296: void
SendMediaMetadataToHost(
This probably should be in an ifdef block.

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:26: class IPCDataSource;
Remove

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:102: void
OnParseMediaMetadataRequestBlobBytesFinished(int64 request_id,
Remove

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/media_ga...
File chrome/utility/media_galleries/ipc_data_source.cc (right):

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_data_source.cc:48:
content::UtilityThread::Get()->Send(new ChromeUtilityHostMsg_RequestBlobBytes(
note: I guess this is more correctly named UtilityProcessDataSource since it can
only be used from the utility process.  But IPCDataSource seems fine as well.

[tommycli, 2014-01-08 18:59:37]

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:26:
scoped_ptr<std::string> data) {
On 2014/01/08 17:48:06, vandebo wrote:
> Sorry, I didn't know that BlobReader returns std::string.  I think that's
wrong,
> but in that case, there's no reason to copy it again and you should use
> std::string throughout.

Done.

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:53:
DCHECK(!callback.is_null());
On 2014/01/08 17:48:06, vandebo wrote:
> nit: move to StartWokOnIOThread

Done.

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:99: int64
request_id,
On 2014/01/08 17:48:06, vandebo wrote:
> nit: args fit on one line.

Kind of pathological, but officially, this is preferred:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h
(right):

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:30: typedef
extensions::api::media_galleries::MediaMetadata MediaMetadata;
On 2014/01/08 17:48:06, vandebo wrote:
> Remove

Done.

https://codereview.chromium.org/103283003/diff/400002/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.h:73: // Only
accessed on the IO thread.
On 2014/01/08 17:48:06, vandebo wrote:
> All member variables are only accessed on the IO thread, maybe change this
> comment to say that.

Done.

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:296: void
SendMediaMetadataToHost(
On 2014/01/08 17:48:06, vandebo wrote:
> This probably should be in an ifdef block.

Done.

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:26: class IPCDataSource;
On 2014/01/08 17:48:06, vandebo wrote:
> Remove

Done.

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:102: void
OnParseMediaMetadataRequestBlobBytesFinished(int64 request_id,
On 2014/01/08 17:48:06, vandebo wrote:
> Remove

Done. Whoops. Thanks.

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/media_ga...
File chrome/utility/media_galleries/ipc_data_source.cc (right):

https://codereview.chromium.org/103283003/diff/400002/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_data_source.cc:48:
content::UtilityThread::Get()->Send(new ChromeUtilityHostMsg_RequestBlobBytes(
On 2014/01/08 17:48:06, vandebo wrote:
> note: I guess this is more correctly named UtilityProcessDataSource since it
can
> only be used from the utility process.  But IPCDataSource seems fine as well.

Yeah that's true. It's hard to name this thing. Maybe
BrowserProcessToUtilityProcessIPCTunneledDataSource.

[vandebo (ex-Chrome), 2014-01-08 20:18:21]

LGTM

[tommycli, 2014-01-08 20:28:06]

thestig: Need OWNER review on chrome/chrome.gyp and
chrome/utility/chrome_content_utility_client.*
tsepez: Need OWNER review on chrome/common/chrome_utility_messages.h

[Tom Sepez, 2014-01-08 22:53:00]

Messages LGTM.

[tommycli, 2014-01-10 19:06:50]

thestig: Can I get a quick looksee on this one? I think it's been mostly
reviewed already.

[Lei Zhang, 2014-01-10 20:46:09]

lgtm

https://codereview.chromium.org/103283003/diff/910001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/910001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:88:
make_scoped_ptr(metadata_dictionary.DeepCopy()))));
Given then way its used in SendMediaMetadataToHost(), probably easier to just
do: base::Owned(metadata_dictionary.DeepCopy())

https://codereview.chromium.org/103283003/diff/910001/chrome/utility/media_ga...
File chrome/utility/media_galleries/ipc_data_source.h (right):

https://codereview.chromium.org/103283003/diff/910001/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_data_source.h:45: int64 total_size_;
const

[tommycli, 2014-01-10 22:41:07]

thestig: Thanks!

https://codereview.chromium.org/103283003/diff/910001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/910001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:88:
make_scoped_ptr(metadata_dictionary.DeepCopy()))));
On 2014/01/10 20:46:09, Lei Zhang wrote:
> Given then way its used in SendMediaMetadataToHost(), probably easier to just
> do: base::Owned(metadata_dictionary.DeepCopy())

For some reason, changing the callback sig to a raw ptr and using base::Owned
made the NULL case not work. (I use NULL in OnProcessCrashed).

https://codereview.chromium.org/103283003/diff/910001/chrome/utility/media_ga...
File chrome/utility/media_galleries/ipc_data_source.h (right):

https://codereview.chromium.org/103283003/diff/910001/chrome/utility/media_ga...
chrome/utility/media_galleries/ipc_data_source.h:45: int64 total_size_;
On 2014/01/10 20:46:09, Lei Zhang wrote:
> const

Done.

[Lei Zhang, 2014-01-10 22:48:48]

https://codereview.chromium.org/103283003/diff/910001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/910001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:88:
make_scoped_ptr(metadata_dictionary.DeepCopy()))));
On 2014/01/10 22:41:08, tommycli wrote:
> On 2014/01/10 20:46:09, Lei Zhang wrote:
> > Given then way its used in SendMediaMetadataToHost(), probably easier to
just
> > do: base::Owned(metadata_dictionary.DeepCopy())
> 
> For some reason, changing the callback sig to a raw ptr and using base::Owned
> made the NULL case not work. (I use NULL in OnProcessCrashed).

Does passing an empty dummy DictionaryValue work? With base::Owned, you can't
have a Null const ref.

[tommycli, 2014-01-10 23:33:59]

https://codereview.chromium.org/103283003/diff/910001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc
(right):

https://codereview.chromium.org/103283003/diff/910001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_media_metadata_parser.cc:88:
make_scoped_ptr(metadata_dictionary.DeepCopy()))));
On 2014/01/10 22:48:48, Lei Zhang wrote:
> On 2014/01/10 22:41:08, tommycli wrote:
> > On 2014/01/10 20:46:09, Lei Zhang wrote:
> > > Given then way its used in SendMediaMetadataToHost(), probably easier to
> just
> > > do: base::Owned(metadata_dictionary.DeepCopy())
> > 
> > For some reason, changing the callback sig to a raw ptr and using
base::Owned
> > made the NULL case not work. (I use NULL in OnProcessCrashed).
> 
> Does passing an empty dummy DictionaryValue work? With base::Owned, you can't
> have a Null const ref.

Yes! That does work. I'm not sure how i feel about the dummy, but i guess
OnProcessCrashed is an exceptional case.

[commit-bot: I haz the power, 2014-01-11 00:14:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/103283003/1090001

[commit-bot: I haz the power, 2014-01-11 02:25:41]

Change committed as 244310