Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2456803002:
Remove OnOptInChanged callback. (Closed)
|
Created:
4 years, 1 month ago by hidehiko Modified:
4 years, 1 month ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, extensions-reviews_chromium.org, sadrul, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, kalyank, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove OnOptInChanged callback.
The callback is used in two different purposes.
- Callback in browsertest.
This is unused, so just removed.
- Trigger to update the badge.
The actual trigger timing is opt-in preference change.
Replace it by OnOptInEnabled().
BUG=657687
BUG=b/31079732
TEST=Ran on test device.
Enable ARC. Install conflict app. Verify the badge. Disable
ARC. Verify the badge is disappeared.
Committed: https://crrev.com/954755088fb179bb7feb9427ea084eb82daaaaa8
Cr-Commit-Position: refs/heads/master@{#429911}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Reflect the conclusion of offline discussion. #Patch Set 4 : rebase #
Total comments: 5
Patch Set 5 : Address comments. #
Messages
Total messages: 31 (12 generated)
The CQ bit was checked by hidehiko@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...
hidehiko@chromium.org changed reviewers: + lhchavez@chromium.org, skuhne@chromium.org, xdai@chromium.org
PTAL: - lhchavez@: c/b/c/arc - skuhne@: c/b/ui/ash/launcher OWNER. - xdai@: original CL author. FYI: khmel@.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/27 07:43:17, hidehiko wrote: > PTAL: > - lhchavez@: c/b/c/arc > - skuhne@: c/b/ui/ash/launcher OWNER. > - xdai@: original CL author. > > FYI: khmel@. You CL changes current UX. Could you please confirm with PM that is acceptable behavior? Please take a look this code. https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/gfx_u... Badge is actually applied based on changing state, not preference. Following is broken: 1. Enable arc in settings (badge is not applied yet) 2. Agree terms and press sign in (badge is applied after this step) 2a. Cancel OptIn (badge is not applied) You CL breaks point 1
c/b/ui/ash/launcher lgtm
On 2016/10/27 15:32:21, khmel wrote: > On 2016/10/27 07:43:17, hidehiko wrote: > > PTAL: > > - lhchavez@: c/b/c/arc > > - skuhne@: c/b/ui/ash/launcher OWNER. > > - xdai@: original CL author. > > > > FYI: khmel@. > > You CL changes current UX. Could you please confirm with PM that is acceptable > behavior? > Please take a look this code. > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/gfx_u... > > Badge is actually applied based on changing state, not preference. > Following is broken: > > 1. Enable arc in settings (badge is not applied yet) > 2. Agree terms and press sign in (badge is applied after this step) > 2a. Cancel OptIn (badge is not applied) > > You CL breaks point 1 AFAI tested, 2 is not observed. The badge is applied only when the conflicting app is installed. Before the actual start, ARC app cannot be installed? xdai@, could you verify the behavior?
On 2016/10/27 16:01:56, hidehiko wrote: > On 2016/10/27 15:32:21, khmel wrote: > > On 2016/10/27 07:43:17, hidehiko wrote: > > > PTAL: > > > - lhchavez@: c/b/c/arc > > > - skuhne@: c/b/ui/ash/launcher OWNER. > > > - xdai@: original CL author. > > > > > > FYI: khmel@. > > > > You CL changes current UX. Could you please confirm with PM that is acceptable > > behavior? > > Please take a look this code. > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/gfx_u... > > > > Badge is actually applied based on changing state, not preference. > > Following is broken: > > > > 1. Enable arc in settings (badge is not applied yet) > > 2. Agree terms and press sign in (badge is applied after this step) > > 2a. Cancel OptIn (badge is not applied) > > > > You CL breaks point 1 > > AFAI tested, 2 is not observed. The badge is applied only when the conflicting > app is installed. > Before the actual start, ARC app cannot be installed? > > xdai@, could you verify the behavior? Please also take into the consideration that we have concept of default (pre-installed) app. That means Arc app may exist even before arc.enabled.
On 2016/10/27 16:01:56, hidehiko wrote: > On 2016/10/27 15:32:21, khmel wrote: > > On 2016/10/27 07:43:17, hidehiko wrote: > > > PTAL: > > > - lhchavez@: c/b/c/arc > > > - skuhne@: c/b/ui/ash/launcher OWNER. > > > - xdai@: original CL author. > > > > > > FYI: khmel@. > > > > You CL changes current UX. Could you please confirm with PM that is acceptable > > behavior? > > Please take a look this code. > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/gfx_u... > > > > Badge is actually applied based on changing state, not preference. > > Following is broken: > > > > 1. Enable arc in settings (badge is not applied yet) > > 2. Agree terms and press sign in (badge is applied after this step) > > 2a. Cancel OptIn (badge is not applied) > > > > You CL breaks point 1 > > AFAI tested, 2 is not observed. The badge is applied only when the conflicting > app is installed. > Before the actual start, ARC app cannot be installed? > > xdai@, could you verify the behavior? Please also take into the consideration that we have concept of default (pre-installed) app. That means Arc app may exist even before arc.enabled.
On 2016/10/27 16:06:52, khmel wrote: > On 2016/10/27 16:01:56, hidehiko wrote: > > On 2016/10/27 15:32:21, khmel wrote: > > > On 2016/10/27 07:43:17, hidehiko wrote: > > > > PTAL: > > > > - lhchavez@: c/b/c/arc > > > > - skuhne@: c/b/ui/ash/launcher OWNER. > > > > - xdai@: original CL author. > > > > > > > > FYI: khmel@. > > > > > > You CL changes current UX. Could you please confirm with PM that is > acceptable > > > behavior? > > > Please take a look this code. > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/gfx_u... > > > > > > Badge is actually applied based on changing state, not preference. > > > Following is broken: > > > > > > 1. Enable arc in settings (badge is not applied yet) > > > 2. Agree terms and press sign in (badge is applied after this step) > > > 2a. Cancel OptIn (badge is not applied) > > > > > > You CL breaks point 1 > > > > AFAI tested, 2 is not observed. The badge is applied only when the conflicting > > app is installed. > > Before the actual start, ARC app cannot be installed? > > > > xdai@, could you verify the behavior? > > Please also take into the consideration that we have concept of default > (pre-installed) app. That means Arc app may exist even before arc.enabled. I think for now this CL should be fine since we only badge the Chrome Apps in this table (https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/gfx_u...). Currently the pre-installed apps only include "Playstore" and "Play Games", which won't be in this table anyway?
On 2016/10/28 00:13:23, xdai1 wrote: > On 2016/10/27 16:06:52, khmel wrote: > > On 2016/10/27 16:01:56, hidehiko wrote: > > > On 2016/10/27 15:32:21, khmel wrote: > > > > On 2016/10/27 07:43:17, hidehiko wrote: > > > > > PTAL: > > > > > - lhchavez@: c/b/c/arc > > > > > - skuhne@: c/b/ui/ash/launcher OWNER. > > > > > - xdai@: original CL author. > > > > > > > > > > FYI: khmel@. > > > > > > > > You CL changes current UX. Could you please confirm with PM that is > > acceptable > > > > behavior? > > > > Please take a look this code. > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/gfx_u... > > > > > > > > Badge is actually applied based on changing state, not preference. > > > > Following is broken: > > > > > > > > 1. Enable arc in settings (badge is not applied yet) > > > > 2. Agree terms and press sign in (badge is applied after this step) > > > > 2a. Cancel OptIn (badge is not applied) > > > > > > > > You CL breaks point 1 > > > > > > AFAI tested, 2 is not observed. The badge is applied only when the > conflicting > > > app is installed. > > > Before the actual start, ARC app cannot be installed? > > > > > > xdai@, could you verify the behavior? > > > > Please also take into the consideration that we have concept of default > > (pre-installed) app. That means Arc app may exist even before arc.enabled. > > I think for now this CL should be fine since we only badge the Chrome Apps in > this table > (https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/gfx_u...). > Currently the pre-installed apps only include "Playstore" and "Play Games", > which won't be in this table anyway? Any Android app can be pre-installed, there are different concept here. "PlayStore" and "Play Games" are included into default cheets build (Please note, that "Play Games" is not advertised in app list before Arc opted in at this moment (however this may change)). We support OEM customization where any app can be listed and installed after Arc has started (for such apps we show icons in app launcher before Arc is opted in).
The CQ bit was checked by hidehiko@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...
hidehiko@chromium.org changed reviewers: + xiyuan@chromium.org
So, the conclusion is; we do not need the callback, because the badge should be shown on ARC app's install / uninstall timing. We are not worried about conflict default_app cases. PTAL. +R=xiyuan@ for gfx_utils.cc https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/gfx_utils.cc (left): https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/gfx_utils.cc:197: arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get(); Note: conceptually HasEquivalentInstalledArcApp should include this check, so we do not need this.
gfx_utils.cc LGTM
lgtm
c/b/arc lgtm
khmel@chromium.org changed reviewers: + khmel@chromium.org
https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/gfx_utils.cc (left): https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/gfx_utils.cc:199: if (!arc_auth_service || nit: you might no need #include "chrome/browser/chromeos/arc/arc_auth_service.h" and probably #include "chrome/browser/chromeos/arc/arc_support_host.h"
https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/launcher_extension_app_updater.h (left): https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_extension_app_updater.h:9: #include "chrome/browser/chromeos/arc/arc_auth_service.h" nit: Check if we need this else.
Good catch, Yury. PTAL. https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/gfx_utils.cc (left): https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/gfx_utils.cc:199: if (!arc_auth_service || On 2016/11/03 23:09:17, khmel wrote: > nit: you might no need #include "chrome/browser/chromeos/arc/arc_auth_service.h" > > and probably > > #include "chrome/browser/chromeos/arc/arc_support_host.h" Done. https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/launcher_extension_app_updater.h (left): https://codereview.chromium.org/2456803002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_extension_app_updater.h:9: #include "chrome/browser/chromeos/arc/arc_auth_service.h" On 2016/11/03 23:10:10, khmel wrote: > nit: Check if we need this else. Done.
lgtm
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org, xiyuan@chromium.org, lhchavez@chromium.org, xdai@chromium.org Link to the patchset: https://codereview.chromium.org/2456803002/#ps80001 (title: "Address comments.")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
Remove OnOptInChanged callback.
The callback is used in two different purposes.
- Callback in browsertest.
This is unused, so just removed.
- Trigger to update the badge.
The actual trigger timing is opt-in preference change.
Replace it by OnOptInEnabled().
BUG=657687
BUG=b/31079732
TEST=Ran on test device.
Enable ARC. Install conflict app. Verify the badge. Disable
ARC. Verify the badge is disappeared.
==========
to
==========
Remove OnOptInChanged callback.
The callback is used in two different purposes.
- Callback in browsertest.
This is unused, so just removed.
- Trigger to update the badge.
The actual trigger timing is opt-in preference change.
Replace it by OnOptInEnabled().
BUG=657687
BUG=b/31079732
TEST=Ran on test device.
Enable ARC. Install conflict app. Verify the badge. Disable
ARC. Verify the badge is disappeared.
Committed: https://crrev.com/954755088fb179bb7feb9427ea084eb82daaaaa8
Cr-Commit-Position: refs/heads/master@{#429911}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/954755088fb179bb7feb9427ea084eb82daaaaa8 Cr-Commit-Position: refs/heads/master@{#429911} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
