Add a flag to specify whether to show ARC Files app in launcher.

Add a flag to specify whether to show ARC Files app in launcher.

BUG=685312
TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*"

Review-Url: https://codereview.chromium.org/2648263007
Cr-Commit-Position: refs/heads/master@{#447318}
Committed: https://chromium.googlesource.com/chromium/src/+/bc979776bf3949f8111d18f3e476833de024d8e1

[xutan, 2017-01-25 21:06:28]

Description was changed from

==========
Add a flag to specify whether to show ARC Files app in launcher.

BUG=685312
TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*"
==========

to

==========
Add a flag to specify whether to show ARC Files app in launcher.

BUG=685312
TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*"
==========

[xutan, 2017-01-25 21:06:28]

xutan@chromium.org changed reviewers:
+ hirono@chromium.org

[xutan, 2017-01-25 21:15:45]

Just to add a placeholder flag. The actual implementation is out of the scope of
this CL.

[hirono, 2017-01-26 01:17:50]

https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:2220: {"show-arc-files-app",
IDS_FLAGS_SHOW_ARC_FILES_APP_NAME,
Do we need to publish the flag for non-developers?
If no, I think just adding the launch flag without exposing it in the flags page
is enough for now because the flag does not change anything now.

[xutan, 2017-01-26 02:45:15]

On 2017/01/26 01:17:50, hirono wrote:
>
https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags.cc
> File chrome/browser/about_flags.cc (right):
> 
>
https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags....
> chrome/browser/about_flags.cc:2220: {"show-arc-files-app",
> IDS_FLAGS_SHOW_ARC_FILES_APP_NAME,
> Do we need to publish the flag for non-developers?
> If no, I think just adding the launch flag without exposing it in the flags
page
> is enough for now because the flag does not change anything now.

From what I heard is we'd like to publish it to non-developers through
about:flags to give testers a way to launch ARC Files app. I'll double check w/
folks on our Jan. 26.

[hirono, 2017-01-26 03:34:39]

SGTM and LGTM

[Steve McKay, 2017-01-26 19:36:26]

smckay@chromium.org changed reviewers:
+ smckay@chromium.org

[Steve McKay, 2017-01-26 19:36:27]

https://codereview.chromium.org/2648263007/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/2648263007/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:92: Show ARC Files app
I'm not sure we want to use the word "ARC" in user visible strings (even strings
for nerds fishing around in about:flags). Ping Weifang to see what kind of
terminology to use. Maybe we call it the "Next Generation" file manager. Or
something like that.

https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:2220: {"show-arc-files-app",
IDS_FLAGS_SHOW_ARC_FILES_APP_NAME,
On 2017/01/26 01:17:50, hirono wrote:
> Do we need to publish the flag for non-developers?
> If no, I think just adding the launch flag without exposing it in the flags
page
> is enough for now because the flag does not change anything now.

I think we want to let users turn this on on stable releases. We don't expect
many to do it, but it's part of the path to being enabled by default.


Still, we should verify w/ Weifang.

[xutan, 2017-01-26 20:37:35]

xutan@chromium.org changed reviewers:
+ weifangsun@google.com

[xutan, 2017-01-26 21:27:01]

xutan@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-01-27 17:33:46]

https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:2222:
SINGLE_VALUE_TYPE(chromeos::switches::kShowArcFilesApp)},
For new enable/disable style flags, the best practice is to use a base::Feature
- which you can use FEATURE_VALUE_TYPE() here for. Can you switch to that?

[xutan, 2017-01-27 22:39:39]

xutan@chromium.org changed reviewers:
+ yusukes@chromium.org

[xutan, 2017-01-27 22:56:43]

On 2017/01/27 17:33:46, Alexei Svitkine (very slow) wrote:
>
https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags.cc
> File chrome/browser/about_flags.cc (right):
> 
>
https://codereview.chromium.org/2648263007/diff/1/chrome/browser/about_flags....
> chrome/browser/about_flags.cc:2222:
> SINGLE_VALUE_TYPE(chromeos::switches::kShowArcFilesApp)},
> For new enable/disable style flags, the best practice is to use a
base::Feature
> - which you can use FEATURE_VALUE_TYPE() here for. Can you switch to that?

Done!

[Alexei Svitkine (slow), 2017-01-27 23:10:33]

lgtm

[Yusuke Sato, 2017-01-30 20:29:31]

lgtm

[xutan, 2017-01-31 17:55:15]

The CQ bit was checked by xutan@chromium.org

[xutan, 2017-01-31 17:55:16]

The patchset sent to the CQ was uploaded after l-g-t-m from hirono@chromium.org,
asvitkine@chromium.org, yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2648263007/#ps60001
(title: "Revise flag name and flag description per PM's suggestion.")

[commit-bot: I haz the power, 2017-01-31 17:55:46]

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-01-31 17:57:45]

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

[commit-bot: I haz the power, 2017-01-31 17:57:47]

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...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[xutan, 2017-01-31 19:35:29]

The CQ bit was checked by xutan@chromium.org

[xutan, 2017-01-31 19:35:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, hirono@chromium.org, yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2648263007/#ps80001
(title: "Change the description again after discussion. Rebase on remote
master.")

[commit-bot: I haz the power, 2017-01-31 19:36:23]

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-01-31 20:41:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485891329043410,
"parent_rev": "aac65d39bae46b220c1ca90cf24fb59be7dc3cbc", "commit_rev":
"bc979776bf3949f8111d18f3e476833de024d8e1"}

[commit-bot: I haz the power, 2017-01-31 20:42:11]

Description was changed from

==========
Add a flag to specify whether to show ARC Files app in launcher.

BUG=685312
TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*"
==========

to

==========
Add a flag to specify whether to show ARC Files app in launcher.

BUG=685312
TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*"

Review-Url: https://codereview.chromium.org/2648263007
Cr-Commit-Position: refs/heads/master@{#447318}
Committed:
https://chromium.googlesource.com/chromium/src/+/bc979776bf3949f8111d18f3e476...
==========

[commit-bot: I haz the power, 2017-01-31 20:42:13]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/bc979776bf3949f8111d18f3e476...