Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2838273003:
AMP Background Tab: field trial testing configuration. (Closed)
DescriptionAMP Background Tab: field trial testing configuration.
This adds the CCTBackgroundTab feature to the AmpBackgroundTab field trial
testing configuration. Launch bug at crbug.com/710720.
BUG=710720
Review-Url: https://codereview.chromium.org/2838273003
Cr-Commit-Position: refs/heads/master@{#482577}
Committed: https://chromium.googlesource.com/chromium/src/+/f45953a2a571cecf395e59f617980e70a50502b6
Patch Set 1 #Patch Set 2 : Change experiment name to match finch config #
Total comments: 3
Patch Set 3 : comments #Messages
Total messages: 27 (10 generated)
mattcary@chromium.org changed reviewers: + lizeb@chromium.org
Benoit: Does this look sane to you?
mattcary@chromium.org changed reviewers: + nikunjb@chromium.org
Nik--- Here is the field trial testing configuration to go along with the finch cl/154288877. Thanks! Matt
On 2017/04/26 13:35:30, mattcary wrote: > Nik--- > > Here is the field trial testing configuration to go along with the finch > cl/154288877. > > Thanks! > > Matt Ping, now that cl/154288877 is submitted.
On 2017/05/04 13:55:15, mattcary wrote: > On 2017/04/26 13:35:30, mattcary wrote: > > Nik--- > > > > Here is the field trial testing configuration to go along with the finch > > cl/154288877. > > > > Thanks! > > > > Matt > > Ping, now that cl/154288877 is submitted. looks fine to me, but I don't know this file :-)
On 2017/05/09 14:28:00, Benoit L wrote: > On 2017/05/04 13:55:15, mattcary wrote: > > On 2017/04/26 13:35:30, mattcary wrote: > > > Nik--- > > > > > > Here is the field trial testing configuration to go along with the finch > > > cl/154288877. > > > > > > Thanks! > > > > > > Matt > > > > Ping, now that cl/154288877 is submitted. > > looks fine to me, but I don't know this file :-) ping, again?
On 2017/05/30 11:24:31, mattcary wrote: > On 2017/05/09 14:28:00, Benoit L wrote: > > On 2017/05/04 13:55:15, mattcary wrote: > > > On 2017/04/26 13:35:30, mattcary wrote: > > > > Nik--- > > > > > > > > Here is the field trial testing configuration to go along with the finch > > > > cl/154288877. > > > > > > > > Thanks! > > > > > > > > Matt > > > > > > Ping, now that cl/154288877 is submitted. > > > > looks fine to me, but I don't know this file :-) > > ping, again? Still pinging. This is going to beta now.
On 2017/06/15 06:39:22, mattcary wrote: > On 2017/05/30 11:24:31, mattcary wrote: > > On 2017/05/09 14:28:00, Benoit L wrote: > > > On 2017/05/04 13:55:15, mattcary wrote: > > > > On 2017/04/26 13:35:30, mattcary wrote: > > > > > Nik--- > > > > > > > > > > Here is the field trial testing configuration to go along with the finch > > > > > cl/154288877. > > > > > > > > > > Thanks! > > > > > > > > > > Matt > > > > > > > > Ping, now that cl/154288877 is submitted. > > > > > > looks fine to me, but I don't know this file :-) > > > > ping, again? > > Still pinging. This is going to beta now. Sorry for the delay. I am not chromium reviewer so can you include isherman@ as reviewer.
Description was changed from ========== AMP Background Tab: field trial testing configuration. This adds the CCTBackgroundTab feature to the AmpBackgroundTab field trial testing configuration. Launch bug at crbug.com/710720. BUG=710720 ========== to ========== AMP Background Tab: field trial testing configuration. This adds the CCTBackgroundTab feature to the AmpBackgroundTab field trial testing configuration. Launch bug at crbug.com/710720. BUG=710720 ==========
mattcary@chromium.org changed reviewers: + isherman@chromium.org - nikunjb@chromium.org
On 2017/06/21 16:35:09, nikunjb1 wrote: > On 2017/06/15 06:39:22, mattcary wrote: > > On 2017/05/30 11:24:31, mattcary wrote: > > > On 2017/05/09 14:28:00, Benoit L wrote: > > > > On 2017/05/04 13:55:15, mattcary wrote: > > > > > On 2017/04/26 13:35:30, mattcary wrote: > > > > > > Nik--- > > > > > > > > > > > > Here is the field trial testing configuration to go along with the > finch > > > > > > cl/154288877. > > > > > > > > > > > > Thanks! > > > > > > > > > > > > Matt > > > > > > > > > > Ping, now that cl/154288877 is submitted. > > > > > > > > looks fine to me, but I don't know this file :-) > > > > > > ping, again? > > > > Still pinging. This is going to beta now. > > Sorry for the delay. I am not chromium reviewer so can you include isherman@ as > reviewer. Gotcha, thanks.
The CQ bit was checked by isherman@chromium.org
lgtm
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 isherman@chromium.org
https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... testing/variations/fieldtrial_testing_config.json:29: "name": "AmpBackgroundTab", Actually, sorry, I'm a bit confused. Reading the server-side config, it looks like this is the default behavior. If so, what's the reason for adding a testing config entry for it?
On 2017/06/22 16:42:31, Ilya Sherman wrote: > https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... > File testing/variations/fieldtrial_testing_config.json (right): > > https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... > testing/variations/fieldtrial_testing_config.json:29: "name": > "AmpBackgroundTab", > Actually, sorry, I'm a bit confused. Reading the server-side config, it looks > like this is the default behavior. If so, what's the reason for adding a > testing config entry for it? No, the default is disabled: https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li...
On 2017/06/23 09:36:33, mattcary wrote: > On 2017/06/22 16:42:31, Ilya Sherman wrote: > > > https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... > > File testing/variations/fieldtrial_testing_config.json (right): > > > > > https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... > > testing/variations/fieldtrial_testing_config.json:29: "name": > > "AmpBackgroundTab", > > Actually, sorry, I'm a bit confused. Reading the server-side config, it looks > > like this is the default behavior. If so, what's the reason for adding a > > testing config entry for it? > > No, the default is disabled: > > https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li... Okay, so the server-side settings for existing_behavior are reversed? If so, LGTM, but please update the server-side configs as well. https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... testing/variations/fieldtrial_testing_config.json:22: "AmpBackgoundTab": [ nit: s/gound/ground
On 2017/06/26 19:19:34, Ilya Sherman wrote: > On 2017/06/23 09:36:33, mattcary wrote: > > On 2017/06/22 16:42:31, Ilya Sherman wrote: > > > > > > https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... > > > File testing/variations/fieldtrial_testing_config.json (right): > > > > > > > > > https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... > > > testing/variations/fieldtrial_testing_config.json:29: "name": > > > "AmpBackgroundTab", > > > Actually, sorry, I'm a bit confused. Reading the server-side config, it > looks > > > like this is the default behavior. If so, what's the reason for adding a > > > testing config entry for it? > > > > No, the default is disabled: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li... > > Okay, so the server-side settings for existing_behavior are reversed? If so, > LGTM, but please update the server-side configs as well. > Oh, thanks, good catch. Will send CL.
https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2838273003/diff/20001/testing/variations/fiel... testing/variations/fieldtrial_testing_config.json:22: "AmpBackgoundTab": [ On 2017/06/26 19:19:33, Ilya Sherman wrote: > nit: s/gound/ground Done.
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2838273003/#ps40001 (title: "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": 40001, "attempt_start_ts": 1498550283770150,
"parent_rev": "7401b2d29873f46d3ee28dd651e2c8c85a0ee197", "commit_rev":
"f45953a2a571cecf395e59f617980e70a50502b6"}
Message was sent while issue was closed.
Description was changed from ========== AMP Background Tab: field trial testing configuration. This adds the CCTBackgroundTab feature to the AmpBackgroundTab field trial testing configuration. Launch bug at crbug.com/710720. BUG=710720 ========== to ========== AMP Background Tab: field trial testing configuration. This adds the CCTBackgroundTab feature to the AmpBackgroundTab field trial testing configuration. Launch bug at crbug.com/710720. BUG=710720 Review-Url: https://codereview.chromium.org/2838273003 Cr-Commit-Position: refs/heads/master@{#482577} Committed: https://chromium.googlesource.com/chromium/src/+/f45953a2a571cecf395e59f61798... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f45953a2a571cecf395e59f61798... |
