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

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:
5 months, 3 weeks ago by emaxx
Modified:
5 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 #

Messages

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

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa