Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(200)

Issue 2854293003: Fix losing of scoped feature channel in tests when it's equal to stable (Closed)

Created:
3 years, 7 months ago by emaxx
Modified:
3 years, 7 months ago
Reviewers:
Devlin, lazyboy
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, lazyboy
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix losing of scoped feature channel in tests when it's equal to stable This fixes the issue with the value set by ScopedCurrentChannel potentially being overwritten by the browser process initialization code. This could happen when the ScopedCurrentChannel instance was constructed with the default channel (that is, the STABLE channel) before the browser initialization code happens, as the latter would then assume that the channel wasn't set before and overwrite it. This issue means that it was impossible to set up the STABLE channel in the browser test in the setup code that runs before the browser initialization, unless the building environment was configured to be stable. BUG=none, 626343 TEST=existing tests Review-Url: https://codereview.chromium.org/2854293003 Cr-Commit-Position: refs/heads/master@{#473402} Committed: https://chromium.googlesource.com/chromium/src/+/2ae3a0a699cdb174ed7646ba10597f84bbb496ff

Patch Set 1 #

Total comments: 3

Patch Set 2 : Rebase #

Patch Set 3 : Remember the set channel also under scoped setter #

Total comments: 6

Patch Set 4 : Take account of override count #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -19 lines) Patch
M chrome/browser/extensions/chrome_extensions_browser_client.cc View 1 chunk +1 line, -3 lines 0 comments Download
M extensions/common/features/feature_channel.h View 1 2 3 1 chunk +7 lines, -5 lines 0 comments Download
M extensions/common/features/feature_channel.cc View 1 2 3 1 chunk +23 lines, -11 lines 0 comments Download

Messages

Total messages: 31 (20 generated)
emaxx
lazyboy@: PTAL.
3 years, 7 months ago (2017-05-03 18:35:51 UTC) #6
lazyboy
Thanks. I'll forward this to Devlin. He had spent some fun times chasing test failures ...
3 years, 7 months ago (2017-05-03 19:01:14 UTC) #8
Devlin
Just to ensure I understand: > This issue means that it was impossible to set ...
3 years, 7 months ago (2017-05-03 22:43:26 UTC) #9
emaxx
On 2017/05/03 22:43:26, Devlin wrote: > Just to ensure I understand: > > This issue ...
3 years, 7 months ago (2017-05-07 01:54:03 UTC) #11
emaxx
rdevlin.cronin@ / lazyboy@: Friendly ping.
3 years, 7 months ago (2017-05-15 14:38:16 UTC) #13
Devlin
https://codereview.chromium.org/2854293003/diff/1/extensions/common/features/feature_channel.cc File extensions/common/features/feature_channel.cc (right): https://codereview.chromium.org/2854293003/diff/1/extensions/common/features/feature_channel.cc#newcode24 extensions/common/features/feature_channel.cc:24: if (!g_current_channel_override_count) We'd have this issue already, but I ...
3 years, 7 months ago (2017-05-15 20:34:48 UTC) #14
emaxx
On 2017/05/15 20:34:48, Devlin (catching up) wrote: > I'm not sure if it's worth addressing ...
3 years, 7 months ago (2017-05-19 21:54:18 UTC) #19
Devlin
lgtm, thanks for tolerating the bikeshedding :) https://codereview.chromium.org/2854293003/diff/40001/extensions/common/features/feature_channel.cc File extensions/common/features/feature_channel.cc (right): https://codereview.chromium.org/2854293003/diff/40001/extensions/common/features/feature_channel.cc#newcode14 extensions/common/features/feature_channel.cc:14: version_info::Channel g_current_channel ...
3 years, 7 months ago (2017-05-19 22:14:17 UTC) #20
emaxx
https://codereview.chromium.org/2854293003/diff/40001/extensions/common/features/feature_channel.cc File extensions/common/features/feature_channel.cc (right): https://codereview.chromium.org/2854293003/diff/40001/extensions/common/features/feature_channel.cc#newcode14 extensions/common/features/feature_channel.cc:14: version_info::Channel g_current_channel = version_info::Channel::STABLE; On 2017/05/19 22:14:17, Devlin (catching ...
3 years, 7 months ago (2017-05-20 02:48:54 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2854293003/60001
3 years, 7 months ago (2017-05-20 02:49:26 UTC) #28
commit-bot: I haz the power
3 years, 7 months ago (2017-05-20 02:55:02 UTC) #31
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2ae3a0a699cdb174ed7646ba1059...

Powered by Google App Engine
This is Rietveld 408576698