Add Team Drive subtree to the directory list view.

Add Team Drive subtree to the directory list view.

Team Drive files are loaded to /team_drives directory. This change will
show it in the UI.

Google Drive(volume)
 - My Drive
 - "Team Drives"
   - "ABC team drive"

Before the change, only the root entries and fake entries were shown
with special icon (as opposed to the folder icon) and label (from
localized string resource, as opposed to filename of each entry).

Now we are showing "Team Drives" and "ABC team drive" with the special
icon just like such root/fake entries.
However, the label of "ABC team drive" should be the name of the entry
(which comes from the name of a Team Drive). hasIndividualName property
is introduced for this reason.


TEST=tested manually with --team-drive
TEST=browser_tests --gtest_filter=FileManagerJsTest.*:GalleryBrowserTest*
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2839863002
Cr-Commit-Position: refs/heads/master@{#468579}
Committed: https://chromium.googlesource.com/chromium/src/+/317eb67a43957db557f51f360dfeb3a40ffdd690

[yamaguchi, 2017-04-25 07:12:52]

Description was changed from

==========
Add Team Drive subtree to the directory list view.


TEST=tested manually with --team-drive
BUG=684275
==========

to

==========
Add Team Drive subtree to the directory list view.


TEST=tested manually with --team-drive
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yamaguchi, 2017-04-25 07:17:26]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 07:17:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 08:29:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 08:29:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yamaguchi, 2017-04-25 09:10:34]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 09:10:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 09:41:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 09:41:33]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2017-04-25 09:44:16]

Description was changed from

==========
Add Team Drive subtree to the directory list view.


TEST=tested manually with --team-drive
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add Team Drive subtree to the directory list view.


TEST=tested manually with --team-drive
TEST=browser_tests
--gtest_filter=FileManagerJsTest.VolumeManagerTest:FileManagerJsTest.DirectoryTreeTest
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yamaguchi, 2017-04-25 09:44:36]

yamaguchi@chromium.org changed reviewers:
+ fukino@chromium.org

[yamaguchi, 2017-04-25 09:44:37]

ptal

[fukino, 2017-04-25 15:06:37]

Could you elaborate non-trivial patches like this?

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/entry_location_impl.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/entry_location_impl.js:53:
this.hasFixedLabel = [
nit: I think
 entryType == AAA ||
 entryType == BBB ||
 ...

is more straightforward.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/volume_info_impl.js:61:
VolumeInfoImpl.teamDrivesEnabled().then(function(result) {
nit: I don't think we need to add a function to wrap a call by a promise.

chrome.commandLinePrivate.hasSwitch('', function(enabled) {
  this.isTeamDrivesEnabled_ = enabled;
}.bind(this));

is sufficient.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/util.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/util.js:641: var tree =
entry.fullPath.split('/');
FakeEntry does not have fullPath.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/util.js:642: if (tree.length != 3)
nit: maybe return "tree.length == 3 && util.isTeamDriveEntry(entry)"?

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/volume_manager_common.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/volume_manager_common.js:74: VOLUME_ROOT:
'VOLUME_ROOT',
'volume_root' for consistency. ditto for others.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/location_line.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/location_line.js:5: 
remove

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/location_line.js:99: if
(entry.fullPath.indexOf(VolumeManagerCommon.TEAM_DRIVES_DIRECTORY_PATH) ==
you can use String.prototype.startsWith.

[fukino, 2017-04-25 15:07:11]

> Could you elaborate non-trivial patches like this?
In the bug description :)

[fukino, 2017-04-25 15:07:51]

On 2017/04/25 15:07:11, fukino wrote:
> > Could you elaborate non-trivial patches like this?
> In the bug description :)

s/bug/CL/
I should go sleep...

[yamaguchi, 2017-04-26 03:12:56]

Description was changed from

==========
Add Team Drive subtree to the directory list view.


TEST=tested manually with --team-drive
TEST=browser_tests
--gtest_filter=FileManagerJsTest.VolumeManagerTest:FileManagerJsTest.DirectoryTreeTest
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add Team Drive subtree to the directory list view.

Team Drive files are loaded to /team_drives directory. This change will
show it in the UI.

Google Drive(volume)
 - My Drive
 - "Team Drives"
   - "ABC team drive"

Before the change, root entries and fake entries are shown with both special
icon (as opposed to the folder icon) and label (from localized string resource,
as opposed to filename of each entry).

"ABC team drive" is the name of the entry (which comes from the name of a Team
Drive) but we want to use special icon. hasIndividualName property is introduced
for this reason.


TEST=tested manually with --team-drive
TEST=browser_tests
--gtest_filter=FileManagerJsTest.VolumeManagerTest:FileManagerJsTest.DirectoryTreeTest
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yamaguchi, 2017-04-26 07:45:42]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 07:45:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2017-04-26 08:12:57]

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/entry_location_impl.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/entry_location_impl.js:53:
this.hasFixedLabel = [
On 2017/04/25 15:06:37, fukino wrote:
> nit: I think
>  entryType == AAA ||
>  entryType == BBB ||
>  ...
> 
> is more straightforward.

Done.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/volume_info_impl.js:61:
VolumeInfoImpl.teamDrivesEnabled().then(function(result) {
On 2017/04/25 15:06:37, fukino wrote:
> nit: I don't think we need to add a function to wrap a call by a promise.
> 
> chrome.commandLinePrivate.hasSwitch('', function(enabled) {
>   this.isTeamDrivesEnabled_ = enabled;
> }.bind(this));
> 
> is sufficient.

Done.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/volume_info_impl.js:74: if
(volumeType === VolumeManagerCommon.VolumeType.DRIVE) {
This causes "Uncaught ReferenceError: VolumeManagerCommon is not defined" error
when running under browser_tests, although it passes.

I am doubting having the value in VolumeManagerCommon.
VolumeManagerCommon says that it is:
> * Namespace for common types shared between VolumeManager and
> * VolumeManagerWrapper.

Would it worth creating a new file for the constants, to be shared between
foreground and background? Or do we already have appropriate file for this
purpose?

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/util.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/util.js:641: var tree =
entry.fullPath.split('/');
On 2017/04/25 15:06:37, fukino wrote:
> FakeEntry does not have fullPath.

Done.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/util.js:642: if (tree.length != 3)
On 2017/04/25 15:06:37, fukino wrote:
> nit: maybe return "tree.length == 3 && util.isTeamDriveEntry(entry)"?

Done.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/volume_manager_common.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/volume_manager_common.js:74: VOLUME_ROOT:
'VOLUME_ROOT',
On 2017/04/25 15:06:37, fukino wrote:
> 'volume_root' for consistency. ditto for others.

Done.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/location_line.js (right):

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/location_line.js:5: 
On 2017/04/25 15:06:37, fukino wrote:
> remove

Done.

https://codereview.chromium.org/2839863002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/location_line.js:99: if
(entry.fullPath.indexOf(VolumeManagerCommon.TEAM_DRIVES_DIRECTORY_PATH) ==
On 2017/04/25 15:06:37, fukino wrote:
> you can use String.prototype.startsWith.

Done.

https://codereview.chromium.org/2839863002/diff/40001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/directory_tree_unittest.js
(right):

https://codereview.chromium.org/2839863002/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/directory_tree_unittest.js:71:
function testCreateDirectoryTree(callback) {
Now I am working to add similar test to this with the Team Drives flag enabled
if it's feasible.

[commit-bot: I haz the power, 2017-04-26 09:39:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 09:39:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yamaguchi, 2017-04-26 12:25:32]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 12:25:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 14:22:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 14:22:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yamaguchi, 2017-04-27 02:32:21]

NMW

https://codereview.chromium.org/2839863002/diff/60001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/60001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/volume_info_impl.js:61:
chrome.commandLinePrivate.hasSwitch('team-drives', function(enabled) {
This line causes crash of some tests in browser_tests unless adding appropriate
mocks.
Also the Gallery app fails to start.

[yamaguchi, 2017-04-27 06:07:15]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 06:07:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 07:26:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 07:26:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yamaguchi, 2017-04-27 07:40:28]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 07:40:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 08:07:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 08:07:05]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2017-04-27 08:36:06]

ptal

https://codereview.chromium.org/2839863002/diff/60001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/60001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/volume_info_impl.js:61:
chrome.commandLinePrivate.hasSwitch('team-drives', function(enabled) {
On 2017/04/27 02:32:21, yamaguchi wrote:
> This line causes crash of some tests in browser_tests unless adding
appropriate
> mocks.
> Also the Gallery app fails to start.

Fixed the both.

[yamaguchi, 2017-04-27 08:41:09]

Description was changed from

==========
Add Team Drive subtree to the directory list view.

Team Drive files are loaded to /team_drives directory. This change will
show it in the UI.

Google Drive(volume)
 - My Drive
 - "Team Drives"
   - "ABC team drive"

Before the change, root entries and fake entries are shown with both special
icon (as opposed to the folder icon) and label (from localized string resource,
as opposed to filename of each entry).

"ABC team drive" is the name of the entry (which comes from the name of a Team
Drive) but we want to use special icon. hasIndividualName property is introduced
for this reason.


TEST=tested manually with --team-drive
TEST=browser_tests
--gtest_filter=FileManagerJsTest.VolumeManagerTest:FileManagerJsTest.DirectoryTreeTest
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add Team Drive subtree to the directory list view.

Team Drive files are loaded to /team_drives directory. This change will
show it in the UI.

Google Drive(volume)
 - My Drive
 - "Team Drives"
   - "ABC team drive"

Before the change, only the root entries and fake entries were shown
with special icon (as opposed to the folder icon) and label (from
localized string resource, as opposed to filename of each entry).

Now we are showing "Team Drives" and "ABC team drive" with the special
icon just like such root/fake entries.
However, the label of "ABC team drive" should be the name of the entry
(which comes from the name of a Team Drive). hasIndividualName property
is introduced for this reason.


TEST=tested manually with --team-drive
TEST=browser_tests --gtest_filter=FileManagerJsTest.*:GalleryBrowserTest*
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2017-04-27 10:44:58]

Sorry! I'll look into more tomorrow.
I'm trying to understand the clear meaning of each property in
VolumeInfo/LocationInfo and see if the new property is necessary
based on
https://docs.google.com/spreadsheets/d/1vYogPPtg8PumB6S0MDPIf6ZOykBBDOBGfQB4D...

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/mock_volume_manager.js (right):

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/mock_volume_manager.js:248: 
blank line is added unintentionally?

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:58: /** @type
{boolean}} */
Remove the extra '}'.
(I was surprised this passes the compiler...)

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/galler...
File ui/file_manager/gallery/manifest.json (right):

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/galler...
ui/file_manager/gallery/manifest.json:27: "commandLinePrivate",
nit: Alphabetise

[yamaguchi, 2017-04-28 11:12:16]

Instead of introducing the |EntryType|, we could add 1 more different RootTypes
enum value.
                RootType               isRoot
Team Drives   - TEAM_DRIVES_GRAND_ROOT true
+Team Drive A - TEAM_DRIVE             true
 +File B      - TEAM_DRIVE             false

hasFixedLabel can be computed by these values.

I'll try to see if this actually works.

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/entry_location_impl.js (right):

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/entry_location_impl.js:35:
this.isDriveBased =
RootType.TEAM_DRIVES is also based on Drive.
Currently we use isDriveBased property for
- when typing into search box, show auto-completion instead of incremental
search
- show/hide "share" button
Added it for the first usage.

However the "sharing" the root or a sub folder in a Team Drive is not feasible.
In the future we'd want to have it "add member to team drive" instead of sharing
it.

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/mock_volume_manager.js (right):

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/mock_volume_manager.js:248: 
On 2017/04/27 10:44:57, fukino wrote:
> blank line is added unintentionally?

Reverted.

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:58: /** @type
{boolean}} */
On 2017/04/27 10:44:57, fukino wrote:
> Remove the extra '}'.
> (I was surprised this passes the compiler...)

Done.

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/galler...
File ui/file_manager/gallery/manifest.json (right):

https://codereview.chromium.org/2839863002/diff/100001/ui/file_manager/galler...
ui/file_manager/gallery/manifest.json:27: "commandLinePrivate",
On 2017/04/27 10:44:58, fukino wrote:
> nit: Alphabetise

Done.

[yamaguchi, 2017-04-28 12:33:16]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 12:33:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2017-04-28 12:43:34]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 12:43:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2017-04-28 12:44:16]

On 2017/04/28 11:12:16, yamaguchi wrote:
> Instead of introducing the |EntryType|, we could add 1 more different
RootTypes
> enum value.
>                 RootType               isRoot
> Team Drives   - TEAM_DRIVES_GRAND_ROOT true
> +Team Drive A - TEAM_DRIVE             true
>  +File B      - TEAM_DRIVE             false
> 
> hasFixedLabel can be computed by these values.
> 
> I'll try to see if this actually works.
> 

Revised reflecting this idea. PTAL.

[commit-bot: I haz the power, 2017-04-28 14:34:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 14:34:39]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-05-01 00:31:42]

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:71: /** @type
{Promise.<!Array<!DirectoryEntry>>} */
nit: Promise<!Array...
Could you remove '.' from the annotation for this.displayRootPromise_, too?

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:72:
this.driveDisplayRootPromises_ = null;
Do we need this member variable? It's accessed only in one function.

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:244: var
displayRootPromise =
I think adding following block and keeping other parts as they were makes the
intention clearer.
WDYT?

if (this.isTeamDrivesEnabled_) {
  // Make sure that the TeamDrives display root is resolved when Drive root is
resolved.
  this.displayRootPromise_ = Promise.all([
    this.displayRootPromise_,
    new.Promise(window.webkit....)
  ]).then(function(displayRoots) {
    this.teamDriveDisplayRoot_ = displayRoots[1];
    return displayRoots[0];
  });
}

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/ui/directory_tree.js (right):

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:750: var
fixedEntries = [this.entry];
nit: Fake entries are also fixed entries (with fixed labels), so this variable
name sounds a bit confusing to me.
Could you this variable name as entries, and instead, rename the |entries| in
line #754 to |fakeEntries|, and then "entries = entries.concat(fakeEntries)" in
line #765?

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:789: // First
item is My Drive, and the second item is Team Drives.
nit: First => The first

[yamaguchi, 2017-05-01 08:33:55]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 08:34:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 09:04:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 09:04:40]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2017-05-01 09:05:25]

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:71: /** @type
{Promise.<!Array<!DirectoryEntry>>} */
On 2017/05/01 00:31:41, fukino wrote:
> nit: Promise<!Array...
> Could you remove '.' from the annotation for this.displayRootPromise_, too?

Done.

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:72:
this.driveDisplayRootPromises_ = null;
On 2017/05/01 00:31:41, fukino wrote:
> Do we need this member variable? It's accessed only in one function.

Done.
No, we don't need to have it a member variable since we ceased to return it by a
getter function like the other one.

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:244: var
displayRootPromise =
On 2017/05/01 00:31:41, fukino wrote:
> I think adding following block and keeping other parts as they were makes the
> intention clearer.
> WDYT?
> 
> if (this.isTeamDrivesEnabled_) {
>   // Make sure that the TeamDrives display root is resolved when Drive root is
> resolved.
>   this.displayRootPromise_ = Promise.all([
>     this.displayRootPromise_,
>     new.Promise(window.webkit....)
>   ]).then(function(displayRoots) {
>     this.teamDriveDisplayRoot_ = displayRoots[1];
>     return displayRoots[0];
>   });
> }

Done.
Overwriting with the result of .then() sounds a nice idea. Thanks!

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/ui/directory_tree.js (right):

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:750: var
fixedEntries = [this.entry];
On 2017/05/01 00:31:42, fukino wrote:
> nit: Fake entries are also fixed entries (with fixed labels), so this variable
> name sounds a bit confusing to me.
> Could you this variable name as entries, and instead, rename the |entries| in
> line #754 to |fakeEntries|, and then "entries = entries.concat(fakeEntries)"
in
> line #765?

Done.

https://codereview.chromium.org/2839863002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:789: // First
item is My Drive, and the second item is Team Drives.
On 2017/05/01 00:31:42, fukino wrote:
> nit: First => The first

Done.

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:248: .all([
This may look weired, but it's the result of the auto formatter.

[fukino, 2017-05-02 04:23:54]

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:235:
this.displayRootPromise_.then(function(displayRoot) {
These lines can be removed. It's already done in L264.

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:257:
console.log(displayRoots);
Remove this debug log.

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:259:
}.bind(this), opt_onFailure);
remove opt_onFailure. It can be called twice, since we already have the
rejection handler in L269.

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:262: // store the
obtained displayRoot.
Please revert the change in L262-266.

[yamaguchi, 2017-05-02 05:07:38]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 05:07:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2017-05-02 05:34:52]

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
File ui/file_manager/file_manager/background/js/volume_info_impl.js (right):

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:235:
this.displayRootPromise_.then(function(displayRoot) {
On 2017/05/02 04:23:54, fukino wrote:
> These lines can be removed. It's already done in L264.

Done.

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:257:
console.log(displayRoots);
On 2017/05/02 04:23:54, fukino wrote:
> Remove this debug log.

Done.

https://codereview.chromium.org/2839863002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/background/js/volume_info_impl.js:259:
}.bind(this), opt_onFailure);
On 2017/05/02 04:23:54, fukino wrote:
> remove opt_onFailure. It can be called twice, since we already have the
> rejection handler in L269.

Done.

[commit-bot: I haz the power, 2017-05-02 05:42:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 05:42:21]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-05-02 07:23:15]

LGTM. Thank you!

[yamaguchi, 2017-05-02 07:23:43]

The CQ bit was checked by yamaguchi@chromium.org

[yamaguchi, 2017-05-02 07:23:56]

On 2017/05/02 07:23:15, fukino wrote:
> LGTM. Thank you!

Thanks so much!

[commit-bot: I haz the power, 2017-05-02 07:24:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 07:29:01]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1493709823299910,
"parent_rev": "99175652a133cca12297714b2cfbfc89b910cba3", "commit_rev":
"317eb67a43957db557f51f360dfeb3a40ffdd690"}

[commit-bot: I haz the power, 2017-05-02 07:29:37]

Description was changed from

==========
Add Team Drive subtree to the directory list view.

Team Drive files are loaded to /team_drives directory. This change will
show it in the UI.

Google Drive(volume)
 - My Drive
 - "Team Drives"
   - "ABC team drive"

Before the change, only the root entries and fake entries were shown
with special icon (as opposed to the folder icon) and label (from
localized string resource, as opposed to filename of each entry).

Now we are showing "Team Drives" and "ABC team drive" with the special
icon just like such root/fake entries.
However, the label of "ABC team drive" should be the name of the entry
(which comes from the name of a Team Drive). hasIndividualName property
is introduced for this reason.


TEST=tested manually with --team-drive
TEST=browser_tests --gtest_filter=FileManagerJsTest.*:GalleryBrowserTest*
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add Team Drive subtree to the directory list view.

Team Drive files are loaded to /team_drives directory. This change will
show it in the UI.

Google Drive(volume)
 - My Drive
 - "Team Drives"
   - "ABC team drive"

Before the change, only the root entries and fake entries were shown
with special icon (as opposed to the folder icon) and label (from
localized string resource, as opposed to filename of each entry).

Now we are showing "Team Drives" and "ABC team drive" with the special
icon just like such root/fake entries.
However, the label of "ABC team drive" should be the name of the entry
(which comes from the name of a Team Drive). hasIndividualName property
is introduced for this reason.


TEST=tested manually with --team-drive
TEST=browser_tests --gtest_filter=FileManagerJsTest.*:GalleryBrowserTest*
BUG=684275
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2839863002
Cr-Commit-Position: refs/heads/master@{#468579}
Committed:
https://chromium.googlesource.com/chromium/src/+/317eb67a43957db557f51f360dfe...
==========

[commit-bot: I haz the power, 2017-05-02 07:29:39]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/317eb67a43957db557f51f360dfe...