Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2041)

Issue 102963007: Make the Keystone tag contain -32bit and -full as needed (Closed)

Created:
6 years, 11 months ago by Mark Mentovai
Modified:
6 years, 11 months ago
Reviewers:
TVL, Nico
CC:
chromium-reviews, grt+watch_chromium.org, TVL
Visibility:
Public.

Description

Expand the Keystone tag to contain the system's CPU's bitness and whether a full installer is desired. Formerly, the tag identified only the channel that Chrome was on. The tag is being enhanced to detect the CPU's bitness (adding "-32bit" for 32-bit-only, non-64-bit-capable CPUs) and whether a full (as opposed to binary diff patch) update is requested (adding "-full"). CPU bitness detection ought to be a feature of Keystone, but Keystone uses the NXGetLocalArchInfo to determine the CPU type, and winds up always reporting "i486". The "-32bit" tag suffix will be present whenever the "hw.cpu64bit_capable" sysctl name is not found or has value 0. This enables proper detection of users who are capable of running 64-bit Chrome on the server side. When a binary diff patch update application fails in dirpatcher, typically the result of modifications made to existing installations, the "-full" tag suffix will be set. On a subsequent update attempt, the server can detect this value and provide the client with a full updater package, which does not depend on the existing installation. The "-full" tag suffix is cleared on successful update installation. BUG=18323, 54047, 225352, 303280, 316916 R=thakis@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242964

Patch Set 1 : #

Total comments: 12

Patch Set 2 : #

Total comments: 1

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+261 lines, -12 lines) Patch
M build/mac/tweak_info_plist.py View 1 3 chunks +28 lines, -0 lines 0 comments Download
M chrome/browser/mac/keystone_glue.mm View 6 chunks +81 lines, -2 lines 0 comments Download
M chrome/installer/mac/keystone_install.sh View 1 2 11 chunks +152 lines, -10 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Mark Mentovai
Tom is the co-OWNER of record here, but he’s been away from the project long ...
6 years, 11 months ago (2014-01-02 23:10:13 UTC) #1
Nico
lgtm I think https://codereview.chromium.org/102963007/diff/50001/build/mac/tweak_info_plist.py File build/mac/tweak_info_plist.py (right): https://codereview.chromium.org/102963007/diff/50001/build/mac/tweak_info_plist.py#newcode210 build/mac/tweak_info_plist.py:210: components = ('32bit', 'full') assert sorted(components) ...
6 years, 11 months ago (2014-01-02 23:56:28 UTC) #2
TVL
https://codereview.chromium.org/102963007/diff/50001/build/mac/tweak_info_plist.py File build/mac/tweak_info_plist.py (right): https://codereview.chromium.org/102963007/diff/50001/build/mac/tweak_info_plist.py#newcode218 build/mac/tweak_info_plist.py:218: if combination & 1 << component_index: do you want ...
6 years, 11 months ago (2014-01-03 00:09:39 UTC) #3
Mark Mentovai
Updated according to feedback. I also added an extra clause to keystone_install.sh to check that ...
6 years, 11 months ago (2014-01-03 16:46:40 UTC) #4
TVL
lgtm https://codereview.chromium.org/102963007/diff/50001/chrome/browser/mac/keystone_glue.mm File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/102963007/diff/50001/chrome/browser/mac/keystone_glue.mm#newcode1031 chrome/browser/mac/keystone_glue.mm:1031: [appPath_ stringByAppendingPathComponent:@".want_full_installer"]; Ah, might be worth a comment ...
6 years, 11 months ago (2014-01-03 19:41:23 UTC) #5
Mark Mentovai
I added the comment to keystone_install.sh. I also improved the permission handling of the .want_full_installer ...
6 years, 11 months ago (2014-01-03 20:05:27 UTC) #6
Mark Mentovai
6 years, 11 months ago (2014-01-03 23:38:40 UTC) #7
Message was sent while issue was closed.
Committed patchset #3 manually as r242964 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698