Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1767603003:
[chromecast] Add cast_shell --accept-resource-provider switch (Closed)
|
Created:
4 years, 9 months ago by jyw Modified:
4 years, 5 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[chromecast] Add cast_shell --accept-resource-provider switch and a utility function, GetSwitchValueBoolean.
Also has the cast_shell flag --accept-resource-provider imply
--alsa-check-close-timeout=0 unless the close timeout is explicitly
overridden.
BUG=internal b/26091363
TEST=pepperoni build
cast_base_unittests --gtest_filter=ChromecastSwitchesTest.*
Committed: https://crrev.com/b92b0b2ccec55e07d667315283ba06006bba42db
Cr-Commit-Position: refs/heads/master@{#380024}
Patch Set 1 #Patch Set 2 : bool -> int #
Total comments: 16
Patch Set 3 : unittests and comment-addressing #Patch Set 4 : use kSwitchValue{True,False} and fix DONOTSUBMIT comment #
Total comments: 4
Patch Set 5 : #
Total comments: 4
Patch Set 6 : fix nits #
Total comments: 2
Patch Set 7 : remove ret variable #
Total comments: 3
Messages
Total messages: 36 (13 generated)
Description was changed from ========== [chromecast] Upstream a cast_shell switch and some utility functions. Also has the cast_shell flag --accept-resource-provider imply --alsa-check-close-timeout=0 unless the close timeout is explicitly overridden. BUG=internal b/26091363 TEST=pepperoni build. ========== to ========== [chromecast] Upstream a cast_shell switch and some utility functions. Also has the cast_shell flag --accept-resource-provider imply --alsa-check-close-timeout=0 unless the close timeout is explicitly overridden. BUG=internal b/26091363 TEST=pepperoni build. ==========
jyw@chromium.org changed reviewers: + byungchul@chromium.org, mengyu@chromium.org
jyw@chromium.org changed reviewers: + kmackay@chromium.org
halliwell@chromium.org changed reviewers: + halliwell@chromium.org
Commit message shouldn't be phrased as 'upstream XXX'. This will be the origin of this code in chromium's git repo, so give it a clear comment as if you were writing this code for the first time. e.g. 'adds --accept-resource-provider switch'. I'm not really feeling great value from the utility switch functions. How many places are they used from? In particular, one of them isn't used anywhere else here, which probably leaves them open to someone deleting them if they ever refactor code. At the very least, a couple of tests would be worthwhile. https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:5: #include "base/command_line.h" ordering nit: put the header corresponding to the cc file first, then everything else in a separate group https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:77: .compare(switches::kSwitchValueFalse); seems like this function accepts any old value that's not "false". In particular, 'FALSE', 'False', '0' and various other misleading things would evaluate true. Should we compare to an explicit 'true' value and name it IsSwitchExplicitTrue for symmetry with the other function? https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... File chromecast/base/chromecast_switches.h (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.h:34: // DONOTSUBMIT what kind of switch is this DONOTSUBMIT :) https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.h:47: namespace base { I don't think we should be putting code in chromecast/ into base:: namespace
Description was changed from ========== [chromecast] Upstream a cast_shell switch and some utility functions. Also has the cast_shell flag --accept-resource-provider imply --alsa-check-close-timeout=0 unless the close timeout is explicitly overridden. BUG=internal b/26091363 TEST=pepperoni build. ========== to ========== [chromecast] Add cast_shell --accept-resource-provider switch and some utility functions. Also has the cast_shell flag --accept-resource-provider imply --alsa-check-close-timeout=0 unless the close timeout is explicitly overridden. BUG=internal b/26091363 TEST=pepperoni build. ==========
https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:85: .compare(switches::kSwitchValueFalse) == 0; Also, just remembered: GetSwitchValueASCII returns an empty string if the switch doesn't exist. So it's redundant to call HasSwitch. Which also makes this function have even less value for me as a utility :)
https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:77: .compare(switches::kSwitchValueFalse); On 2016/03/05 01:01:50, halliwell wrote: > seems like this function accepts any old value that's not "false". In > particular, 'FALSE', 'False', '0' and various other misleading things would > evaluate true. > > Should we compare to an explicit 'true' value and name it IsSwitchExplicitTrue > for symmetry with the other function? These functions are used to disable a feature explicitly especially for OEMs when it is enabled by default process.json. For example, when default process.json has --accept-resource-provider=true and OEM wants to disable it, they just give --accept-resource-provider=false instead of creating a separate process.json. We don't encourage OEMs to have separate process.json files because their own process.json may have command line flags obsoleted later, or not enable some new features. GetSwitchValueBoolean() is used to make a flag false by default. So, no flag -> false, flag without value -> true, flag with a value except "false" -> true, flag with a value "false" -> false while IsSwitchExplicitFalse() makes a flag true by default. So, no flag -> true, flag without value -> true, flag with a value except "false" -> true, flag with a value "false" -> false https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:85: .compare(switches::kSwitchValueFalse) == 0; On 2016/03/05 01:10:58, halliwell wrote: > Also, just remembered: GetSwitchValueASCII returns an empty string if the switch > doesn't exist. So it's redundant to call HasSwitch. > > Which also makes this function have even less value for me as a utility :) One benefit of these functions is consistency in command line flag pattern. https://codereview.chromium.org/1767603003/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:263: : kDefaultCheckCloseTimeoutMs; Is this a right indentation? Why isn't it: int default_close_timeout = base::GetSwitchValueBoolean(switches::kAcceptResourceProvider) ? 0 : kDefaultCheckCloseTimeoutMs; ?
https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:77: .compare(switches::kSwitchValueFalse); On 2016/03/07 17:19:40, byungchul wrote: > On 2016/03/05 01:01:50, halliwell wrote: > > seems like this function accepts any old value that's not "false". In > > particular, 'FALSE', 'False', '0' and various other misleading things would > > evaluate true. > > > > Should we compare to an explicit 'true' value and name it IsSwitchExplicitTrue > > for symmetry with the other function? > > These functions are used to disable a feature explicitly especially for OEMs > when it is enabled by default process.json. For example, when default > process.json has --accept-resource-provider=true and OEM wants to disable it, > they just give --accept-resource-provider=false instead of creating a separate > process.json. We don't encourage OEMs to have separate process.json files > because their own process.json may have command line flags obsoleted later, or > not enable some new features. > > GetSwitchValueBoolean() is used to make a flag false by default. So, > no flag -> false, > flag without value -> true, > flag with a value except "false" -> true, > flag with a value "false" -> false > > while IsSwitchExplicitFalse() makes a flag true by default. So, > no flag -> true, > flag without value -> true, > flag with a value except "false" -> true, > flag with a value "false" -> false Thanks for explanation. It definitely sounds good to keep these functions for consistency in argument handling. However, this motivation is not clear from the CL as it stands. I would suggest: 1) Adding some comment similar to yours here. 2) Naming the two functions in a consistent/symmetric way, to reflect the fact that they are the same thing but with different default value.
https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:77: .compare(switches::kSwitchValueFalse); On 2016/03/07 17:19:40, byungchul wrote: > On 2016/03/05 01:01:50, halliwell wrote: > > seems like this function accepts any old value that's not "false". In > > particular, 'FALSE', 'False', '0' and various other misleading things would > > evaluate true. > > > > Should we compare to an explicit 'true' value and name it IsSwitchExplicitTrue > > for symmetry with the other function? > > These functions are used to disable a feature explicitly especially for OEMs > when it is enabled by default process.json. For example, when default > process.json has --accept-resource-provider=true and OEM wants to disable it, > they just give --accept-resource-provider=false instead of creating a separate > process.json. We don't encourage OEMs to have separate process.json files > because their own process.json may have command line flags obsoleted later, or > not enable some new features. > > GetSwitchValueBoolean() is used to make a flag false by default. So, > no flag -> false, > flag without value -> true, > flag with a value except "false" -> true, > flag with a value "false" -> false > > while IsSwitchExplicitFalse() makes a flag true by default. So, > no flag -> true, > flag without value -> true, > flag with a value except "false" -> true, > flag with a value "false" -> false I'd rather just accept 'true' or 'false' (or no value). If a flag is specified with some other random thing, print an error and use the default value (as if no flag was specified at all). https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:77: .compare(switches::kSwitchValueFalse); On 2016/03/07 23:33:55, halliwell wrote: > On 2016/03/07 17:19:40, byungchul wrote: > > On 2016/03/05 01:01:50, halliwell wrote: > > > seems like this function accepts any old value that's not "false". In > > > particular, 'FALSE', 'False', '0' and various other misleading things would > > > evaluate true. > > > > > > Should we compare to an explicit 'true' value and name it > IsSwitchExplicitTrue > > > for symmetry with the other function? > > > > These functions are used to disable a feature explicitly especially for OEMs > > when it is enabled by default process.json. For example, when default > > process.json has --accept-resource-provider=true and OEM wants to disable it, > > they just give --accept-resource-provider=false instead of creating a separate > > process.json. We don't encourage OEMs to have separate process.json files > > because their own process.json may have command line flags obsoleted later, or > > not enable some new features. > > > > GetSwitchValueBoolean() is used to make a flag false by default. So, > > no flag -> false, > > flag without value -> true, > > flag with a value except "false" -> true, > > flag with a value "false" -> false > > > > while IsSwitchExplicitFalse() makes a flag true by default. So, > > no flag -> true, > > flag without value -> true, > > flag with a value except "false" -> true, > > flag with a value "false" -> false > > Thanks for explanation. It definitely sounds good to keep these functions for > consistency in argument handling. > > However, this motivation is not clear from the CL as it stands. I would > suggest: > 1) Adding some comment similar to yours here. > 2) Naming the two functions in a consistent/symmetric way, to reflect the fact > that they are the same thing but with different default value. Or, just have one function, and pass in the default value?
PTAL https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:5: #include "base/command_line.h" On 2016/03/05 01:01:50, halliwell wrote: > ordering nit: put the header corresponding to the cc file first, then everything > else in a separate group Done. https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:77: .compare(switches::kSwitchValueFalse); On 2016/03/08 00:06:11, kmackay wrote: > On 2016/03/07 17:19:40, byungchul wrote: > > On 2016/03/05 01:01:50, halliwell wrote: > > > seems like this function accepts any old value that's not "false". In > > > particular, 'FALSE', 'False', '0' and various other misleading things would > > > evaluate true. > > > > > > Should we compare to an explicit 'true' value and name it > IsSwitchExplicitTrue > > > for symmetry with the other function? > > > > These functions are used to disable a feature explicitly especially for OEMs > > when it is enabled by default process.json. For example, when default > > process.json has --accept-resource-provider=true and OEM wants to disable it, > > they just give --accept-resource-provider=false instead of creating a separate > > process.json. We don't encourage OEMs to have separate process.json files > > because their own process.json may have command line flags obsoleted later, or > > not enable some new features. > > > > GetSwitchValueBoolean() is used to make a flag false by default. So, > > no flag -> false, > > flag without value -> true, > > flag with a value except "false" -> true, > > flag with a value "false" -> false > > > > while IsSwitchExplicitFalse() makes a flag true by default. So, > > no flag -> true, > > flag without value -> true, > > flag with a value except "false" -> true, > > flag with a value "false" -> false > > I'd rather just accept 'true' or 'false' (or no value). If a flag is specified > with some other random thing, print an error and use the default value (as if no > flag was specified at all). Done. https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... File chromecast/base/chromecast_switches.h (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.h:34: // DONOTSUBMIT what kind of switch is this On 2016/03/05 01:01:51, halliwell wrote: > DONOTSUBMIT :) Done. https://codereview.chromium.org/1767603003/diff/20001/chromecast/base/chromec... chromecast/base/chromecast_switches.h:47: namespace base { On 2016/03/05 01:01:51, halliwell wrote: > I don't think we should be putting code in chromecast/ into base:: namespace Done. https://codereview.chromium.org/1767603003/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/1767603003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:263: : kDefaultCheckCloseTimeoutMs; On 2016/03/07 17:19:40, byungchul wrote: > Is this a right indentation? Why isn't it: > > int default_close_timeout = > base::GetSwitchValueBoolean(switches::kAcceptResourceProvider) > ? 0 > : kDefaultCheckCloseTimeoutMs; > > ? git cl format is a no-op.
Description was changed from
==========
[chromecast] Add cast_shell --accept-resource-provider switch and some utility
functions.
Also has the cast_shell flag --accept-resource-provider imply
--alsa-check-close-timeout=0 unless the close timeout is explicitly
overridden.
BUG=internal b/26091363
TEST=pepperoni build.
==========
to
==========
[chromecast] Add cast_shell --accept-resource-provider switch and some utility
functions.
Also has the cast_shell flag --accept-resource-provider imply
--alsa-check-close-timeout=0 unless the close timeout is explicitly
overridden.
BUG=internal b/26091363
TEST=pepperoni build
cast_base_unittests --gtest_filter=ChromecastSwitchesTest.*
==========
Description was changed from
==========
[chromecast] Add cast_shell --accept-resource-provider switch and some utility
functions.
Also has the cast_shell flag --accept-resource-provider imply
--alsa-check-close-timeout=0 unless the close timeout is explicitly
overridden.
BUG=internal b/26091363
TEST=pepperoni build
cast_base_unittests --gtest_filter=ChromecastSwitchesTest.*
==========
to
==========
[chromecast] Add cast_shell --accept-resource-provider switch and a utility
function, GetSwitchValueBoolean.
Also has the cast_shell flag --accept-resource-provider imply
--alsa-check-close-timeout=0 unless the close timeout is explicitly
overridden.
BUG=internal b/26091363
TEST=pepperoni build
cast_base_unittests --gtest_filter=ChromecastSwitchesTest.*
==========
https://codereview.chromium.org/1767603003/diff/60001/chromecast/base/chromec... File chromecast/base/chromecast_switches.h (right): https://codereview.chromium.org/1767603003/diff/60001/chromecast/base/chromec... chromecast/base/chromecast_switches.h:48: It should be 'chromecast' for consistency.
https://codereview.chromium.org/1767603003/diff/60001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/60001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:83: .compare(switches::kSwitchValueTrue) && I'd really prefer to use == or != rather than compare() (!= in this case).
https://codereview.chromium.org/1767603003/diff/60001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/60001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:83: .compare(switches::kSwitchValueTrue) && On 2016/03/08 17:32:48, kmackay wrote: > I'd really prefer to use == or != rather than compare() (!= in this case). Done. https://codereview.chromium.org/1767603003/diff/60001/chromecast/base/chromec... File chromecast/base/chromecast_switches.h (right): https://codereview.chromium.org/1767603003/diff/60001/chromecast/base/chromec... chromecast/base/chromecast_switches.h:48: On 2016/03/08 04:29:42, halliwell wrote: > It should be 'chromecast' for consistency. Done.
lgtm % nits https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:92: .compare(switches::kSwitchValueFalse); nit: use != instead of compare() https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... File chromecast/base/chromecast_switches_unittest.cc (right): https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... chromecast/base/chromecast_switches_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016?
https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... chromecast/base/chromecast_switches.cc:92: .compare(switches::kSwitchValueFalse); On 2016/03/08 22:51:42, kmackay wrote: > nit: use != instead of compare() Done. https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... File chromecast/base/chromecast_switches_unittest.cc (right): https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... chromecast/base/chromecast_switches_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/08 22:51:42, kmackay wrote: > nit: 2016? Done.
On 2016/03/08 23:04:57, jyw wrote: > https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... > File chromecast/base/chromecast_switches.cc (right): > > https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... > chromecast/base/chromecast_switches.cc:92: > .compare(switches::kSwitchValueFalse); > On 2016/03/08 22:51:42, kmackay wrote: > > nit: use != instead of compare() > > Done. > > https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... > File chromecast/base/chromecast_switches_unittest.cc (right): > > https://codereview.chromium.org/1767603003/diff/80001/chromecast/base/chromec... > chromecast/base/chromecast_switches_unittest.cc:1: // Copyright 2015 The > Chromium Authors. All rights reserved. > On 2016/03/08 22:51:42, kmackay wrote: > > nit: 2016? > > Done. lgtm!
lgtm a nit. https://codereview.chromium.org/1767603003/diff/100001/chromecast/base/chrome... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/100001/chromecast/base/chrome... chromecast/base/chromecast_switches.cc:92: switches::kSwitchValueFalse; If you return here, you don't need "ret".
The CQ bit was checked by jyw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmackay@chromium.org, byungchul@chromium.org, halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/1767603003/#ps120001 (title: "remove ret variable")
https://codereview.chromium.org/1767603003/diff/100001/chromecast/base/chrome... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/100001/chromecast/base/chrome... chromecast/base/chromecast_switches.cc:92: switches::kSwitchValueFalse; On 2016/03/09 01:04:44, byungchul wrote: > If you return here, you don't need "ret". Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767603003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767603003/120001
Message was sent while issue was closed.
Description was changed from
==========
[chromecast] Add cast_shell --accept-resource-provider switch and a utility
function, GetSwitchValueBoolean.
Also has the cast_shell flag --accept-resource-provider imply
--alsa-check-close-timeout=0 unless the close timeout is explicitly
overridden.
BUG=internal b/26091363
TEST=pepperoni build
cast_base_unittests --gtest_filter=ChromecastSwitchesTest.*
==========
to
==========
[chromecast] Add cast_shell --accept-resource-provider switch and a utility
function, GetSwitchValueBoolean.
Also has the cast_shell flag --accept-resource-provider imply
--alsa-check-close-timeout=0 unless the close timeout is explicitly
overridden.
BUG=internal b/26091363
TEST=pepperoni build
cast_base_unittests --gtest_filter=ChromecastSwitchesTest.*
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from
==========
[chromecast] Add cast_shell --accept-resource-provider switch and a utility
function, GetSwitchValueBoolean.
Also has the cast_shell flag --accept-resource-provider imply
--alsa-check-close-timeout=0 unless the close timeout is explicitly
overridden.
BUG=internal b/26091363
TEST=pepperoni build
cast_base_unittests --gtest_filter=ChromecastSwitchesTest.*
==========
to
==========
[chromecast] Add cast_shell --accept-resource-provider switch and a utility
function, GetSwitchValueBoolean.
Also has the cast_shell flag --accept-resource-provider imply
--alsa-check-close-timeout=0 unless the close timeout is explicitly
overridden.
BUG=internal b/26091363
TEST=pepperoni build
cast_base_unittests --gtest_filter=ChromecastSwitchesTest.*
Committed: https://crrev.com/b92b0b2ccec55e07d667315283ba06006bba42db
Cr-Commit-Position: refs/heads/master@{#380024}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b92b0b2ccec55e07d667315283ba06006bba42db Cr-Commit-Position: refs/heads/master@{#380024}
Message was sent while issue was closed.
alokp@chromium.org changed reviewers: + alokp@chromium.org
Message was sent while issue was closed.
This broke our TOT builder: error: obj/chromecast/base/base/chromecast_switches.o: multiple definition of 'switches::kAcceptResourceProvider' /obj/chromecast/internal/cast/base/base/cast_switches.o: previous definition here
Message was sent while issue was closed.
On 2016/03/09 17:58:25, alokp wrote: > This broke our TOT builder: > > error: obj/chromecast/base/base/chromecast_switches.o: multiple definition of > 'switches::kAcceptResourceProvider' > > /obj/chromecast/internal/cast/base/base/cast_switches.o: previous definition > here That's just because the internal change hasn't landed yet.
Message was sent while issue was closed.
https://codereview.chromium.org/1767603003/diff/120001/chromecast/base/chrome... File chromecast/base/chromecast_switches.cc (right): https://codereview.chromium.org/1767603003/diff/120001/chromecast/base/chrome... chromecast/base/chromecast_switches.cc:84: command_line->GetSwitchValueASCII(switch_string) != "") { Why didn't you use a local var to cache command_line->GetSwitchValueASCII(switch_string)?
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
Late drive-by. Please file a bug and fix. https://codereview.chromium.org/1767603003/diff/120001/chromecast/base/chrome... File chromecast/base/chromecast_switches_unittest.cc (right): https://codereview.chromium.org/1767603003/diff/120001/chromecast/base/chrome... chromecast/base/chromecast_switches_unittest.cc:13: base::CommandLine::Reset(); You probably want base::test::ScopedCommandLine instead. This modifies a global shared by all tests in a single process. I think this only works because this is part of cast_base_unittests, which is tiny. If you did this in chrome's unit_tests, it would have screwed up the global state badly for other tests running as part of the same process.
Message was sent while issue was closed.
https://codereview.chromium.org/1767603003/diff/120001/chromecast/base/chrome... File chromecast/base/chromecast_switches_unittest.cc (right): https://codereview.chromium.org/1767603003/diff/120001/chromecast/base/chrome... chromecast/base/chromecast_switches_unittest.cc:13: base::CommandLine::Reset(); On 2016/07/14 10:01:23, Lei Zhang (Slow Thursday) wrote: > You probably want base::test::ScopedCommandLine instead. This modifies a global > shared by all tests in a single process. I think this only works because this is > part of cast_base_unittests, which is tiny. If you did this in chrome's > unit_tests, it would have screwed up the global state badly for other tests > running as part of the same process. Thanks, filed crbug.com/628219. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
