Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695113004:
Split IsArcPlayStoreEnabled() and ArcSessionManager's enabled concepts. (Closed)
|
Created:
3 years, 10 months ago by hidehiko Modified:
3 years, 10 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, khmel Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit IsArcPlayStoreEnabled() and ArcSessionManager's enabled concepts.
This CL refactors ArcSessionManager internally.
This CL introduces |enabled_| and Enable()/Disable() methods to
the ArcSessionManager, by extracting code from OnOptInPreferenceChanged().
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
Review-Url: https://codereview.chromium.org/2695113004
Cr-Commit-Position: refs/heads/master@{#451003}
Committed: https://chromium.googlesource.com/chromium/src/+/f6655f54e44faf766d0a3dd6f67524bc0548e2c9
Patch Set 1 #
Total comments: 32
Patch Set 2 : Address comments. #
Total comments: 6
Patch Set 3 : Address comments. #
Total comments: 4
Patch Set 4 : Address comments #
Messages
Total messages: 31 (18 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: + derat@chromium.org, lhchavez@chromium.org
PTAL. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:764: StartTermsOfServiceNegotiation(); Note: dup call check is no longer needed, because it is guarded by |enabled_| in Enable(). https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/not... File chrome/browser/chromeos/note_taking_helper_unittest.cc (left): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/not... chrome/browser/chromeos/note_taking_helper_unittest.cc:648: EXPECT_EQ(4, observer.num_updates()); Note: I removed OnArcPlayStoreEnabledChanged() invocation from re-enable case, because it should be already called immediately (in this test case, it is counted at L646).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks for cleaning this up! i'm not very familiar with this code, but lgtm with some comment suggestions. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:234: // enabled_. Fix it. nit: |enabled_| to match rest of comment https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:441: // Chrome may be shutdown before completing ARC data removal. nit: s/shutdown/shut down/ ("shut down" is a verb, "shutdown" is a noun) https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:444: VLOG(1) << "ARC previously requested to remove data."; nit: "ARC data removal requested."? https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:449: VLOG(1) << "ARC is already enabled at beginning."; nit: i'd make this just "ARC is already enabled." https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:453: VLOG(1) << "ARC is disabled at beginning. Removing the data."; nit: "ARC is initially disabled. Removing data." https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:705: // Data removal request is in progress. Set flag to re-enable Arc once it is nit: s/Arc/ARC/ https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:173: // Enabels ARC session. This triggers to start ARC instance, or maybe nit: "Enables" https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:174: // to start Terms Of Service negotiation. maybe "This starts the ARC instance (or displays the ToS if they haven't been accepted yet)."? https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:176: // Currently, enabled/disabled is tried to whether Google Play Store is nit: s/tried/tied/ ? https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:180: // Disables ARC session. This triggers to stop ARC instance, or if it is "This stops the ARC instance (or closes the ToS if they're shown)." https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:183: // Currently, enabled/disabled is tried to whether Google Play Store is i'd delete this sentence since you have it earlier https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:240: // For restarting, we would like to workaround it, and this is the method. nit: "This method can be called to bypass that check when restarting."
khmel@chromium.org changed reviewers: + khmel@chromium.org
https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:178: void Enable(); These functions are public, Why do we need to have it public? Is any test case, consumer, unit tests for this?
https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:480: enabled_ = false; This works as dupe of State. Can we probably better introduce new state? Now we have arc.enabled, enabled_, state_ and all are dependent. I think it could be very hard to understand the logic. Also it would be nice to see unit_tests that covers test cases when enabled_ invoked.
https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:480: enabled_ = false; On 2017/02/15 16:00:24, khmel wrote: > This works as dupe of State. Can we probably better introduce new state? Now we > have arc.enabled, enabled_, state_ and all are dependent. I think it could be > very hard to understand the logic. > Also it would be nice to see unit_tests that covers test cases when enabled_ > invoked. it doesn't seem to be _quite_ a dupe in this change. Having said that, hidehiko@, do you think we can incorporate |enabled_| into |state_| so we have halve our number of possible states? If it is definitely not possible, please clearly document the relationship between the three variables somewhere. https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:176: // Currently, enabled/disabled is tied to whether Google Play Store is Maybe add a TODO? https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:178: void Enable(); I find it a bit inconsistent that we have SetArcPlayStoreEnabled(bool) and Enable()/Disable(). Is there any compelling reason why it's not SetArcEnabled(bool)?
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...
Thank you for review! PTAL. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:234: // enabled_. Fix it. On 2017/02/15 14:50:05, Daniel Erat wrote: > nit: |enabled_| to match rest of comment Done. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:441: // Chrome may be shutdown before completing ARC data removal. On 2017/02/15 14:50:05, Daniel Erat wrote: > nit: s/shutdown/shut down/ > > ("shut down" is a verb, "shutdown" is a noun) Done. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:444: VLOG(1) << "ARC previously requested to remove data."; On 2017/02/15 14:50:05, Daniel Erat wrote: > nit: "ARC data removal requested."? Done. Mentioned "previous session" explicitly, because it is key reason here. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:449: VLOG(1) << "ARC is already enabled at beginning."; On 2017/02/15 14:50:06, Daniel Erat wrote: > nit: i'd make this just "ARC is already enabled." Done. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:453: VLOG(1) << "ARC is disabled at beginning. Removing the data."; On 2017/02/15 14:50:05, Daniel Erat wrote: > nit: "ARC is initially disabled. Removing data." Done. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:480: enabled_ = false; On 2017/02/15 19:18:53, Luis Héctor Chávez wrote: > On 2017/02/15 16:00:24, khmel wrote: > > This works as dupe of State. Can we probably better introduce new state? Now > we > > have arc.enabled, enabled_, state_ and all are dependent. I think it could be > > very hard to understand the logic. > > Also it would be nice to see unit_tests that covers test cases when enabled_ > > invoked. > > it doesn't seem to be _quite_ a dupe in this change. Having said that, > hidehiko@, do you think we can incorporate |enabled_| into |state_| so we have > halve our number of possible states? > > If it is definitely not possible, please clearly document the relationship > between the three variables somewhere. |enabled_| can be changed at any time by Enable()/Disable() ((will be) external events), and it represents what client code thinks about its state. |state_| is the state machine, and has transition rules based on its circumstances, including old |state_| value, |enabled_| value, and other conditions (like whether ARC is running/stopped etc.). (Though, current code has some problematic state transition, e.g. STOPPED is immediately set on ARC instance stop triggering, while it takes time, etc.). So, although technically we can merge those two variable into one, but it just makes state-machine complicated (Actually, we need all current State enum * enable/disable combination). Luis, I added comment and TODO, to document, after the fix of state transition. Does it work for you? Yury, I agree that the logic around |enabled_| and |state_| machine code is complicated. Though, due to complicated dependency we now have here, it is not simple task to clean them up. Actually, this is one of the CL series to rework on it. As for "arc.enabled" preference, it will be removed from this class. Please let me work on it in a following CL, to keep the CL size reasonably smaller for each step. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:705: // Data removal request is in progress. Set flag to re-enable Arc once it is On 2017/02/15 14:50:05, Daniel Erat wrote: > nit: s/Arc/ARC/ Done. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:173: // Enabels ARC session. This triggers to start ARC instance, or maybe On 2017/02/15 14:50:06, Daniel Erat wrote: > nit: "Enables" Done. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:174: // to start Terms Of Service negotiation. On 2017/02/15 14:50:06, Daniel Erat wrote: > maybe "This starts the ARC instance (or displays the ToS if they haven't been > accepted yet)."? Updated the comment. Note: ToS negotiation have various ways depending on the situation for ARC. To reflect it, I just avoided to use "display" / "close" word explicitly. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:176: // Currently, enabled/disabled is tried to whether Google Play Store is On 2017/02/15 14:50:06, Daniel Erat wrote: > nit: s/tried/tied/ ? Good catch. Done. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:178: void Enable(); On 2017/02/15 15:54:34, khmel wrote: > These functions are public, Why do we need to have it public? Is any test case, > consumer, unit tests for this? This is preparation to extract preference handler related code from this class. In following CLs, Enable()/Disable() will be called from a new class, so these are public. We can temporarily put them in private now, and can move in the following CLs, but it sounds redundant to me, so I just put them into public: section. As for testing, currently existing ones should cover them. In a following CL to extract preference handler from here, I'll rework on unittests. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:180: // Disables ARC session. This triggers to stop ARC instance, or if it is On 2017/02/15 14:50:06, Daniel Erat wrote: > "This stops the ARC instance (or closes the ToS if they're shown)." Done. Similar to Enable() case, I avoided to limit ToS related stuff to UI. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:183: // Currently, enabled/disabled is tried to whether Google Play Store is On 2017/02/15 14:50:06, Daniel Erat wrote: > i'd delete this sentence since you have it earlier I see. Done. https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:240: // For restarting, we would like to workaround it, and this is the method. On 2017/02/15 14:50:06, Daniel Erat wrote: > nit: "This method can be called to bypass that check when restarting." Thank you for good suggestion! Done. https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:176: // Currently, enabled/disabled is tied to whether Google Play Store is On 2017/02/15 19:18:53, Luis Héctor Chávez wrote: > Maybe add a TODO? Instead, referred TODO at SetArcPlayStoreEnabled(). https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:178: void Enable(); On 2017/02/15 19:18:53, Luis Héctor Chávez wrote: > I find it a bit inconsistent that we have SetArcPlayStoreEnabled(bool) and > Enable()/Disable(). Is there any compelling reason why it's not > SetArcEnabled(bool)? No big reason, but I just followed RequestStart/Stop style used in ArcSessionRunner, because we will have similar structure.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2695113004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:480: enabled_ = false; On 2017/02/15 19:45:11, hidehiko wrote: > On 2017/02/15 19:18:53, Luis Héctor Chávez wrote: > > On 2017/02/15 16:00:24, khmel wrote: > > > This works as dupe of State. Can we probably better introduce new state? Now > > we > > > have arc.enabled, enabled_, state_ and all are dependent. I think it could > be > > > very hard to understand the logic. > > > Also it would be nice to see unit_tests that covers test cases when enabled_ > > > invoked. > > > > it doesn't seem to be _quite_ a dupe in this change. Having said that, > > hidehiko@, do you think we can incorporate |enabled_| into |state_| so we have > > halve our number of possible states? > > > > If it is definitely not possible, please clearly document the relationship > > between the three variables somewhere. > > |enabled_| can be changed at any time by Enable()/Disable() ((will be) external > events), and it represents what client code thinks about its state. OK, then since this is orthogonal to |state_|, it does make sense to have it as a separate bit. Thanks for the explanation. > > |state_| is the state machine, and has transition rules based on its > circumstances, including old |state_| value, |enabled_| value, and other > conditions (like whether ARC is running/stopped etc.). (Though, current code has > some problematic state transition, e.g. STOPPED is immediately set on ARC > instance stop triggering, while it takes time, etc.). > > So, although technically we can merge those two variable into one, but it just > makes state-machine complicated (Actually, we need all current State enum * > enable/disable combination). This is what I suspected. Let's keep |enabled_| separate from |state_|. > > Luis, I added comment and TODO, to document, after the fix of state transition. > Does it work for you? Totally! > > Yury, I agree that the logic around |enabled_| and |state_| machine code is > complicated. Though, due to complicated dependency we now have here, it is not > simple task to clean them up. Actually, this is one of the CL series to rework > on it. > As for "arc.enabled" preference, it will be removed from this class. Please let > me work on it in a following CL, to keep the CL size reasonably smaller for each > step. https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:178: void Enable(); On 2017/02/15 19:45:12, hidehiko wrote: > On 2017/02/15 19:18:53, Luis Héctor Chávez wrote: > > I find it a bit inconsistent that we have SetArcPlayStoreEnabled(bool) and > > Enable()/Disable(). Is there any compelling reason why it's not > > SetArcEnabled(bool)? > > No big reason, but I just followed RequestStart/Stop style used in > ArcSessionRunner, because we will have similar structure. Given that we want to mirror that, should this be RequestEnable()? That way it also reflects the nature that it does not necessarily enable it (because of the ToS negotiation). https://codereview.chromium.org/2695113004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2695113004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:480: enabled_ = false; Ah so this is what makes |enabled_| different from opt-in. A better name for this would definitely help make the concept clearer. https://codereview.chromium.org/2695113004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2695113004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:283: // Whether ArcSessionManager is enabled (starting to run ARC instance) or nit: given that you want to have this represent what the client's intention is, should re rename this to |enable_requested_| or something along those lines? Also, can you update the comment so that it says that by setting this to true it triggers the start of the state machine that ultimately enables ARC? (The main motivation for this nit-pickiness is that we have over-used the term "enabled" in the past and this has brought us to do some amount of change to be a bit more nuanced with the actual meaning. Can we preemptively do that this time?)
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...
Thank you for review! PTAL, Luis. https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2695113004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:178: void Enable(); On 2017/02/15 21:54:43, Luis Héctor Chávez wrote: > On 2017/02/15 19:45:12, hidehiko wrote: > > On 2017/02/15 19:18:53, Luis Héctor Chávez wrote: > > > I find it a bit inconsistent that we have SetArcPlayStoreEnabled(bool) and > > > Enable()/Disable(). Is there any compelling reason why it's not > > > SetArcEnabled(bool)? > > > > No big reason, but I just followed RequestStart/Stop style used in > > ArcSessionRunner, because we will have similar structure. > > Given that we want to mirror that, should this be RequestEnable()? That way it > also reflects the nature that it does not necessarily enable it (because of the > ToS negotiation). Done. https://codereview.chromium.org/2695113004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2695113004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:480: enabled_ = false; On 2017/02/15 21:54:43, Luis Héctor Chávez wrote: > Ah so this is what makes |enabled_| different from opt-in. A better name for > this would definitely help make the concept clearer. Acknowledged. https://codereview.chromium.org/2695113004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2695113004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:283: // Whether ArcSessionManager is enabled (starting to run ARC instance) or On 2017/02/15 21:54:43, Luis Héctor Chávez wrote: > nit: given that you want to have this represent what the client's intention is, > should re rename this to |enable_requested_| or something along those lines? > > Also, can you update the comment so that it says that by setting this to true it > triggers the start of the state machine that ultimately enables ARC? > > (The main motivation for this nit-pickiness is that we have over-used the term > "enabled" in the past and this has brought us to do some amount of change to be > a bit more nuanced with the actual meaning. Can we preemptively do that this > time?) Makes sense. Done.
lgtm, defer to khmel@.
On 2017/02/16 03:28:47, Luis Héctor Chávez wrote: > lgtm, defer to khmel@. Thank you! PTAL, khmel@.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org Link to the patchset: https://codereview.chromium.org/2695113004/#ps60001 (title: "Address comments")
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": 60001, "attempt_start_ts": 1487265281845370,
"parent_rev": "239a309b3ab26db7aaac940b124e03f6461fb8ec", "commit_rev":
"f6655f54e44faf766d0a3dd6f67524bc0548e2c9"}
Message was sent while issue was closed.
Description was changed from ========== Split IsArcPlayStoreEnabled() and ArcSessionManager's enabled concepts. This CL refactors ArcSessionManager internally. This CL introduces |enabled_| and Enable()/Disable() methods to the ArcSessionManager, by extracting code from OnOptInPreferenceChanged(). BUG=657687 BUG=b/31079732 TEST=Ran trybots. ========== to ========== Split IsArcPlayStoreEnabled() and ArcSessionManager's enabled concepts. This CL refactors ArcSessionManager internally. This CL introduces |enabled_| and Enable()/Disable() methods to the ArcSessionManager, by extracting code from OnOptInPreferenceChanged(). BUG=657687 BUG=b/31079732 TEST=Ran trybots. Review-Url: https://codereview.chromium.org/2695113004 Cr-Commit-Position: refs/heads/master@{#451003} Committed: https://chromium.googlesource.com/chromium/src/+/f6655f54e44faf766d0a3dd6f675... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f6655f54e44faf766d0a3dd6f675... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
