Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2359003004:
Disable ACTION_BOOT_COMPLETED event for 3rd party apps. (Closed)
|
Created:
4 years, 3 months ago by xzhou Modified:
4 years, 2 months ago Reviewers:
mitsuji, Daniel Erat, asvitkine_google, jam, rickyz (no longer on Chrome), Alexei Svitkine (slow), Luis Héctor Chávez CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, asvitkine+watch_chromium.org, hashimoto+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, yusukes+watch_chromium.org, jschuh Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable ACTION_BOOT_COMPLETED event for 3rd party apps.
Android apps can use the broadcast to start automatically. This
Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party
applications.
BUG=650313
TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*"
Committed: https://crrev.com/938368c527b95c74317edc7e12924d13787df57e
Cr-Commit-Position: refs/heads/master@{#422707}
Patch Set 1 #Patch Set 2 : Revise user facing strings in chrome://flags #
Total comments: 2
Patch Set 3 : Add TEST and crbug #Patch Set 4 : Add owners of files touched. #Patch Set 5 : Replace flags with feature list. #
Total comments: 4
Patch Set 6 : Change flag variable name and make it enabled by default #
Total comments: 11
Patch Set 7 : Fix comments by Alexei. #Patch Set 8 : Revise flags strings. #
Total comments: 3
Patch Set 9 : Revise flag string. #
Total comments: 15
Patch Set 10 : Fix Daniel's comments #
Total comments: 6
Patch Set 11 : Add new line at end of arc_features.h #Patch Set 12 : Remove _arc_ in some variables and comments #
Total comments: 2
Patch Set 13 : Refine flag detail string. #Patch Set 14 : Change load to start. #Messages
Total messages: 106 (64 generated)
Description was changed from ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=30836420 ========== to ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=30836420 ==========
xzhou@chromium.org changed reviewers: + lhchavez@chromium.org, mitsuji@chromium.org, rickyz@chromium.org
The CQ bit was checked by xzhou@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.
rickyz@, lhchavez@, I deleted the previous code review for technique issues. Could you please review this one. Thanks. Mitsuji@, could you please review the file chrome/app/chromeos_strings.grdp and let me know how to polish the strings. Thanks a lot.
The commit message is missing a TEST= field. You should also probably file a bug in crbug.com/ and use that ID instead of the b/ one. Make sure both bugs reference each other in a comment. Other than that, lgtm. https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:6317: <message name="IDS_FLAGS_ARC_ACTION_BOOT_COMPLETED" desc="Name of the flag for blocking the ACTION_BOOT_COMPLETED broadcast for 3rd party apps on ARC."> nit: s/3rd party/third-party/. Same in L6320.
On 2016/09/24 at 02:03:25, lhchavez wrote: > The commit message is missing a TEST= field. You should also probably file a bug in crbug.com/ and use that ID instead of the b/ one. Make sure both bugs reference each other in a comment. > > Other than that, lgtm. > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > File chrome/app/chromeos_strings.grdp (right): > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > chrome/app/chromeos_strings.grdp:6317: <message name="IDS_FLAGS_ARC_ACTION_BOOT_COMPLETED" desc="Name of the flag for blocking the ACTION_BOOT_COMPLETED broadcast for 3rd party apps on ARC."> > nit: s/3rd party/third-party/. Same in L6320. Oops sorry, I had actually sticking with the internal bug when xzhou@ asked (though for future notice, the bug format for internal bugs is BUG=b:1 to disambiguate from crbug.com bugs) - is there a general rule of thumb for where things should be tracked (I guess always create both)? The git history seemed to show a mix.
On 2016/09/25 21:06:23, rickyz wrote: > On 2016/09/24 at 02:03:25, lhchavez wrote: > > The commit message is missing a TEST= field. You should also probably file a > bug in crbug.com/ and use that ID instead of the b/ one. Make sure both bugs > reference each other in a comment. > > > > Other than that, lgtm. > > > > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > > File chrome/app/chromeos_strings.grdp (right): > > > > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > > chrome/app/chromeos_strings.grdp:6317: <message > name="IDS_FLAGS_ARC_ACTION_BOOT_COMPLETED" desc="Name of the flag for blocking > the ACTION_BOOT_COMPLETED broadcast for 3rd party apps on ARC."> > > nit: s/3rd party/third-party/. Same in L6320. > > Oops sorry, I had actually sticking with the internal bug when xzhou@ asked > (though for future notice, the bug format for internal bugs is BUG=b:1 to > disambiguate from http://crbug.com bugs) - is there a general rule of thumb for where > things should be tracked (I guess always create both)? The git history seemed to > show a mix. Fixed Luis's comments and created a crbug.
Fixed Luis' comments and created crbug 650313. https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:6317: <message name="IDS_FLAGS_ARC_ACTION_BOOT_COMPLETED" desc="Name of the flag for blocking the ACTION_BOOT_COMPLETED broadcast for 3rd party apps on ARC."> On 2016/09/24 02:03:25, Luis Héctor Chávez wrote: > nit: s/3rd party/third-party/. Same in L6320. Done.
The CQ bit was checked by xzhou@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 xzhou@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...)
Description was changed from ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=30836420 ========== to ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=650313 ==========
The CQ bit was checked by xzhou@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.
On 2016/09/26 18:13:30, xzhou wrote: > Fixed Luis' comments and created crbug 650313. > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > File chrome/app/chromeos_strings.grdp (right): > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > chrome/app/chromeos_strings.grdp:6317: <message > name="IDS_FLAGS_ARC_ACTION_BOOT_COMPLETED" desc="Name of the flag for blocking > the ACTION_BOOT_COMPLETED broadcast for 3rd party apps on ARC."> > On 2016/09/24 02:03:25, Luis Héctor Chávez wrote: > > nit: s/3rd party/third-party/. Same in L6320. > > Done. fyi, in Rietveld (this code review site), if you amend the commit message locally and do `git cl upload`, the commit message is not updated. You need to use the "Edit Issue" link in order to add the TEST= field.
On 2016/09/25 21:06:23, rickyz wrote: > On 2016/09/24 at 02:03:25, lhchavez wrote: > > The commit message is missing a TEST= field. You should also probably file a > bug in crbug.com/ and use that ID instead of the b/ one. Make sure both bugs > reference each other in a comment. > > > > Other than that, lgtm. > > > > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > > File chrome/app/chromeos_strings.grdp (right): > > > > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > > chrome/app/chromeos_strings.grdp:6317: <message > name="IDS_FLAGS_ARC_ACTION_BOOT_COMPLETED" desc="Name of the flag for blocking > the ACTION_BOOT_COMPLETED broadcast for 3rd party apps on ARC."> > > nit: s/3rd party/third-party/. Same in L6320. > > Oops sorry, I had actually sticking with the internal bug when xzhou@ asked > (though for future notice, the bug format for internal bugs is BUG=b:1 to > disambiguate from http://crbug.com bugs) - is there a general rule of thumb for where > things should be tracked (I guess always create both)? The git history seemed to > show a mix. Some reviewers say that we should _always_ use crbug here, others are more lenient towards it (hence the lack of consistency). I am of the opinion that we should try always using crbug, since it provides additional advantages, like being able to get merges approved faster (if necessary in the future).
Description was changed from ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=650313 ========== to ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=650313 TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*" ==========
On 2016/09/27 15:32:57, Luis Héctor Chávez wrote: > On 2016/09/25 21:06:23, rickyz wrote: > > On 2016/09/24 at 02:03:25, lhchavez wrote: > > > The commit message is missing a TEST= field. You should also probably file a > > bug in crbug.com/ and use that ID instead of the b/ one. Make sure both bugs > > reference each other in a comment. > > > > > > Other than that, lgtm. > > > > > > > > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > > > File chrome/app/chromeos_strings.grdp (right): > > > > > > > > > https://codereview.chromium.org/2359003004/diff/20001/chrome/app/chromeos_str... > > > chrome/app/chromeos_strings.grdp:6317: <message > > name="IDS_FLAGS_ARC_ACTION_BOOT_COMPLETED" desc="Name of the flag for blocking > > the ACTION_BOOT_COMPLETED broadcast for 3rd party apps on ARC."> > > > nit: s/3rd party/third-party/. Same in L6320. > > > > Oops sorry, I had actually sticking with the internal bug when xzhou@ asked > > (though for future notice, the bug format for internal bugs is BUG=b:1 to > > disambiguate from http://crbug.com bugs) - is there a general rule of thumb > for where > > things should be tracked (I guess always create both)? The git history seemed > to > > show a mix. > > Some reviewers say that we should _always_ use crbug here, others are more > lenient towards it (hence the lack of consistency). I am of the opinion that we > should try always using crbug, since it provides additional advantages, like > being able to get merges approved faster (if necessary in the future). Fixed TEST field in commit message. It seems crbug is correctly linked while b/ bugs is not. So I used crbug here.
xzhou@chromium.org changed reviewers: + asvitkine@google.com, jam@chromium.org
Add file owners for review.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Instead of adding a new flag, have you considered using the base::Feature API, which is now best practice for enable/disable style flags. See go/feature-api-announcement
which parts do you want me to look at?
On 2016/09/29 20:09:55, jam wrote: > which parts do you want me to look at? Thanks for your quick reply. I selected you as a reviewer just because 'git cl owner' shows you own most of the files I modified (first time using 'git cl owner'). I will change the reviewer if you are busy. If you like, could you please help me to review chromeos/dbus/ folder? This CL adds an option to allow user block Android apps from start automatically in ARC++. Thanks a lot.
On 2016/09/29 16:49:11, Alexei Svitkine (slow) wrote:
> Instead of adding a new flag, have you considered using the base::Feature API,
> which is now best practice for enable/disable style flags.
>
> See go/feature-api-announcement
Hi, Alexei:
Thanks for your comments, I tried feature-api and it works but won't pass
pre-submit duo to the error below. I can not include chrome dependencies in ARC.
So I will keep original changes.
======== Copied Error Message ========
** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
components/arc/arc_bridge_bootstrap.cc
Illegal include: "chrome/common/chrome_features.h"
Because of no rule applying.
Presubmit checks took 15.9s to calculate.
If your code is in components/arc - then define your base::Feature there too. On Fri, Sep 30, 2016 at 2:47 PM, <xzhou@chromium.org> wrote: > On 2016/09/29 16:49:11, Alexei Svitkine (slow) wrote: > > Instead of adding a new flag, have you considered using the > base::Feature API, > > which is now best practice for enable/disable style flags. > > > > See go/feature-api-announcement > <https://goto.google.com/feature-api-announcement> > > Hi, Alexei: > > Thanks for your comments, I tried feature-api and it works but won't pass > pre-submit duo to the error below. I can not include chrome dependencies > in ARC. > So I will keep original changes. > > ======== Copied Error Message ======== > ** Presubmit ERRORS ** > You added one or more #includes that violate checkdeps rules. > components/arc/arc_bridge_bootstrap.cc > Illegal include: "chrome/common/chrome_features.h" > Because of no rule applying. > > Presubmit checks took 15.9s to calculate. > > https://codereview.chromium.org/2359003004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi, Alexei: Thanks for your suggestions. I have moved the features to components/arc/arc_features.[h|cc]. And reverted the original change in chromeos/chromeos_switches. The change is in patch set 5. Thanks. On 2016/09/30 18:58:10, chromium-reviews wrote: > If your code is in components/arc - then define your base::Feature there > too. > > On Fri, Sep 30, 2016 at 2:47 PM, <mailto:xzhou@chromium.org> wrote: > > > On 2016/09/29 16:49:11, Alexei Svitkine (slow) wrote: > > > Instead of adding a new flag, have you considered using the > > base::Feature API, > > > which is now best practice for enable/disable style flags. > > > > > > See go/feature-api-announcement > > <https://goto.google.com/feature-api-announcement> > > > > Hi, Alexei: > > > > Thanks for your comments, I tried feature-api and it works but won't pass > > pre-submit duo to the error below. I can not include chrome dependencies > > in ARC. > > So I will keep original changes. > > > > ======== Copied Error Message ======== > > ** Presubmit ERRORS ** > > You added one or more #includes that violate checkdeps rules. > > components/arc/arc_bridge_bootstrap.cc > > Illegal include: "chrome/common/chrome_features.h" > > Because of no rule applying. > > > > Presubmit checks took 15.9s to calculate. > > > > https://codereview.chromium.org/2359003004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by xzhou@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/2359003004/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2359003004/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2111: }, Nit: This should go on the previous line - as it done for other entries. https://codereview.chromium.org/2359003004/diff/80001/components/arc/arc_feat... File components/arc/arc_features.cc (right): https://codereview.chromium.org/2359003004/diff/80001/components/arc/arc_feat... components/arc/arc_features.cc:11: "DisableArcBootCompletedBroadcast", base::FEATURE_DISABLED_BY_DEFAULT}; Instead of making the feature called kDisable* and making it disabled by default (double negative), make the feature kArcBootCompletedBroadcast and make it enabled by default.
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 xzhou@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...
On 2016/09/30 21:34:04, Alexei Svitkine (slow) wrote: > https://codereview.chromium.org/2359003004/diff/80001/chrome/browser/about_fl... > File chrome/browser/about_flags.cc (right): > > https://codereview.chromium.org/2359003004/diff/80001/chrome/browser/about_fl... > chrome/browser/about_flags.cc:2111: }, > Nit: This should go on the previous line - as it done for other entries. > > https://codereview.chromium.org/2359003004/diff/80001/components/arc/arc_feat... > File components/arc/arc_features.cc (right): > > https://codereview.chromium.org/2359003004/diff/80001/components/arc/arc_feat... > components/arc/arc_features.cc:11: "DisableArcBootCompletedBroadcast", > base::FEATURE_DISABLED_BY_DEFAULT}; > Instead of making the feature called kDisable* and making it disabled by default > (double negative), make the feature kArcBootCompletedBroadcast and make it > enabled by default. Changed in Patch 6. Thanks.
Changed variable name as well as flag descriptions. See Patch 6. Thanks. https://codereview.chromium.org/2359003004/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2359003004/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2111: }, On 2016/09/30 21:34:04, Alexei Svitkine (slow) wrote: > Nit: This should go on the previous line - as it done for other entries. Done. https://codereview.chromium.org/2359003004/diff/80001/components/arc/arc_feat... File components/arc/arc_features.cc (right): https://codereview.chromium.org/2359003004/diff/80001/components/arc/arc_feat... components/arc/arc_features.cc:11: "DisableArcBootCompletedBroadcast", base::FEATURE_DISABLED_BY_DEFAULT}; On 2016/09/30 21:34:04, Alexei Svitkine (slow) wrote: > Instead of making the feature called kDisable* and making it disabled by default > (double negative), make the feature kArcBootCompletedBroadcast and make it > enabled by default. I changed the variable name as well as the description. But the variable name in arc_bridge_bootstrap.cc is not changed because the IPC is already defined. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) 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 xzhou@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...)
lgtm https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... File components/arc/arc_features.cc (right): https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... components/arc/arc_features.cc:9: // Disable ACTION_BOOT_COMPLETD broadcast for 3rd party applications on ARC. Nit: Update comment. You can still mention that the feature provides the ability to disable.
Actually, have some more comments - but lgtm % those. https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... File components/arc/arc_features.cc (right): https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... components/arc/arc_features.cc:3: // found in the LICENSE file. Nit: Add empty line below. https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... components/arc/arc_features.cc:14: } Nit: Add // namespace comment https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... File components/arc/arc_features.h (right): https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... components/arc/arc_features.h:12: namespace features { Nit: This should probably use "arc" namespace same as the rest of the component. https://codereview.chromium.org/2359003004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2359003004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:86356: + <int value="-1922697944" label="DisableArcBootCompletedBroadcast:disabled"/> Please fix these, however.
not lgtm https://codereview.chromium.org/2359003004/diff/100001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/100001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6318: Android apps load automatically Please reach out to mistuji@ before making any changes on these strings. These have grammar errors that need to be corrected as well.
The CQ bit was checked by xzhou@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...
On 2016/10/03 15:43:27, Luis Héctor Chávez wrote: > not lgtm > > https://codereview.chromium.org/2359003004/diff/100001/chrome/app/chromeos_st... > File chrome/app/chromeos_strings.grdp (right): > > https://codereview.chromium.org/2359003004/diff/100001/chrome/app/chromeos_st... > chrome/app/chromeos_strings.grdp:6318: Android apps load automatically > Please reach out to mistuji@ before making any changes on these strings. These > have grammar errors that need to be corrected as well. Updated the flag strings, see comments at https://b.corp.google.com/issues/30836420.
On 2016/10/03 15:04:32, Alexei Svitkine (slow) wrote: > Actually, have some more comments - but lgtm % those. > > https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... > File components/arc/arc_features.cc (right): > > https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... > components/arc/arc_features.cc:3: // found in the LICENSE file. > Nit: Add empty line below. > > https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... > components/arc/arc_features.cc:14: } > Nit: Add // namespace comment > > https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... > File components/arc/arc_features.h (right): > > https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... > components/arc/arc_features.h:12: namespace features { > Nit: This should probably use "arc" namespace same as the rest of the component. > > https://codereview.chromium.org/2359003004/diff/100001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2359003004/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:86356: + <int value="-1922697944" > label="DisableArcBootCompletedBroadcast:disabled"/> > Please fix these, however. Done.
The CQ bit was checked by xzhou@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...
In the future, please reply to comments individually via the review tool, rather than quoting multiple comments at once. On Mon, Oct 3, 2016 at 3:04 PM, <xzhou@chromium.org> wrote: > On 2016/10/03 15:43:27, Luis Héctor Chávez wrote: > > not lgtm > > > > > https://codereview.chromium.org/2359003004/diff/100001/ > chrome/app/chromeos_strings.grdp > > File chrome/app/chromeos_strings.grdp (right): > > > > > https://codereview.chromium.org/2359003004/diff/100001/ > chrome/app/chromeos_strings.grdp#newcode6318 > > chrome/app/chromeos_strings.grdp:6318: Android apps load automatically > > Please reach out to mistuji@ before making any changes on these > strings. These > > have grammar errors that need to be corrected as well. > > Updated the flag strings, see comments at > https://b.corp.google.com/issues/30836420. > > https://codereview.chromium.org/2359003004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2359003004/diff/140001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/140001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6321: Allow Android apps load automatically after startup and reboot. Enabled by default. Allow Android apps to load automatically after startup and reboot. Enabled by default.
https://codereview.chromium.org/2359003004/diff/140001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/140001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6321: Allow Android apps load automatically after startup and reboot. Enabled by default. On 2016/10/03 19:49:00, Luis Héctor Chávez wrote: > Allow Android apps to load automatically after startup and reboot. Enabled by > default. FYI: already chatted with mitsuji@ and he made the same comment.
The CQ bit was checked by xzhou@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...
On 2016/10/03 19:53:49, Luis Héctor Chávez wrote: > https://codereview.chromium.org/2359003004/diff/140001/chrome/app/chromeos_st... > File chrome/app/chromeos_strings.grdp (right): > > https://codereview.chromium.org/2359003004/diff/140001/chrome/app/chromeos_st... > chrome/app/chromeos_strings.grdp:6321: Allow Android apps load automatically > after startup and reboot. Enabled by default. > On 2016/10/03 19:49:00, Luis Héctor Chávez wrote: > > Allow Android apps to load automatically after startup and reboot. Enabled by > > default. > > FYI: already chatted with mitsuji@ and he made the same comment. Thanks, Luis. I fixed this in patch set 9.
remember, for each comment in the review, you need to click on the comment's "Done" or "Reply" links. anyways, lgtm again.
Revised flag strings and fixed a couple of nit comments. https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... File components/arc/arc_features.cc (right): https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... components/arc/arc_features.cc:3: // found in the LICENSE file. On 2016/10/03 15:04:31, Alexei Svitkine (slow) wrote: > Nit: Add empty line below. Done. https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... components/arc/arc_features.cc:9: // Disable ACTION_BOOT_COMPLETD broadcast for 3rd party applications on ARC. On 2016/10/03 15:02:48, Alexei Svitkine (slow) wrote: > Nit: Update comment. You can still mention that the feature provides the ability > to disable. Done. https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... components/arc/arc_features.cc:14: } On 2016/10/03 15:04:31, Alexei Svitkine (slow) wrote: > Nit: Add // namespace comment Done. https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... File components/arc/arc_features.h (right): https://codereview.chromium.org/2359003004/diff/100001/components/arc/arc_fea... components/arc/arc_features.h:12: namespace features { On 2016/10/03 15:04:32, Alexei Svitkine (slow) wrote: > Nit: This should probably use "arc" namespace same as the rest of the component. Done. https://codereview.chromium.org/2359003004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2359003004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:86356: + <int value="-1922697944" label="DisableArcBootCompletedBroadcast:disabled"/> On 2016/10/03 15:04:32, Alexei Svitkine (slow) wrote: > Please fix these, however. Done. https://codereview.chromium.org/2359003004/diff/140001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/140001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6321: Allow Android apps load automatically after startup and reboot. Enabled by default. On 2016/10/03 19:49:00, Luis Héctor Chávez wrote: > Allow Android apps to load automatically after startup and reboot. Enabled by > default. Done.
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 xzhou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2359003004/#ps160001 (title: "Revise flag string.")
The CQ bit was unchecked by xzhou@chromium.org
The CQ bit was checked by xzhou@chromium.org
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...)
xzhou@chromium.org changed reviewers: + derat@chromium.org
On 2016/10/03 22:03:22, commit-bot: I haz the power wrote: > 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...) derat@ Could you please review the following code: chrome/browser/chromeos/settings/device_settings_test_helper.cc chrome/browser/chromeos/settings/device_settings_test_helper.h chromeos/dbus/fake_session_manager_client.cc chromeos/dbus/fake_session_manager_client.h chromeos/dbus/mock_session_manager_client.h chromeos/dbus/session_manager_client.cc chromeos/dbus/session_manager_client.h Thanks.
https://codereview.chromium.org/2359003004/diff/160001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/160001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6317: <message name="IDS_FLAGS_ARC_ACTION_BOOT_COMPLETED" desc="Name of the flag for blocking the ACTION_BOOT_COMPLETED broadcast for third-party apps on ARC."> please be consistent in naming. it looks like you're calling it "arc boot completed broadcast" elsewhere, so it'd be good to rename this to IDS_FLAGS_ARC_BOOT_COMPLETED_BROADCAST to match (ditto for description) https://codereview.chromium.org/2359003004/diff/160001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6321: Allow Android apps to load automatically after startup and reboot. Enabled by default. nit: it doesn't look like we describe the default for other flags. if that's correct, please remove the second sentence here. https://codereview.chromium.org/2359003004/diff/160001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2359003004/diff/160001/chrome/browser/about_f... chrome/browser/about_flags.cc:36: #include "components/arc/arc_features.h" components/arc/ is chrome-os-only, so please put this header within the OS_CHROMEOS ifdef below https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:300: // Read the arc-boot-completed-broadcast flag nit: remove this unnecessary comment https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... File components/arc/arc_features.cc (right): https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... components/arc/arc_features.cc:10: // Controls ACTION_BOOT_COMPLETD broadcast for third party applications on ARC. nit: s/COMPLETD/COMPLETED/ https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... File components/arc/arc_features.h (right): https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... components/arc/arc_features.h:7: #ifndef CHROMEOS_ARC_FEATURES_H_ this is the wrong header guard for this file's path. i thought we had a presubmit about this. CHROMEOS_COMPONENTS_ARC_ARC_FEATURES_H_ https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... components/arc/arc_features.h:14: #if defined(OS_CHROMEOS) this ifdef doesn't make sense. this file shouldn't be built for non-chrome-os platforms, and there are no similar ifdefs in components/arc/. https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... components/arc/arc_features.h:15: extern const base::Feature kArcBootCompletedBroadcast; please rename this to kBootCompletedBroadcastFeature -- the "Arc" at the beginning seems redundant since it's already in the "arc" namespace, and it should have "Feature" at the end to make its type more obvious when it's referenced from some other file.
The CQ bit was checked by xzhou@chromium.org to run a CQ dry run
Fix derat@'s comments. https://codereview.chromium.org/2359003004/diff/160001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/160001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6317: <message name="IDS_FLAGS_ARC_ACTION_BOOT_COMPLETED" desc="Name of the flag for blocking the ACTION_BOOT_COMPLETED broadcast for third-party apps on ARC."> On 2016/10/03 22:39:28, Daniel Erat wrote: > please be consistent in naming. it looks like you're calling it "arc boot > completed broadcast" elsewhere, so it'd be good to rename this to > IDS_FLAGS_ARC_BOOT_COMPLETED_BROADCAST to match (ditto for description) I changed the flag name. Regarding the "ACTION_BOOT_COMPLETED" in the description, it is a broadcast name in Android (https://developer.android.com/reference/android/content/Intent.html#ACTION_BO...). Should I keep it for easy reference? Thanks. https://codereview.chromium.org/2359003004/diff/160001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6321: Allow Android apps to load automatically after startup and reboot. Enabled by default. On 2016/10/03 22:39:28, Daniel Erat wrote: > nit: it doesn't look like we describe the default for other flags. if that's > correct, please remove the second sentence here. Done. https://codereview.chromium.org/2359003004/diff/160001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2359003004/diff/160001/chrome/browser/about_f... chrome/browser/about_flags.cc:36: #include "components/arc/arc_features.h" On 2016/10/03 22:39:28, Daniel Erat wrote: > components/arc/ is chrome-os-only, so please put this header within the > OS_CHROMEOS ifdef below Done. https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:300: // Read the arc-boot-completed-broadcast flag On 2016/10/03 22:39:28, Daniel Erat wrote: > nit: remove this unnecessary comment Done. https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... File components/arc/arc_features.cc (right): https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... components/arc/arc_features.cc:10: // Controls ACTION_BOOT_COMPLETD broadcast for third party applications on ARC. On 2016/10/03 22:39:28, Daniel Erat wrote: > nit: s/COMPLETD/COMPLETED/ Done. https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... File components/arc/arc_features.h (right): https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... components/arc/arc_features.h:7: #ifndef CHROMEOS_ARC_FEATURES_H_ On 2016/10/03 22:39:28, Daniel Erat wrote: > this is the wrong header guard for this file's path. i thought we had a > presubmit about this. > > CHROMEOS_COMPONENTS_ARC_ARC_FEATURES_H_ Done. https://codereview.chromium.org/2359003004/diff/160001/components/arc/arc_fea... components/arc/arc_features.h:15: extern const base::Feature kArcBootCompletedBroadcast; On 2016/10/03 22:39:28, Daniel Erat wrote: > please rename this to kBootCompletedBroadcastFeature -- the "Arc" at the > beginning seems redundant since it's already in the "arc" namespace, and it > should have "Feature" at the end to make its type more obvious when it's > referenced from some other file. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks, looks okay to me with a few more nits https://codereview.chromium.org/2359003004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/device_settings_test_helper.cc (right): https://codereview.chromium.org/2359003004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/settings/device_settings_test_helper.cc:190: bool disable_arc_boot_completed_broadcast, nit: remove "arc" here and in other similar parameters, i.e. just call it "disable_boot_completed_broadcast". the method is StartArcInstance, so it's already clear that this is referring to arc. https://codereview.chromium.org/2359003004/diff/180001/chromeos/dbus/fake_ses... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/2359003004/diff/180001/chromeos/dbus/fake_ses... chromeos/dbus/fake_session_manager_client.cc:166: bool disable_arc_boot_completed_broadcast, rename this too (and note that this doesn't match the header right now) https://codereview.chromium.org/2359003004/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2359003004/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:300: bool disable_arc_boot_completed_broadcast = nit: remove "arc" here too
derat@, thanks a lot for your quick review. Renamed "disable_arc_boot_completed_broadcast" to "disable_boot_completed_broadcast" for simplicity. https://codereview.chromium.org/2359003004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/device_settings_test_helper.cc (right): https://codereview.chromium.org/2359003004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/settings/device_settings_test_helper.cc:190: bool disable_arc_boot_completed_broadcast, On 2016/10/03 23:50:25, Daniel Erat wrote: > nit: remove "arc" here and in other similar parameters, i.e. just call it > "disable_boot_completed_broadcast". the method is StartArcInstance, so it's > already clear that this is referring to arc. Done. https://codereview.chromium.org/2359003004/diff/180001/chromeos/dbus/fake_ses... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/2359003004/diff/180001/chromeos/dbus/fake_ses... chromeos/dbus/fake_session_manager_client.cc:166: bool disable_arc_boot_completed_broadcast, On 2016/10/03 23:50:25, Daniel Erat wrote: > rename this too (and note that this doesn't match the header right now) Done. https://codereview.chromium.org/2359003004/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2359003004/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:300: bool disable_arc_boot_completed_broadcast = On 2016/10/03 23:50:25, Daniel Erat wrote: > nit: remove "arc" here too Done.
The CQ bit was checked by xzhou@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! lgtm with one more nit; feel free to submit after addressing. https://codereview.chromium.org/2359003004/diff/220001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/220001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6321: Allow Android apps to load automatically after startup and reboot. one more nit here: "after startup and reboot" seems a bit odd; should this really be "after signing in" instead? arc apps aren't going to load until the user signs in, right?
On 2016/10/04 00:14:49, Daniel Erat wrote: > thanks! lgtm with one more nit; feel free to submit after addressing. > > https://codereview.chromium.org/2359003004/diff/220001/chrome/app/chromeos_st... > File chrome/app/chromeos_strings.grdp (right): > > https://codereview.chromium.org/2359003004/diff/220001/chrome/app/chromeos_st... > chrome/app/chromeos_strings.grdp:6321: Allow Android apps to load automatically > after startup and reboot. > one more nit here: "after startup and reboot" seems a bit odd; should this > really be "after signing in" instead? arc apps aren't going to load until the > user signs in, right? Thanks again. I will check with mitsuji@ for this since this is user facing UI and need PM approval. Discussion will be at https://b.corp.google.com/issues/30836420.
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 xzhou@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 xzhou@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...
Refined the details string (https://b.corp.google.com/issues/30836420). https://codereview.chromium.org/2359003004/diff/220001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2359003004/diff/220001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6321: Allow Android apps to load automatically after startup and reboot. On 2016/10/04 00:14:49, Daniel Erat wrote: > one more nit here: "after startup and reboot" seems a bit odd; should this > really be "after signing in" instead? arc apps aren't going to load until the > user signs in, right? Done.
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 xzhou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, lhchavez@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2359003004/#ps260001 (title: "Change load to start.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=650313 TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*" ========== to ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=650313 TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*" ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=650313 TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*" ========== to ========== Disable ACTION_BOOT_COMPLETED event for 3rd party apps. Android apps can use the broadcast to start automatically. This Change disable ACTION_BOOT_COMPLETED broadcast for 3rd party applications. BUG=650313 TEST=unit_tests --gtest_filter="*AboutFlagsHistogramTest*" Committed: https://crrev.com/938368c527b95c74317edc7e12924d13787df57e Cr-Commit-Position: refs/heads/master@{#422707} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/938368c527b95c74317edc7e12924d13787df57e Cr-Commit-Position: refs/heads/master@{#422707} |
