Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2489853002:
Added function to provide the short board name of the device (Closed)
DescriptionAdded function to provide the short board name of the device
To be used in Chrome OS when separating functionality based on board.
Fixed the bad usage of GetLsbReleaseBoard().
BUG=653814, 663857
Committed: https://crrev.com/1a5a3f57210bc089a74d37ef6cc50f034d281c41
Cr-Commit-Position: refs/heads/master@{#435679}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fixed review comments #Patch Set 3 : Fixed bad usage of function #Patch Set 4 : Fixed the lock_state_controller too #
Total comments: 3
Patch Set 5 : Fixed the review comment #
Total comments: 7
Patch Set 6 : Fixed review comments #Patch Set 7 : Small nit #
Total comments: 3
Patch Set 8 : Fixed review comments #
Total comments: 6
Patch Set 9 : Added comment related parrot devices #Patch Set 10 : Nit #
Messages
Total messages: 48 (14 generated)
Description was changed from ========== Added function to provide the short board name of the device To be used in Chrome OS when separating functionality based on board. BUG=653814 ========== to ========== Added function to provide the short board name of the device To be used in Chrome OS when separating functionality based on board. BUG=653814 ==========
igorcov@chromium.org changed reviewers: + tnagel@chromium.org
I've filed 663857 for this.
Please add a unit test. https://codereview.chromium.org/2489853002/diff/1/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/2489853002/diff/1/base/sys_info.h#newcode109 base/sys_info.h:109: // Convenience function for GetLsbReleaseValue("CHROMEOS_RELEASE_BOARD",...). Please improve the docs of this method as well so that other developers can avoid the pitfall.
Added unit tests and improved the comments. Please take a look again. Thank you,
On 2016/11/16 14:19:10, igorcov wrote: > Added unit tests and improved the comments. Please take a look again. > Thank you, I'd suggest to address the issues raised in bug 663857 in the same CL to justify your changes.
Description was changed from ========== Added function to provide the short board name of the device To be used in Chrome OS when separating functionality based on board. BUG=653814 ========== to ========== Added function to provide the short board name of the device To be used in Chrome OS when separating functionality based on board. Fixed the bad usage of GetLsbReleaseBoard(). BUG=653814,663857 ==========
I've left the LockStateController unchanged, since looking at https://chromium.googlesource.com/chromium/src/+/896955de48930899c6c43f24af45... I assume the developer knew what what to expect from the function. Fixed the DriveMetricsProvider class. Other usages on the function in Chrome source look to be fine.
On 2016/11/17 13:56:30, igorcov wrote: > I assume the developer knew what what to expect from the function. That doesn't match my impression. Why do you think it would work differently for mario than for the other boards?
Hi Nikita, why do you parse mario differently than alex and zgb? That looks like a bug to me: https://chromium.googlesource.com/chromium/src/+/896955de48930899c6c43f24af45... Kind regards, Thiemo
I've changed the LockStateController too, and ran the associated tests for it. Please take a look again.
On 2016/11/17 14:02:18, Thiemo Nagel (slow) wrote: > Hi Nikita, > > why do you parse mario differently than alex and zgb? That looks like a bug to > me: > https://chromium.googlesource.com/chromium/src/+/896955de48930899c6c43f24af45... Intention was to highlight the fact that <x86-mario> is the full board name while "x86-alex" may not be the full board name as there're some modifications of that. I think it is not quite clear here.
On 2016/11/17 14:02:18, Thiemo Nagel (slow) wrote: > Hi Nikita, > > why do you parse mario differently than alex and zgb? That looks like a bug to > me: > https://chromium.googlesource.com/chromium/src/+/896955de48930899c6c43f24af45... Intention was to highlight the fact that <x86-mario> is the full board name while "x86-alex" may not be the full board name as there're some modifications of that. I think it is not quite clear here.
nkostylev@chromium.org changed reviewers: + nkostylev@chromium.org
https://codereview.chromium.org/2489853002/diff/60001/ash/wm/lock_state_contr... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/2489853002/diff/60001/ash/wm/lock_state_contr... ash/wm/lock_state_controller.cc:492: if (board == "x86-mario" || board == "x86-alex" || board == "x86-zgb" || That wouldn't work as there're some modifications of boards that *start with* "x86-alex" / "x86-zgb"
Thanks for clarification Nikita. I've changed the code. Technically this introduces a change from previous version, since the shorten board name is retrieved directly in lower-case. So, a hypothetical board "Daisy" would match too, while in previous version it wouldn't. Please let me know if there's a reason to not have this change.
tnagel@chromium.org changed reviewers: + dbeam@chromium.org
Dan, since you did https://codereview.chromium.org/1057093003, could you maybe comment whether that was intended to cover just parrot, or also parrot_ivb and parrot_freon? https://codereview.chromium.org/2489853002/diff/60001/ash/wm/lock_state_contr... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/2489853002/diff/60001/ash/wm/lock_state_contr... ash/wm/lock_state_controller.cc:492: if (board == "x86-mario" || board == "x86-alex" || board == "x86-zgb" || On 2016/11/22 19:10:11, Nikita (slow) wrote: > That wouldn't work as there're some modifications of boards that *start with* > "x86-alex" / "x86-zgb" Apparently there's x86-alex and x86-alex-he, and likewise for x86-zgb. Note that for daisy, there's daisy, daisy_spring and daisy_skate which are all different devices though, and from reading crbug.com/445226 it doesn't seem to that it was intended to cover all of them. Thus I'd suggest to go for an exact match with daisy. https://codereview.chromium.org/2489853002/diff/80001/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/2489853002/diff/80001/base/sys_info.h#newcode112 base/sys_info.h:112: // locally built system compared to chromebooks that use the official version. Nit: "locally built" --> developer built Nit: "chromebook" --> device (there's also Chromebox, Chromebit, AIO, ...) https://codereview.chromium.org/2489853002/diff/80001/base/sys_info.h#newcode119 base/sys_info.h:119: // Convenience function for GetLsbReleaseBoard() removing "-signed.." if Nit: removing "-signed.." --> removing trailing "-signed-*" (use either star or 3 dots, though I think the star would be more readily understood) https://codereview.chromium.org/2489853002/diff/80001/components/metrics/driv... File components/metrics/drive_metrics_provider_linux.cc (right): https://codereview.chromium.org/2489853002/diff/80001/components/metrics/driv... components/metrics/drive_metrics_provider_linux.cc:35: #if defined(OS_CHROMEOS) There's parrot, parrot_ivb and parrot_freon. Which of them have rotating disks?
https://codereview.chromium.org/2489853002/diff/80001/components/metrics/driv... File components/metrics/drive_metrics_provider_linux.cc (right): https://codereview.chromium.org/2489853002/diff/80001/components/metrics/driv... components/metrics/drive_metrics_provider_linux.cc:35: #if defined(OS_CHROMEOS) On 2016/11/23 20:21:19, Thiemo Nagel (slow) wrote: > There's parrot, parrot_ivb and parrot_freon. Which of them have rotating disks? ¯\_(ツ)_/¯ maybe use StartsWith()?
> > ¯\_(ツ)_/¯ > > maybe use StartsWith()? Sgtm.
On 2016/11/24 10:07:16, Thiemo Nagel (slow) wrote: > > > > ¯\_(ツ)_/¯ > > > > maybe use StartsWith()? > > Sgtm. I guess if we wanted, we could try harder and ask the kernel to take an actual look at the hardware, but I guess there's no compelling reason to do so at this point.
https://codereview.chromium.org/2489853002/diff/60001/ash/wm/lock_state_contr... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/2489853002/diff/60001/ash/wm/lock_state_contr... ash/wm/lock_state_controller.cc:492: if (board == "x86-mario" || board == "x86-alex" || board == "x86-zgb" || On 2016/11/23 20:21:19, Thiemo Nagel (slow) wrote: > On 2016/11/22 19:10:11, Nikita (slow) wrote: > > That wouldn't work as there're some modifications of boards that *start with* > > "x86-alex" / "x86-zgb" > > Apparently there's x86-alex and x86-alex-he, and likewise for x86-zgb. > > Note that for daisy, there's daisy, daisy_spring and daisy_skate which are all > different devices though, and from reading crbug.com/445226 it doesn't seem to > that it was intended to cover all of them. Thus I'd suggest to go for an exact > match with daisy. Done. https://codereview.chromium.org/2489853002/diff/80001/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/2489853002/diff/80001/base/sys_info.h#newcode112 base/sys_info.h:112: // locally built system compared to chromebooks that use the official version. On 2016/11/23 20:21:19, Thiemo Nagel (slow) wrote: > Nit: "locally built" --> developer built > Nit: "chromebook" --> device (there's also Chromebox, Chromebit, AIO, ...) Done. https://codereview.chromium.org/2489853002/diff/80001/base/sys_info.h#newcode119 base/sys_info.h:119: // Convenience function for GetLsbReleaseBoard() removing "-signed.." if On 2016/11/23 20:21:19, Thiemo Nagel (slow) wrote: > Nit: removing "-signed.." --> removing trailing "-signed-*" > (use either star or 3 dots, though I think the star would be more readily > understood) Done. https://codereview.chromium.org/2489853002/diff/80001/components/metrics/driv... File components/metrics/drive_metrics_provider_linux.cc (right): https://codereview.chromium.org/2489853002/diff/80001/components/metrics/driv... components/metrics/drive_metrics_provider_linux.cc:35: #if defined(OS_CHROMEOS) On 2016/11/23 21:18:37, Dan Beam wrote: > On 2016/11/23 20:21:19, Thiemo Nagel (slow) wrote: > > There's parrot, parrot_ivb and parrot_freon. Which of them have rotating > disks? > > ¯\_(ツ)_/¯ > > maybe use StartsWith()? Done.
https://codereview.chromium.org/2489853002/diff/120001/ash/wm/lock_state_cont... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/2489853002/diff/120001/ash/wm/lock_state_cont... ash/wm/lock_state_controller.cc:492: if (board == "x86-mario" || board == "daisy" || Add a comment why mario and daisy are handled differently than alex and zgb! It took 3 people to figure out the correct treatment of the different boards, please do preserve that knowledge. https://codereview.chromium.org/2489853002/diff/120001/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/2489853002/diff/120001/base/sys_info.h#newcod... base/sys_info.h:119: // Convenience function for GetLsbReleaseBoard() removing trailing "-signed*" -signed-* Please be precise!
https://codereview.chromium.org/2489853002/diff/120001/ash/wm/lock_state_cont... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/2489853002/diff/120001/ash/wm/lock_state_cont... ash/wm/lock_state_controller.cc:492: if (board == "x86-mario" || board == "daisy" || On 2016/11/25 15:16:12, Thiemo Nagel (slow) wrote: > Add a comment why mario and daisy are handled differently than alex and zgb! It > took 3 people to figure out the correct treatment of the different boards, > please do preserve that knowledge. Done, thanks everyone for digging this up.
lgtm (but I'm not an owner)
igorcov@chromium.org changed reviewers: + asvitkine@chromium.org, pkotwicz@chromium.org, thakis@chromium.org
thakis@ PTAL base/* asvitkine@ PTAL components/metrics/drive_metrics_provider_linux.cc pkotwicz@ PTAL ash/wm/lock_state_controller.cc
lgtm https://codereview.chromium.org/2489853002/diff/140001/components/metrics/dri... File components/metrics/drive_metrics_provider_linux.cc (right): https://codereview.chromium.org/2489853002/diff/140001/components/metrics/dri... components/metrics/drive_metrics_provider_linux.cc:39: !base::StartsWith(board, "parrot", base::CompareCase::SENSITIVE)) { why do you need the StartsWith() call if you're now calling GetStrippedFoo?
https://codereview.chromium.org/2489853002/diff/140001/components/metrics/dri... File components/metrics/drive_metrics_provider_linux.cc (right): https://codereview.chromium.org/2489853002/diff/140001/components/metrics/dri... components/metrics/drive_metrics_provider_linux.cc:39: !base::StartsWith(board, "parrot", base::CompareCase::SENSITIVE)) { On 2016/11/28 15:52:17, Nico wrote: > why do you need the StartsWith() call if you're now calling GetStrippedFoo? There are more types of "parrot", specifically: parrot, parrot_ivb and parrot_freon. Seems like all of them happen to have rotating disks.
lgtm % comments Assuming the logic makes sense to the ChromeOS folks. https://codereview.chromium.org/2489853002/diff/140001/base/sys_info_chromeos.cc File base/sys_info_chromeos.cc (right): https://codereview.chromium.org/2489853002/diff/140001/base/sys_info_chromeos... base/sys_info_chromeos.cc:206: if (index != std::string::npos) { Nit: No {} https://codereview.chromium.org/2489853002/diff/140001/components/metrics/dri... File components/metrics/drive_metrics_provider_linux.cc (right): https://codereview.chromium.org/2489853002/diff/140001/components/metrics/dri... components/metrics/drive_metrics_provider_linux.cc:39: !base::StartsWith(board, "parrot", base::CompareCase::SENSITIVE)) { On 2016/11/28 15:58:02, igorcov wrote: > On 2016/11/28 15:52:17, Nico wrote: > > why do you need the StartsWith() call if you're now calling GetStrippedFoo? > > There are more types of "parrot", specifically: parrot, parrot_ivb and > parrot_freon. Seems like all of them happen to have rotating disks. This logic in the code should have a comment about it.
https://codereview.chromium.org/2489853002/diff/140001/base/sys_info_chromeos.cc File base/sys_info_chromeos.cc (right): https://codereview.chromium.org/2489853002/diff/140001/base/sys_info_chromeos... base/sys_info_chromeos.cc:206: if (index != std::string::npos) { On 2016/11/28 16:03:28, Alexei Svitkine (slow) wrote: > Nit: No {} Done. https://codereview.chromium.org/2489853002/diff/140001/components/metrics/dri... File components/metrics/drive_metrics_provider_linux.cc (right): https://codereview.chromium.org/2489853002/diff/140001/components/metrics/dri... components/metrics/drive_metrics_provider_linux.cc:39: !base::StartsWith(board, "parrot", base::CompareCase::SENSITIVE)) { On 2016/11/28 16:03:28, Alexei Svitkine (slow) wrote: > On 2016/11/28 15:58:02, igorcov wrote: > > On 2016/11/28 15:52:17, Nico wrote: > > > why do you need the StartsWith() call if you're now calling GetStrippedFoo? > > > > There are more types of "parrot", specifically: parrot, parrot_ivb and > > parrot_freon. Seems like all of them happen to have rotating disks. > > This logic in the code should have a comment about it. Done.
lgtm but general observation: something somewhere seems to be creating this string from various permutations. why are we taking the composite version and blowing it back apart? can't we just use one of the specific components that make up the big, composite board name thing?
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org, thakis@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2489853002/#ps180001 (title: "Nit")
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 commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/11/29 01:01:10, Dan Beam wrote:
> lgtm but general observation: something somewhere seems to be creating this
> string from various permutations. why are we taking the composite version and
> blowing it back apart? can't we just use one of the specific components that
> make up the big, composite board name thing?
That string is read from a file ("/etc/lsb-release"). The data in that file is
written by a python script (I assume at the time of installation) and the board
seems to be taken from the input arguments to the script:
https://cs.chromium.org/chromium/src/third_party/chromite/scripts/cros_set_ls...
and I can't find anything further.
I assume we have that information only when installing the image, and are left
with whatever is written in the file to be used. Could as well write in the
file the short name, but feels like too many changes and more redundant data.
igorcov@chromium.org changed reviewers: + varkha@chromium.org - pkotwicz@chromium.org
varkha@chromium.org PTAL ash/wm/* Thanks
On 2016/12/01 17:04:06, igorcov wrote: > mailto:varkha@chromium.org PTAL ash/wm/* > Thanks ash/wm/ lgtm.
The CQ bit was checked by igorcov@chromium.org
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": 180001, "attempt_start_ts": 1480618014857410,
"parent_rev": "33a0beffb5eab94c34586b800f61aac301264797", "commit_rev":
"3379d7b889a6139f52aef801c5e28337704248c3"}
Message was sent while issue was closed.
Description was changed from ========== Added function to provide the short board name of the device To be used in Chrome OS when separating functionality based on board. Fixed the bad usage of GetLsbReleaseBoard(). BUG=653814,663857 ========== to ========== Added function to provide the short board name of the device To be used in Chrome OS when separating functionality based on board. Fixed the bad usage of GetLsbReleaseBoard(). BUG=653814,663857 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Added function to provide the short board name of the device To be used in Chrome OS when separating functionality based on board. Fixed the bad usage of GetLsbReleaseBoard(). BUG=653814,663857 ========== to ========== Added function to provide the short board name of the device To be used in Chrome OS when separating functionality based on board. Fixed the bad usage of GetLsbReleaseBoard(). BUG=653814,663857 Committed: https://crrev.com/1a5a3f57210bc089a74d37ef6cc50f034d281c41 Cr-Commit-Position: refs/heads/master@{#435679} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/1a5a3f57210bc089a74d37ef6cc50f034d281c41 Cr-Commit-Position: refs/heads/master@{#435679} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
