Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693093002:
Fetch file metadata of files under Team Drives. (Closed)
DescriptionFetch file metadata of files under Team Drives.
The function is hidden behind a flag (--team-drives).
After this change, the Files app. will be able to search files in
Team Drives of which the user has access right.
BUG=649911
TEST=google_apis_unittests
TEST=manual test by searching a Team Drive file/folder in My Drive.
Review-Url: https://codereview.chromium.org/2693093002
Cr-Commit-Position: refs/heads/master@{#452033}
Committed: https://chromium.googlesource.com/chromium/src/+/eb24cc590ef75ceee37b0b3290e9dc6f5dac6e9c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove redundant de-referencing. #
Total comments: 12
Patch Set 3 : Move flag definition out to a separate file. Don't add to about:flags for now. #Patch Set 4 : Revert addition of unused string to .grdp. The flag is not yet exposed to about:flags. #
Total comments: 2
Patch Set 5 : Add copyright comments. #
Total comments: 8
Patch Set 6 : Reflect review comments. #Patch Set 7 : Fix comment. #
Total comments: 2
Patch Set 8 : Add unitttest. #Patch Set 9 : Use the generated copy constructor by compiler. #
Total comments: 12
Patch Set 10 : Inject the commandline switch by all constructor invocations. #Patch Set 11 : Resolve compilation error. #
Total comments: 4
Patch Set 12 : Rename the enum to TeamDrivesIntegrationStatus to be more descriptive. #Messages
Total messages: 86 (57 generated)
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yamaguchi@chromium.org changed reviewers: + fukino@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Could you check the compile failure and find a review in OWNERS? https://codereview.chromium.org/2693093002/diff/1/google_apis/drive/drive_api... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_url_generator.cc:25: const char kDriveV2BetaChangelistUrl[] = "drive/v2beta/changes"; Are the new APIs available only in v2beta? If it's already available in v2 or v2internal, we should go with it.
On 2017/02/14 19:24:22, fukino wrote: > Could you check the compile failure and find a review in OWNERS? > > https://codereview.chromium.org/2693093002/diff/1/google_apis/drive/drive_api... > File google_apis/drive/drive_api_url_generator.cc (right): > > https://codereview.chromium.org/2693093002/diff/1/google_apis/drive/drive_api... > google_apis/drive/drive_api_url_generator.cc:25: const char > kDriveV2BetaChangelistUrl[] = "drive/v2beta/changes"; > Are the new APIs available only in v2beta? > If it's already available in v2 or v2internal, we should go with it. As far as I tried, it is available only in v2beta now. I will confirm with the team.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yamaguchi@chromium.org changed reviewers: + haraken@chromium.org, hashimoto@chromium.org
PTAL. +hashimoto for google_apis/drive/* +haraken for histograms.xml https://codereview.chromium.org/2693093002/diff/1/google_apis/drive/drive_api... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_url_generator.cc:25: const char kDriveV2BetaChangelistUrl[] = "drive/v2beta/changes"; On 2017/02/14 19:24:22, fukino wrote: > Are the new APIs available only in v2beta? > If it's already available in v2 or v2internal, we should go with it. It's available only in v2beta right now.
haraken@chromium.org changed reviewers: + isherman@chromium.org
+isherman (I'm not eligible to review histogram.xml other than UseCounter changes.)
histograms.xml lgtm
Do we really want to expose this feature in about:flags now? Shouldn't we wait until this feature gets some usefulness? For development, you can manually add the commandline option when launching chrome. https://codereview.chromium.org/2693093002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2693093002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2249: SINGLE_VALUE_TYPE("team-drives")}, Please use constant. https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:58: const char kEnableTeamDrives[] = "team-drives"; Please put switch constants in a separate file. Usually, they are put in a file whose file name ends with _switch.h/cc. https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:119: const char* url_prefix; Please don't leave this value uninitialized. https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:197: GURL url = base_url_.Resolve(kDriveV2FilesUrl); Please be consistent with the code below which puts url = base_url_.Resolve(kDriveV2FilesUrl) in a else clause. https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.h:25: DriveApiUrlGenerator(const DriveApiUrlGenerator& src); Why do you need to have a copy ctor?
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the good catch. We'll just have it a simple flag without exposing to about:flags for development now. https://codereview.chromium.org/2693093002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2693093002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2249: SINGLE_VALUE_TYPE("team-drives")}, On 2017/02/20 03:26:29, hashimoto wrote: > Please use constant. Reverted this change as we won't expose this as about:flags by this change. https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:58: const char kEnableTeamDrives[] = "team-drives"; On 2017/02/20 03:26:30, hashimoto wrote: > Please put switch constants in a separate file. > Usually, they are put in a file whose file name ends with _switch.h/cc. Done. https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:119: const char* url_prefix; On 2017/02/20 03:26:29, hashimoto wrote: > Please don't leave this value uninitialized. Is this what you meant? (or did you mean make this variable non-const and initialize with something like NULL?) https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:197: GURL url = base_url_.Resolve(kDriveV2FilesUrl); On 2017/02/20 03:26:29, hashimoto wrote: > Please be consistent with the code below which puts url = > base_url_.Resolve(kDriveV2FilesUrl) in a else clause. Done. https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.h:25: DriveApiUrlGenerator(const DriveApiUrlGenerator& src); On 2017/02/20 03:26:30, hashimoto wrote: > Why do you need to have a copy ctor? https://www.chromium.org/developers/coding-style/chromium-style-checker-errors Chromium style checker gives error as "Complex class/struct needs an explicit out-of-line copy constructor." without this. It's because the class's complexity score reaches the threshold (10 points) as we add 1 integral variable (bool) to the class, in addition to the 3 existing non-PoD members (which cost 3 points each).
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2693093002/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_switches.h (right): https://codereview.chromium.org/2693093002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_switches.h:1: #ifndef GOOGLE_APIS_DRIVE_DRIVE_SWITHES_H_ License must match: .*? Copyright (\(c\) )?(2017|2016|2015|2014|2013|2012|2011|2010|2009|2008|2007|2006|2006-2008|2006-2009|2006-2010) The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2693093002/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_switches.h (right): https://codereview.chromium.org/2693093002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_switches.h:1: #ifndef GOOGLE_APIS_DRIVE_DRIVE_SWITHES_H_ On 2017/02/20 09:51:32, yamaguchi wrote: > License must match: > .*? Copyright (\(c\) > )?(2017|2016|2015|2014|2013|2012|2011|2010|2009|2008|2007|2006|2006-2008|2006-2009|2006-2010) > The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is > governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: > \*/)?\n Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:119: const char* url_prefix; On 2017/02/20 08:21:29, yamaguchi wrote: > On 2017/02/20 03:26:29, hashimoto wrote: > > Please don't leave this value uninitialized. > > Is this what you meant? (or did you mean make this variable non-const and > initialize with something like NULL?) I meant just initializing with nullptr. https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_switches.cc (right): https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_switches.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: No "(c)" needed. https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_switches.cc:11: const char kEnableTeamDrives[] = "team-drives"; 1. Use constexpr. 2. This constant is only reference in this compilation unit. Please put it in an unnamed namespace. https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_switches.h (right): https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_switches.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: No "(c)" needed. https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_switches.h:10: // Enables or disables Team Drives integration. This function doesn't enable or disable the feature, it just checks the status, right?
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/20001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:119: const char* url_prefix; On 2017/02/21 10:19:10, hashimoto wrote: > On 2017/02/20 08:21:29, yamaguchi wrote: > > On 2017/02/20 03:26:29, hashimoto wrote: > > > Please don't leave this value uninitialized. > > > > Is this what you meant? (or did you mean make this variable non-const and > > initialize with something like NULL?) > > I meant just initializing with nullptr. Done. https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_switches.cc (right): https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_switches.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/02/21 10:19:10, hashimoto wrote: > nit: No "(c)" needed. Done. https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_switches.cc:11: const char kEnableTeamDrives[] = "team-drives"; On 2017/02/21 10:19:10, hashimoto wrote: > 1. Use constexpr. > 2. This constant is only reference in this compilation unit. Please put it in an > unnamed namespace. Done. https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_switches.h (right): https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_switches.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/02/21 10:19:10, hashimoto wrote: > nit: No "(c)" needed. Done. https://codereview.chromium.org/2693093002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_switches.h:10: // Enables or disables Team Drives integration. On 2017/02/21 10:19:10, hashimoto wrote: > This function doesn't enable or disable the feature, it just checks the status, > right? Right. It was a simple mistake.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Could test the new code paths? Sorry for not catching this earlier. https://codereview.chromium.org/2693093002/diff/120001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.cc:66: DriveApiUrlGenerator::DriveApiUrlGenerator(const DriveApiUrlGenerator& src) Can't you just use "= default"?
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by yamaguchi@chromium.org
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fetch file metadata of files under Team Drives. The function is hidden behind a flag (--team-drives). After this change, the Files app. will be able to search files in Team Drives of which the user has access right. BUG=649911 TEST=manual test by searching a Team Drive file/folder in My Drive. ========== to ========== Fetch file metadata of files under Team Drives. The function is hidden behind a flag (--team-drives). After this change, the Files app. will be able to search files in Team Drives of which the user has access right. BUG=649911 TEST=google_apis_unittests TEST=manual test by searching a Team Drive file/folder in My Drive. ==========
https://codereview.chromium.org/2693093002/diff/120001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.cc:66: DriveApiUrlGenerator::DriveApiUrlGenerator(const DriveApiUrlGenerator& src) On 2017/02/22 03:52:03, hashimoto wrote: > Can't you just use "= default"? Done.
On 2017/02/22 03:52:03, hashimoto wrote: > Could test the new code paths? > Sorry for not catching this earlier. ping? Could you test the new code? > > https://codereview.chromium.org/2693093002/diff/120001/google_apis/drive/driv... > File google_apis/drive/drive_api_url_generator.cc (right): > > https://codereview.chromium.org/2693093002/diff/120001/google_apis/drive/driv... > google_apis/drive/drive_api_url_generator.cc:66: > DriveApiUrlGenerator::DriveApiUrlGenerator(const DriveApiUrlGenerator& src) > Can't you just use "= default"?
On 2017/02/22 06:43:10, hashimoto wrote: > On 2017/02/22 03:52:03, hashimoto wrote: > > Could test the new code paths? > > Sorry for not catching this earlier. > ping? > Could you test the new code? > > Added some test cases. May I have a question, what is our usual way to test this type of class that reads commandline flags, and are run normal unit test (as opposed to browser_tests)? Should all the flags be read outside such class and injected by constructor?
The value should be injected, or you can expose a setter. https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:24: const GURL& base_thumbnail_url); You don't need to have this ctor when there is the one below. https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:29: bool is_team_drive_enabled); Boolean argument is usually difficult to read. Our style guide says "Consider changing the function signature to replace a bool argument with an enum argument." https://google.github.io/styleguide/cppguide.html#Variable_Comments https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator_unittest.cc (right): https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator_unittest.cc:178: const std::string& kV2FilesUrlPrefix = This shouldn't be a reference. constexpr char[] or const std::string https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator_unittest.cc:180: const std::string& kV2BetaFilesUrlPrefix = ditto. https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator_unittest.cc:270: const std::string& kV2ChangesUrlPrefix = ditto. https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator_unittest.cc:272: const std::string& kV2BetaChangesUrlPrefix = ditto.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:24: const GURL& base_thumbnail_url); On 2017/02/22 07:04:51, hashimoto wrote: > You don't need to have this ctor when there is the one below. Removed as well as changing all constructor calls. https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:29: bool is_team_drive_enabled); On 2017/02/22 07:04:51, hashimoto wrote: > Boolean argument is usually difficult to read. > > Our style guide says "Consider changing the function signature to replace a bool > argument with an enum argument." > https://google.github.io/styleguide/cppguide.html#Variable_Comments Done. https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator_unittest.cc (right): https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator_unittest.cc:178: const std::string& kV2FilesUrlPrefix = On 2017/02/22 07:04:51, hashimoto wrote: > This shouldn't be a reference. > constexpr char[] or const std::string Done. https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator_unittest.cc:180: const std::string& kV2BetaFilesUrlPrefix = On 2017/02/22 07:04:51, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator_unittest.cc:270: const std::string& kV2ChangesUrlPrefix = On 2017/02/22 07:04:51, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2693093002/diff/160001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator_unittest.cc:272: const std::string& kV2BetaChangesUrlPrefix = On 2017/02/22 07:04:51, hashimoto wrote: > ditto. Done.
On 2017/02/22 07:04:52, hashimoto wrote: > The value should be injected, or you can expose a setter. I chose to inject the value.
lgtm with nits https://codereview.chromium.org/2693093002/diff/200001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/200001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.cc:67: default; nit: Put this ctor after the ctor below to make it consistent with the header. https://codereview.chromium.org/2693093002/diff/200001/google_apis/drive/driv... File google_apis/drive/drive_switches.h (right): https://codereview.chromium.org/2693093002/diff/200001/google_apis/drive/driv... google_apis/drive/drive_switches.h:10: enum TeamDrivesIntegration { TeamDrivesIntegration sounds like a name of a class which implements the said feature. How about something like TeamDrivesIntegrationStatus?
https://codereview.chromium.org/2693093002/diff/200001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2693093002/diff/200001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.cc:67: default; On 2017/02/22 08:50:21, hashimoto wrote: > nit: Put this ctor after the ctor below to make it consistent with the header. Done. https://codereview.chromium.org/2693093002/diff/200001/google_apis/drive/driv... File google_apis/drive/drive_switches.h (right): https://codereview.chromium.org/2693093002/diff/200001/google_apis/drive/driv... google_apis/drive/drive_switches.h:10: enum TeamDrivesIntegration { On 2017/02/22 08:50:21, hashimoto wrote: > TeamDrivesIntegration sounds like a name of a class which implements the said > feature. > How about something like TeamDrivesIntegrationStatus? Done.
The CQ bit was checked by yamaguchi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2693093002/#ps220001 (title: "Rename the enum to TeamDrivesIntegrationStatus to be more descriptive.")
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
+fukino@ Will you review and approve for chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc ?
lgtm for chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc
The CQ bit was checked by yamaguchi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487770140790780,
"parent_rev": "3b7045893315213e96b80e6f46cf257756086939", "commit_rev":
"eb24cc590ef75ceee37b0b3290e9dc6f5dac6e9c"}
Message was sent while issue was closed.
Description was changed from ========== Fetch file metadata of files under Team Drives. The function is hidden behind a flag (--team-drives). After this change, the Files app. will be able to search files in Team Drives of which the user has access right. BUG=649911 TEST=google_apis_unittests TEST=manual test by searching a Team Drive file/folder in My Drive. ========== to ========== Fetch file metadata of files under Team Drives. The function is hidden behind a flag (--team-drives). After this change, the Files app. will be able to search files in Team Drives of which the user has access right. BUG=649911 TEST=google_apis_unittests TEST=manual test by searching a Team Drive file/folder in My Drive. Review-Url: https://codereview.chromium.org/2693093002 Cr-Commit-Position: refs/heads/master@{#452033} Committed: https://chromium.googlesource.com/chromium/src/+/eb24cc590ef75ceee37b0b3290e9... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/eb24cc590ef75ceee37b0b3290e9... |
