Media Galleries: Move gallery name generation back to StorageMonitor.

Media Galleries: Move gallery name generation back to StorageMonitor.

The name generation is used by multiple consumers, so it is better to put it back where is most accessible.

BUG=261350

[Lei Zhang, 2013-07-23 07:00:14]

In r199264, we stopped setting StorageInfo's name field, but we are still using
it in a few places. This CL gets rid of it so it altogether so nobody can use it
by accident.

Patch set 1 should fix everything on Linux. Assuming this is ok, we'll continue
and fix the other platforms in later patch sets.

[Lei Zhang, 2013-07-23 07:03:31]

The end-to-end tests that should have caught this regression failed because they
continued to set the name field, whereas the production code no longer did.

[vandebo (ex-Chrome), 2013-07-23 23:28:55]

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:93:
base::ASCIIToUTF16(kDeviceName), base::string16(),
Is this to make the devices distinguishable or something?

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_info.cc:215: if (!base_path.empty() &&
(base_path.value() != location_))
This is kind of ugly.  Generally this should happen, but it does since you've
abused location in the MediaGalleryPrefInfo call site.  I'm not sure if there's
a cleaner way around, but it's worth thinking about.

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor_linux.cc (left):

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor_linux.cc:205: string16(),
Other storage monitor impls need the same treatment?

[Lei Zhang, 2013-07-24 05:13:33]

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc
(right):

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_eject_apitest.cc:93:
base::ASCIIToUTF16(kDeviceName), base::string16(),
On 2013/07/23 23:28:55, vandebo wrote:
> Is this to make the devices distinguishable or something?

Yes, for removable devices, the storage label field has similar effects as the
name field did previously. If we never put in |kDeviceName| somewhere, how can
we test for it later in JS?

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_info.cc:215: if (!base_path.empty() &&
(base_path.value() != location_))
On 2013/07/23 23:28:55, vandebo wrote:
> This is kind of ugly.  Generally this should happen, but it does since you've
> abused location in the MediaGalleryPrefInfo call site.  I'm not sure if
there's
> a cleaner way around, but it's worth thinking about.

Not sure what you mean. Let's chat tomorrow.

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor_linux.cc (left):

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor_linux.cc:205: string16(),
On 2013/07/23 23:28:55, vandebo wrote:
> Other storage monitor impls need the same treatment?

Yes, they will once we are happy with the Linux side first.

[Greg Billock, 2013-07-29 18:37:01]

https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc
(right):

https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:90:
details.device_name = base::UTF16ToUTF8(info.GetDisplayName(true));
This was the locus of the bug, right? Should this get split off from the CL to
be back-patched?

[Lei Zhang, 2013-07-29 20:48:27]

I think patch set 4 should work. Will rebase it in patch set 5 and run it
through the trybots.

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/19489006/diff/27001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_info.cc:215: if (!base_path.empty() &&
(base_path.value() != location_))
On 2013/07/24 05:13:34, Lei Zhang wrote:
> On 2013/07/23 23:28:55, vandebo wrote:
> > This is kind of ugly.  Generally this should happen, but it does since
you've
> > abused location in the MediaGalleryPrefInfo call site.  I'm not sure if
> there's
> > a cleaner way around, but it's worth thinking about.
> 
> Not sure what you mean. Let's chat tomorrow.

Ah, I think you were trying to say StorageInfo's |location_| should not include
the relative path from the path to device. I put the sub-folder display code
back where it was before.

https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc
(right):

https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:90:
details.device_name = base::UTF16ToUTF8(info.GetDisplayName(true));
On 2013/07/29 18:37:01, Greg Billock wrote:
> This was the locus of the bug, right? Should this get split off from the CL to
> be back-patched?

But StorageInfo::GetDisplayName() is being added in this CL. It's not something
trivial that can be easily split off.

[Greg Billock, 2013-07-29 21:44:22]

https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc
(right):

https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:90:
details.device_name = base::UTF16ToUTF8(info.GetDisplayName(true));
On 2013/07/29 20:48:27, Lei Zhang wrote:
> On 2013/07/29 18:37:01, Greg Billock wrote:
> > This was the locus of the bug, right? Should this get split off from the CL
to
> > be back-patched?
> 
> But StorageInfo::GetDisplayName() is being added in this CL. It's not
something
> trivial that can be easily split off.

You could call MediaGalleryPrefInfo::GetGalleryDisplayName instead. A hassle,
but likely more easily patched.

[Lei Zhang, 2013-07-29 21:46:58]

On 2013/07/29 21:44:22, Greg Billock wrote:
>
https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
> File
>
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc
> (right):
> 
>
https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
>
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:90:
> details.device_name = base::UTF16ToUTF8(info.GetDisplayName(true));
> On 2013/07/29 20:48:27, Lei Zhang wrote:
> > On 2013/07/29 18:37:01, Greg Billock wrote:
> > > This was the locus of the bug, right? Should this get split off from the
CL
> to
> > > be back-patched?
> > 
> > But StorageInfo::GetDisplayName() is being added in this CL. It's not
> something
> > trivial that can be easily split off.
> 
> You could call MediaGalleryPrefInfo::GetGalleryDisplayName instead. A hassle,
> but likely more easily patched.

Nope, I previously added a DEPS rule to explicitly disallow c/b/storage_monitor
from calling into c/b/media_galleries to avoid circular dependencies.

[Greg Billock, 2013-07-29 23:32:51]

This is in chrome/browser/extensions. It can make use of
c/b/media_galleries apis.


On Mon, Jul 29, 2013 at 2:47 PM, <thestig@chromium.org> wrote:

> On 2013/07/29 21:44:22, Greg Billock wrote:
>
> https://codereview.chromium.**org/19489006/diff/46006/**
> chrome/browser/extensions/api/**media_galleries_private/media_**
>
galleries_private_event_**router.cc<https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc>
>
>> File
>>
>
> chrome/browser/extensions/api/**media_galleries_private/media_**
> galleries_private_event_**router.cc
>
>> (right):
>>
>
>
> https://codereview.chromium.**org/19489006/diff/46006/**
> chrome/browser/extensions/api/**media_galleries_private/media_**
>
galleries_private_event_**router.cc#newcode90<https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc#newcode90>
>
> chrome/browser/extensions/api/**media_galleries_private/media_**
> galleries_private_event_**router.cc:90:
>
>> details.device_name = base::UTF16ToUTF8(info.**GetDisplayName(true));
>> On 2013/07/29 20:48:27, Lei Zhang wrote:
>> > On 2013/07/29 18:37:01, Greg Billock wrote:
>> > > This was the locus of the bug, right? Should this get split off from
>> the
>>
> CL
>
>> to
>> > > be back-patched?
>> >
>> > But StorageInfo::GetDisplayName() is being added in this CL. It's not
>> something
>> > trivial that can be easily split off.
>>
>
>  You could call MediaGalleryPrefInfo::**GetGalleryDisplayName instead. A
>> hassle,
>> but likely more easily patched.
>>
>
> Nope, I previously added a DEPS rule to explicitly disallow
> c/b/storage_monitor
> from calling into c/b/media_galleries to avoid circular dependencies.
>
>
https://codereview.chromium.**org/19489006/<https://codereview.chromium.org/1...
>

[Lei Zhang, 2013-07-30 02:39:55]

https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc
(right):

https://codereview.chromium.org/19489006/diff/46006/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:90:
details.device_name = base::UTF16ToUTF8(info.GetDisplayName(true));
On 2013/07/29 21:44:22, Greg Billock wrote:
> On 2013/07/29 20:48:27, Lei Zhang wrote:
> > On 2013/07/29 18:37:01, Greg Billock wrote:
> > > This was the locus of the bug, right? Should this get split off from the
CL
> to
> > > be back-patched?
> > 
> > But StorageInfo::GetDisplayName() is being added in this CL. It's not
> something
> > trivial that can be easily split off.
> 
> You could call MediaGalleryPrefInfo::GetGalleryDisplayName instead. A hassle,
> but likely more easily patched.

Oh right. Will do in a new CL.

[Lei Zhang, 2013-07-30 22:29:11]

Patch set 8 should work on all platforms. The TestNameGeneration test failure is
handled separately in https://codereview.chromium.org/21302002/

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

On 2013/07/30 22:29:11, Lei Zhang wrote:
> Patch set 8 should work on all platforms. The TestNameGeneration test failure
is
> handled separately in https://codereview.chromium.org/21302002/

Any particular reason this change went stale?

[Lei Zhang, 2014-01-08 20:47:57]

On 2014/01/08 20:01:24, vandebo wrote:
> On 2013/07/30 22:29:11, Lei Zhang wrote:
> > Patch set 8 should work on all platforms. The TestNameGeneration test
failure
> is
> > handled separately in https://codereview.chromium.org/21302002/
> 
> Any particular reason this change went stale?

Nobody reviewed it and it got lonely.