Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2506523002:
Report Windows build and patch number in the component updater checks. (Closed)
|
Created:
4 years, 1 month ago by Sorin Jianu Modified:
4 years, 1 month ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReport Windows build and patch number in the component updater checks.
BUG=665249
Committed: https://crrev.com/c2f055bd40cc1793821f01a9ee2229fa09cac54b
Cr-Commit-Position: refs/heads/master@{#432350}
Patch Set 1 #
Total comments: 6
Patch Set 2 : up to #11 #
Total comments: 4
Patch Set 3 : static constexpr #
Messages
Total messages: 30 (14 generated)
The CQ bit was checked by sorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sorin@chromium.org changed reviewers: + grt@chromium.org, waffles@chromium.org
PTAL Greg, please take a look for the sanity of the solution.
lgtm (but I know little of Windows)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
i like the goal, but i think there's an even better way; see below. thanks for checking. https://codereview.chromium.org/2506523002/diff/1/components/update_client/ut... File components/update_client/utils_win.cc (right): https://codereview.chromium.org/2506523002/diff/1/components/update_client/ut... components/update_client/utils_win.cc:46: // Try "CurrentBuild", then fallback to "CurrentBuildNumber". Convert the does this differ from the build # obtained by GetVersionEx? https://codereview.chromium.org/2506523002/diff/1/components/update_client/ut... components/update_client/utils_win.cc:55: key.ReadValueDW(kRegValUBR, &patch); i like having this value available. i like it so much that i think it's better to read it in OSInfo::OSInfo and cache it in the global instance so that it's easily available throughout the product. wdyt? https://codereview.chromium.org/2506523002/diff/1/components/update_client/ut... components/update_client/utils_win.cc:57: return base::StringPrintf("%d.%d.%u.%lu", major, minor, build_number, patch); wdyt of putting UBR in the string returned by base::SysInfo::OperatingSystemVersion (which already contains the build number) and using that for all platforms? this will have the value appear in a variety of places where it may be useful, such as feedback reports, bugs filed from within Chrome, etc (see a somewhat old list here https://docs.google.com/a/google.com/spreadsheets/d/1XVa79TbhQKfIgQXXcHvkB7U0...).
Thank you Greg! This is what I think it is possible for now. We want to get this in by the branch cut this Thursday. I can reimplement in terms of OSInfo and use it that way, along with the build number (The current CL reads the build number and the ubr from registry since I thought that the code is more cohesive that way). I don't now what breaks if we format the UBR in OperatingSystemVersion. I suggest we do that in a separate CL, since this change is critical for us. Sounds good? https://codereview.chromium.org/2506523002/diff/1/components/update_client/ut... File components/update_client/utils_win.cc (right): https://codereview.chromium.org/2506523002/diff/1/components/update_client/ut... components/update_client/utils_win.cc:46: // Try "CurrentBuild", then fallback to "CurrentBuildNumber". Convert the On 2016/11/15 10:25:49, grt (UTC plus 1) wrote: > does this differ from the build # obtained by GetVersionEx? The same value is retrieved in both cases. https://codereview.chromium.org/2506523002/diff/1/components/update_client/ut... components/update_client/utils_win.cc:55: key.ReadValueDW(kRegValUBR, &patch); On 2016/11/15 10:25:49, grt (UTC plus 1) wrote: > i like having this value available. i like it so much that i think it's better > to read it in OSInfo::OSInfo and cache it in the global instance so that it's > easily available throughout the product. wdyt? It makes sense. I am not quite sure what UBR represents, it seems it is some kind of patch number, so to avoid confusion I might implemented as "UBR" until someone figures out what it is. https://codereview.chromium.org/2506523002/diff/1/components/update_client/ut... components/update_client/utils_win.cc:57: return base::StringPrintf("%d.%d.%u.%lu", major, minor, build_number, patch); On 2016/11/15 10:25:49, grt (UTC plus 1) wrote: > wdyt of putting UBR in the string returned by > base::SysInfo::OperatingSystemVersion (which already contains the build number) > and using that for all platforms? this will have the value appear in a variety > of places where it may be useful, such as feedback reports, bugs filed from > within Chrome, etc (see a somewhat old list here > https://docs.google.com/a/google.com/spreadsheets/d/1XVa79TbhQKfIgQXXcHvkB7U0...). I thought about it but I am not sure what is going to break.
Thank you Greg! This is what I think it is possible for now. We want to get this in by the branch cut this Thursday. I can reimplement in terms of OSInfo and use it that way, along with the build number (The current CL reads the build number and the ubr from registry since I thought that the code is more cohesive that way). I don't now what breaks if we format the UBR in OperatingSystemVersion. I suggest we do that in a separate CL, since this change is critical for us. Sounds good?
On 2016/11/15 19:06:53, Sorin Jianu wrote: > Thank you Greg! This is what I think it is possible for now. > > We want to get this in by the branch cut this Thursday. I can reimplement in > terms of OSInfo and use it that way, along with the build number (The current CL > reads the build number and the ubr from registry since I thought that the code > is more cohesive that way). Thanks. I think that's an improvement. > I don't now what breaks if we format the UBR in OperatingSystemVersion. I > suggest we do that in a separate CL, since this change is critical for us. I surveyed all callers of OperatingSystemVersion when I added the build # to it, and at the time everyone was totally cool with having an extra number in there. Since you're pressed for time, I agree that making that change after the branch point is sensible. After the branch, please take a look at the sheet and ping me so we can double-check that adding another number to the version won't hurt anyone. I think others will benefit from the UBR(?). > Sounds good? Yup!
Thank you Greg. I will make a change to include the patch number after the branch. PTAL.
The CQ bit was checked by sorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with a tweak and a nit that you're welcome to disregard. thanks! https://codereview.chromium.org/2506523002/diff/20001/base/win/windows_versio... File base/win/windows_version.cc (right): https://codereview.chromium.org/2506523002/diff/20001/base/win/windows_versio... base/win/windows_version.cc:92: constexpr wchar_t kRegKeyWindowsNTCurrentVersion[] = "static constexpr ..." so that the string isn't pushed onto the stack. https://codereview.chromium.org/2506523002/diff/20001/base/win/windows_versio... base/win/windows_version.cc:103: key.ReadValueDW(kRegValUBR, &ubr); nit: i would use L"UBR" directly here since using a local var neither reduces wrapping nor avoids repetition of the value. i would likely keep kRegKeyWindowsNTCurrentVersion as-is since it may avoid linewrapping.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Thank you Greg, all addressed. https://codereview.chromium.org/2506523002/diff/20001/base/win/windows_versio... File base/win/windows_version.cc (right): https://codereview.chromium.org/2506523002/diff/20001/base/win/windows_versio... base/win/windows_version.cc:92: constexpr wchar_t kRegKeyWindowsNTCurrentVersion[] = On 2016/11/15 20:59:09, grt (UTC plus 1) wrote: > "static constexpr ..." so that the string isn't pushed onto the stack. Done. This had a massive impact, which I admit I did not expect. I expected that in both cases, with and without static, the code will use the address of the string literals and not push anything that is not needed on the stack. Effectively, I expected the local variables to be optimized out, especially in the constexpr case. That did not happen in the optimized build I inspected. With static constexpr, the entire function is inlined. With just the constexpr declaration, the function is not optimized, the local variables are not pushed onto the stack but there are some streaming XMM instructions moving strings around, which I don't understand. https://codereview.chromium.org/2506523002/diff/20001/base/win/windows_versio... base/win/windows_version.cc:103: key.ReadValueDW(kRegValUBR, &ubr); On 2016/11/15 20:59:09, grt (UTC plus 1) wrote: > nit: i would use L"UBR" directly here since using a local var neither reduces > wrapping nor avoids repetition of the value. i would likely keep > kRegKeyWindowsNTCurrentVersion as-is since it may avoid linewrapping. Done.
Thank you Greg, all addressed.
The CQ bit was checked by sorin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2506523002/#ps40001 (title: "static constexpr")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Report Windows build and patch number in the component updater checks. BUG=665249 ========== to ========== Report Windows build and patch number in the component updater checks. BUG=665249 Committed: https://crrev.com/c2f055bd40cc1793821f01a9ee2229fa09cac54b Cr-Commit-Position: refs/heads/master@{#432350} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c2f055bd40cc1793821f01a9ee2229fa09cac54b Cr-Commit-Position: refs/heads/master@{#432350}
Message was sent while issue was closed.
lgtm++ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
