Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21985002:
Add Finch Checks to the State Machine (Closed)
|
Created:
7 years, 4 months ago by robliao Modified:
7 years, 4 months ago CC:
asargent_no_longer_on_chrome, chromium-reviews, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@SMLog Visibility:
Public. |
DescriptionAdd Finch Checks to the State Machine
Google Now will check the Finch Experiment state for
'Enable*' or '' before running
Additionally, it will also request the background permission unless the Finch Experiment state is 'EnableWithoutBackground'
Added some more logging for the state machine as well.
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217342
Patch Set 1 #
Total comments: 17
Patch Set 2 : CR Feedback #
Total comments: 4
Patch Set 3 : Rebase with Rename and Update to Latest #Patch Set 4 : Rolling In Dependent Fixes #
Total comments: 8
Patch Set 5 : Remove enableExperiment #
Total comments: 4
Patch Set 6 : Quick Fixes #
Total comments: 1
Patch Set 7 : Sync to r217065 #
Total comments: 1
Patch Set 8 : Make Strings Const #
Messages
Total messages: 24 (0 generated)
Initial comments. Thanks! https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:754: function setBackgroundEnable(backgroundEnable, onSuccess) { onSuccess -> callback https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:755: chrome.permissions.contains({permissions: ['background']}, Please instrument new APIs. https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:787: * this extension should be running. Actually, we need a flag that conditionally turns on background mode while we are polling. So, we may have 2 states: polling & background, polling & !background. https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:795: callback) { Are you going to get rid of callback and debugSetStepName in a separate CL? Sorry if I already asked this. https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:850: chrome.metricsPrivate.getFieldTrial('GoogleNow', function(response) { How did you test this? https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:851: // '' means we were enabled via the flags but not the experiment. More precisely: // '' means we were enabled via the flags, but the experiment is not running. Explanation: If experiment is running, turning on the flag will force EnableWithFlag group, otherwise, the value will be ''. ALSO: Please modify C++ code that looks at the experiment code per that email.
https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:754: function setBackgroundEnable(backgroundEnable, onSuccess) { Kept this way to remain consistent with the above. We can change this in another CL. On 2013/08/02 21:54:43, vadimt wrote: > onSuccess -> callback https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:755: chrome.permissions.contains({permissions: ['background']}, On 2013/08/02 21:54:43, vadimt wrote: > Please instrument new APIs. Done. https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:787: * this extension should be running. We will want to update the doc: S & L => poll + background permission; TS = true; no showing toast Do we want to only set this on responding to the toast? On 2013/08/02 21:54:43, vadimt wrote: > Actually, we need a flag that conditionally turns on background mode while we > are polling. So, we may have 2 states: polling & background, polling & > !background. https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:795: callback) { That's the plan. I didn't know if you were doing this or I was. I can set this up. On 2013/08/02 21:54:43, vadimt wrote: > Are you going to get rid of callback and debugSetStepName in a separate CL? > Sorry if I already asked this. https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:850: chrome.metricsPrivate.getFieldTrial('GoogleNow', function(response) { Tested with my personal finch server. On 2013/08/02 21:54:43, vadimt wrote: > How did you test this? https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:851: // '' means we were enabled via the flags but not the experiment. On 2013/08/02 21:54:43, vadimt wrote: > More precisely: > // '' means we were enabled via the flags, but the experiment is not running. > > Explanation: > If experiment is running, turning on the flag will force EnableWithFlag group, > otherwise, the value will be ''. > > ALSO: > Please modify C++ code that looks at the experiment code per that email. Done.
https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:787: * this extension should be running. On 2013/08/03 01:21:18, Robert Liao wrote: > We will want to update the doc: > S & L => poll + background permission; TS = true; no showing toast > > Do we want to only set this on responding to the toast? > On 2013/08/02 21:54:43, vadimt wrote: > > Actually, we need a flag that conditionally turns on background mode while we > > are polling. So, we may have 2 states: polling & background, polling & > > !background. > The ultimate goal is to implement as described in the doc. However, we want to first run the experiment to make sure that background mode is safe. Then, presumably, we'll switch all users to background mode. So, I see this conditional background mode as a temporary technical detail that doesn't need to be documented. However, I've added this as a note in the doc. https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:795: callback) { On 2013/08/03 01:21:18, Robert Liao wrote: > That's the plan. I didn't know if you were doing this or I was. I can set this > up. > On 2013/08/02 21:54:43, vadimt wrote: > > Are you going to get rid of callback and debugSetStepName in a separate CL? > > Sorry if I already asked this. > Please do this only for your task. I'll take care of others. https://codereview.chromium.org/21985002/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/component_loader.cc (right): https://codereview.chromium.org/21985002/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/component_loader.cc:474: (fieldTrialResult == "EnabledViaFlag") || There will be a group 'EnableWithBackground'. I'd check that fieldTrialResult starts with 'Enable'. https://codereview.chromium.org/21985002/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:136: tasks.instrumentApiFunction(chrome.tabs, 'create', 1); Please sync, build and run Chrome. Run you functionality, and you'll have auto-detection of still uninstrumented callbacks (if any). https://codereview.chromium.org/21985002/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:856: var enableExperiment = (response == 'Enable' || You need to check for "EnableWithBackground", "EnabledViaFlag" or ""
https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:787: * this extension should be running. Clarified with Modes without Background: EnableWithoutBackground Modes with Background EnableWithBackground EnabledViaFlag '' On 2013/08/03 02:07:53, vadimt wrote: > On 2013/08/03 01:21:18, Robert Liao wrote: > > We will want to update the doc: > > S & L => poll + background permission; TS = true; no showing toast > > > > Do we want to only set this on responding to the toast? > > On 2013/08/02 21:54:43, vadimt wrote: > > > Actually, we need a flag that conditionally turns on background mode while > we > > > are polling. So, we may have 2 states: polling & background, polling & > > > !background. > > > > The ultimate goal is to implement as described in the doc. > However, we want to first run the experiment to make sure that background mode > is safe. Then, presumably, we'll switch all users to background mode. > So, I see this conditional background mode as a temporary technical detail that > doesn't need to be documented. However, I've added this as a note in the doc. https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:795: callback) { Done. There was no debugSetStepName added in. On 2013/08/03 02:07:53, vadimt wrote: > On 2013/08/03 01:21:18, Robert Liao wrote: > > That's the plan. I didn't know if you were doing this or I was. I can set this > > up. > > On 2013/08/02 21:54:43, vadimt wrote: > > > Are you going to get rid of callback and debugSetStepName in a separate CL? > > > Sorry if I already asked this. > > > > Please do this only for your task. I'll take care of others. > https://codereview.chromium.org/21985002/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/component_loader.cc (right): https://codereview.chromium.org/21985002/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/component_loader.cc:474: (fieldTrialResult == "EnabledViaFlag") || On 2013/08/03 02:07:53, vadimt wrote: > There will be a group 'EnableWithBackground'. I'd check that fieldTrialResult > starts with 'Enable'. Done.
https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:795: callback) { On 2013/08/09 21:46:44, Robert Liao wrote: > Done. There was no debugSetStepName added in. > On 2013/08/03 02:07:53, vadimt wrote: > > On 2013/08/03 01:21:18, Robert Liao wrote: > > > That's the plan. I didn't know if you were doing this or I was. I can set > this > > > up. > > > On 2013/08/02 21:54:43, vadimt wrote: > > > > Are you going to get rid of callback and debugSetStepName in a separate > CL? > > > > Sorry if I already asked this. > > > > > > > Please do this only for your task. I'll take care of others. > > > OK :), but please plan a separate CL where you'll get rid of debugSetStepName and callback ONLY IN STATE_CHANGED_TASK_NAME functions. https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:807: enableExperiment, You are not using this param, so you can remove it. https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:872: var enableExperiment = (response.substring(0, 6) == 'Enable') || You don't need this. Simply write: var enableBackground = response != 'EnableWithoutBackground'; (Or wait till I land variation params API and use it; your choice)
https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:807: enableExperiment, This param is used to gate the starting of Google Now. On 2013/08/09 22:13:39, vadimt wrote: > You are not using this param, so you can remove it. https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:872: var enableExperiment = (response.substring(0, 6) == 'Enable') || We want to enable the experiment even with EnableWithoutBackground. Your proposal would cause that status to not be in the experiment. On 2013/08/09 22:13:39, vadimt wrote: > You don't need this. Simply write: > var enableBackground = response != 'EnableWithoutBackground'; > > (Or wait till I land variation params API and use it; your choice)
https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:807: enableExperiment, On 2013/08/09 22:16:47, Robert Liao wrote: > This param is used to gate the starting of Google Now. > On 2013/08/09 22:13:39, vadimt wrote: > > You are not using this param, so you can remove it. > Ah, correct. Still, you don't need this. The gating happens in C++ code, where you check that the group prefix is Enabled. You don't need to gate once again here. I hope this also addresses the other note.
https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:807: enableExperiment, If we're comfortable that once we load the extension code we're supposed to be here, then we can just check the background mode. On 2013/08/09 22:21:27, vadimt wrote: > On 2013/08/09 22:16:47, Robert Liao wrote: > > This param is used to gate the starting of Google Now. > > On 2013/08/09 22:13:39, vadimt wrote: > > > You are not using this param, so you can remove it. > > > > Ah, correct. > Still, you don't need this. > The gating happens in C++ code, where you check that the group prefix is > Enabled. You don't need to gate once again here. > I hope this also addresses the other note.
https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:807: enableExperiment, On 2013/08/09 22:24:40, Robert Liao wrote: > If we're comfortable that once we load the extension code we're supposed to be > here, then we can just check the background mode. > On 2013/08/09 22:21:27, vadimt wrote: > > On 2013/08/09 22:16:47, Robert Liao wrote: > > > This param is used to gate the starting of Google Now. > > > On 2013/08/09 22:13:39, vadimt wrote: > > > > You are not using this param, so you can remove it. > > > > > > > Ah, correct. > > Still, you don't need this. > > The gating happens in C++ code, where you check that the group prefix is > > Enabled. You don't need to gate once again here. > > I hope this also addresses the other note. > Finch parameters don't change while Chrome is running. They get updated on startup. There are no plans to change this. Rather, they consider forcefully restarting Chrome if needed.
https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/40001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:807: enableExperiment, On 2013/08/09 22:28:26, vadimt wrote: > On 2013/08/09 22:24:40, Robert Liao wrote: > > If we're comfortable that once we load the extension code we're supposed to be > > here, then we can just check the background mode. > > On 2013/08/09 22:21:27, vadimt wrote: > > > On 2013/08/09 22:16:47, Robert Liao wrote: > > > > This param is used to gate the starting of Google Now. > > > > On 2013/08/09 22:13:39, vadimt wrote: > > > > > You are not using this param, so you can remove it. > > > > > > > > > > Ah, correct. > > > Still, you don't need this. > > > The gating happens in C++ code, where you check that the group prefix is > > > Enabled. You don't need to gate once again here. > > > I hope this also addresses the other note. > > > > Finch parameters don't change while Chrome is running. They get updated on > startup. There are no plans to change this. Rather, they consider forcefully > restarting Chrome if needed. Done.
lgtm
Owners, please provide owner approval for the files underneath your name. Thanks! asargent: chrome/browser/extensions/component_loader.cc xiyuan: chrome/browser/resources/google_now/background.js chrome/browser/resources/google_now/background_unittest.gtestjs chrome/browser/resources/google_now/manifest.json jhawkins: chrome/browser/ui/webui/options/geolocation_options_handler.cc
chrome/browser/resources/* LGTM https://codereview.chromium.org/21985002/diff/51001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/geolocation_options_handler.cc (right): https://codereview.chromium.org/21985002/diff/51001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.cc:25: std::string enablePrefix("Enable"); enablePrefix -> enable_prefix https://codereview.chromium.org/21985002/diff/51001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.cc:26: std::string fieldTrialResult = fieldTrialResult -> field_trial_result
https://codereview.chromium.org/21985002/diff/51001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/geolocation_options_handler.cc (right): https://codereview.chromium.org/21985002/diff/51001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.cc:25: std::string enablePrefix("Enable"); On 2013/08/09 23:31:53, xiyuan wrote: > enablePrefix -> enable_prefix Done. https://codereview.chromium.org/21985002/diff/51001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/geolocation_options_handler.cc:26: std::string fieldTrialResult = On 2013/08/09 23:31:53, xiyuan wrote: > fieldTrialResult -> field_trial_result Done. https://codereview.chromium.org/21985002/diff/58001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21985002/diff/58001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:865: console.log('Experiment Status: ' + response); Intentional Addition for Finch Diagnostics
Remaining owners (asargent and jhawkins): Please provide owner approval for the files underneath your name. Thanks! asargent: chrome/browser/extensions/component_loader.cc jhawkins: chrome/browser/ui/webui/options/geolocation_options_handler.cc
lgtm
miket: Please provide owner approval for this file. Thanks! chrome/browser/extensions/component_loader.cc
OWNER LGTM https://codereview.chromium.org/21985002/diff/66001/chrome/browser/extensions... File chrome/browser/extensions/component_loader.cc (right): https://codereview.chromium.org/21985002/diff/66001/chrome/browser/extensions... chrome/browser/extensions/component_loader.cc:475: base::FieldTrialList::FindFullName("GoogleNow"); Can these be const? It might enable some simpler code generation. Not a big deal either way.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/21985002/81001
Message was sent while issue was closed.
Change committed as 217342 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
