Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(11)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks ago by emaxx
Modified:
4 days, 14 hours ago
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 #

Messages

Total messages: 31 (20 generated)
emaxx
lazyboy@: PTAL.
2 weeks, 6 days 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 ...
2 weeks, 6 days ago (2017-05-03 19:01:14 UTC) #8
Devlin (catching up)
Just to ensure I understand: > This issue means that it was impossible to set ...
2 weeks, 6 days 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 ...
2 weeks, 3 days ago (2017-05-07 01:54:03 UTC) #11
emaxx
rdevlin.cronin@ / lazyboy@: Friendly ping.
1 week, 2 days ago (2017-05-15 14:38:16 UTC) #13
Devlin (catching up)
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 ...
1 week, 1 day 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 ...
4 days, 19 hours ago (2017-05-19 21:54:18 UTC) #19
Devlin (catching up)
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 ...
4 days, 19 hours 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 ...
4 days, 15 hours 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
4 days, 15 hours ago (2017-05-20 02:49:26 UTC) #28
commit-bot: I haz the power
4 days, 14 hours 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06