Migrate from fullPath to Entry in the mosaic view.

Migrate from fullPath to Entry in the mosaic view.

This patch removes the last instance of fullPath in the mosaic view and replaces it with use of the volume manager to extract the location information.

TEST=Tested manually.
BUG=320967
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244126

[mtomasz, 2014-01-09 02:42:53]

@hirono: PTAL. Thx.

[hirono, 2014-01-09 04:05:39]

https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js
(right):

https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js:128:
self.volumeManager_ = volumeManager;
Is this member used?

https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js:1638: *
@param {EntryLocation} locationInfo Location information for the tile.
How about passing hidpiEmbedded flag instead of location information?
It reduces dependency.

[mtomasz, 2014-01-10 01:40:10]

https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js
(right):

https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js:128:
self.volumeManager_ = volumeManager;
On 2014/01/09 04:05:39, hirono wrote:
> Is this member used?

Yes, in #149.

https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js:1638: *
@param {EntryLocation} locationInfo Location information for the tile.
On 2014/01/09 04:05:39, hirono wrote:
> How about passing hidpiEmbedded flag instead of location information?
> It reduces dependency.

I was thinking about it. This would limit dependencies, but spread
responsibilities over multiple classes. Determining if hidpi is enabled should
be done by the tile class in my opinion. I think LocationInfo is lightweight
enough to be passed. WDYT?

[hirono, 2014-01-10 03:18:23]

https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js
(right):

https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js:1638: *
@param {EntryLocation} locationInfo Location information for the tile.
On 2014/01/10 01:40:10, mtomasz wrote:
> On 2014/01/09 04:05:39, hirono wrote:
> > How about passing hidpiEmbedded flag instead of location information?
> > It reduces dependency.
> 
> I was thinking about it. This would limit dependencies, but spread
> responsibilities over multiple classes. Determining if hidpi is enabled should
> be done by the tile class in my opinion. I think LocationInfo is lightweight
> enough to be passed. WDYT?

So how about getting the flag in the constructor or the decorate method? It also
reduces the dependency but spreads the responsibilities.

[mtomasz, 2014-01-10 03:43:17]

On 2014/01/10 03:18:23, hirono wrote:
>
https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
> File chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js
> (right):
> 
>
https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js:1638:
*
> @param {EntryLocation} locationInfo Location information for the tile.
> On 2014/01/10 01:40:10, mtomasz wrote:
> > On 2014/01/09 04:05:39, hirono wrote:
> > > How about passing hidpiEmbedded flag instead of location information?
> > > It reduces dependency.
> > 
> > I was thinking about it. This would limit dependencies, but spread
> > responsibilities over multiple classes. Determining if hidpi is enabled
should
> > be done by the tile class in my opinion. I think LocationInfo is lightweight
> > enough to be passed. WDYT?
> 
> So how about getting the flag in the constructor or the decorate method? It
also
> reduces the dependency but spreads the responsibilities.

Can you please clarify? How is it different than your previous suggestion? Thx!

[hirono, 2014-01-10 03:50:20]

On 2014/01/10 03:43:17, mtomasz wrote:
> On 2014/01/10 03:18:23, hirono wrote:
> >
>
https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
> > File
chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/130383003/diff/1/chrome/browser/resources/fil...
> >
chrome/browser/resources/file_manager/foreground/js/photo/mosaic_mode.js:1638:
> *
> > @param {EntryLocation} locationInfo Location information for the tile.
> > On 2014/01/10 01:40:10, mtomasz wrote:
> > > On 2014/01/09 04:05:39, hirono wrote:
> > > > How about passing hidpiEmbedded flag instead of location information?
> > > > It reduces dependency.
> > > 
> > > I was thinking about it. This would limit dependencies, but spread
> > > responsibilities over multiple classes. Determining if hidpi is enabled
> should
> > > be done by the tile class in my opinion. I think LocationInfo is
lightweight
> > > enough to be passed. WDYT?
> > 
> > So how about getting the flag in the constructor or the decorate method? It
> also
> > reduces the dependency but spreads the responsibilities.
> 
> Can you please clarify? How is it different than your previous suggestion?
Thx!

The first suggestion is that the client code obtain the flag and passing it to
the constructor of Mosaic.Tile.
The second suggesting is that the client code passes the volume information and
the constructor of Mosaic.Tile obtains the flag and store it,
where the client code does not responsible for obtaining the flag, and
Mosaic.Tile object does not hold a dependency of VolumeInfo.

[mtomasz, 2014-01-10 04:51:47]

Done! PTAL.

[hirono, 2014-01-10 05:00:40]

Thanks! lgtm!

[commit-bot: I haz the power, 2014-01-10 05:01:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/130383003/90001

[commit-bot: I haz the power, 2014-01-10 08:29:01]

Change committed as 244126