Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
423293004:
Move app_installer into chromium. (Closed)
|
Created:
6 years, 4 months ago by jackhou1 Modified:
6 years, 1 month ago Reviewers:
tapted, Matt Giuca, Lei Zhang, cpu_(ooo_6.6-7.5), palmer, open-source-third-party-reviews, security, grt (UTC plus 2) CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionMove app_installer into chromium.
This code was previously in
https://chrome-internal.googlesource.com/chrome-app-installer.git
We're moving it to chromium with the goal of having it
eventually built as part of Chrome releases.
The third_party code is used to extract the tags added
by dl.google.com. Omaha uses this mechanism to tag their
meta-installers.
BUG=341353
Committed: https://crrev.com/511a873fa5264a01780ee06dd4c19314031ff2a2
Cr-Commit-Position: refs/heads/master@{#301492}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Change app_installer.gypi to app_installer.gyp #
Total comments: 39
Patch Set 3 : Address comments #
Total comments: 4
Patch Set 4 : Address comments #Patch Set 5 : #
Total comments: 8
Patch Set 6 : Get omaha code via DEPS. #Patch Set 7 : Address comments #
Total comments: 2
Patch Set 8 : Update README.chromium. #Patch Set 9 : Sync and rebase #Patch Set 10 : Update README.chromium as per http://crbug.com/414165 #Patch Set 11 : Sync and rebase #
Total comments: 14
Patch Set 12 : Minor updates #Patch Set 13 : Address comments #Patch Set 14 : Refer to untrusted data as "tag" throughout. #Patch Set 15 : Update omaha in src/DEPS. #
Total comments: 6
Patch Set 16 : Address comments #Patch Set 17 : Sync and rebase #Patch Set 18 : Move app_installer to chrome/app_installer #Patch Set 19 : Make app_installer.gyp a gypi. #
Total comments: 10
Patch Set 20 : Address comments #
Total comments: 14
Patch Set 21 : Address comments. #Patch Set 22 : git cl format #
Total comments: 2
Messages
Total messages: 59 (10 generated)
tapted, could you take a look at the gyp/third_party stuff here? I'll ask grt to look at the rest as he reviewed it when it was first committed into chrome-internal.
CL description needs more context (i.e. where this was, why it needs to move out, what third_party stuff is being added and why) And, I think it's best to make app_installer.gyp - i.e. a regular gyp, not a gypi. You might then just have to put a reference to it in build/all.gyp so that ninja offers it as a target. Also where is app_installer_main.cc coming from? (is there a corresponding CL that moves it out?) https://codereview.chromium.org/423293004/diff/1/app_installer/app_installer.... File app_installer/app_installer.gypi (right): https://codereview.chromium.org/423293004/diff/1/app_installer/app_installer.... app_installer/app_installer.gypi:13: 'installer_util', these would be ../chrome/chrome.gyp:installer_util https://codereview.chromium.org/423293004/diff/1/app_installer/app_installer.... app_installer/app_installer.gypi:24: '../app_installer/app_installer_main.cc', remove '../app_installer/' after moving to .gyp https://codereview.chromium.org/423293004/diff/1/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/423293004/diff/1/chrome/chrome.gyp#newcode562 chrome/chrome.gyp:562: '../app_installer/app_installer.gypi', I think you only need a gypi here if there's a circular dependency between gyp files.
https://codereview.chromium.org/423293004/diff/1/app_installer/app_installer.... File app_installer/app_installer.gypi (right): https://codereview.chromium.org/423293004/diff/1/app_installer/app_installer.... app_installer/app_installer.gypi:13: 'installer_util', On 2014/07/30 04:12:50, tapted wrote: > these would be ../chrome/chrome.gyp:installer_util Done. https://codereview.chromium.org/423293004/diff/1/app_installer/app_installer.... app_installer/app_installer.gypi:24: '../app_installer/app_installer_main.cc', On 2014/07/30 04:12:50, tapted wrote: > remove '../app_installer/' after moving to .gyp Done. https://codereview.chromium.org/423293004/diff/1/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/423293004/diff/1/chrome/chrome.gyp#newcode562 chrome/chrome.gyp:562: '../app_installer/app_installer.gypi', On 2014/07/30 04:12:50, tapted wrote: > I think you only need a gypi here if there's a circular dependency between gyp > files. Done.
grt, could you please review this? Most of the code is unchanged from the internal repo. GetUntrustedDataFromTag, which is no longer used in Chrome, I've added to app_installer_main.cc. GetAnyChromeSxSPath, I've added back to chrome_launcher_support, but happy to move that to app_installer as well.
Hi Jack. Apologies for deleting your code! Having this in chromium will make it less fragile. I think this is a good step. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... File app_installer/app_installer.gyp (right): https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer.gyp:6: 'targets': [ 'variables': { 'chromium_code': 1, }, https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer.gyp:6: 'targets': [ 'includes': [ '../build/win_precompile.gypi', ], https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer.gyp:9: { put this in a 'conditions': [['OS="win"', {}]] block since it's windows-only. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer.gyp:24: 'app_installer_main.cc', consider putting this in a win/ sub-directory. this will make it easier to add other platforms in the future. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... File app_installer/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. is there any difference between this and the last one i reviewed elsewhere? https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:23: #include "chrome/common/chrome_switches.h" add a dependency in the .gyp file for chrome.gyp:common_constants for this https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:119: LOG(ERROR) << "Tag extractor could not open file: " << file_name; if each log message will contain file and line (i think they will), consider removing the explanatory text since they add to the binary size and don't provide any info over the source code. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:166: VLOG(1) << "Untrusted data string: " << data_string; do these really need to be in the production code, or can they be DVLOG? https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:220: app_installer::ExitCode GetChrome(bool is_canary) { remove app_installer:: here and elsewhere since this is within a "namespace app_installer {}" block https://codereview.chromium.org/423293004/diff/20001/build/all.gyp File build/all.gyp (right): https://codereview.chromium.org/423293004/diff/20001/build/all.gyp#newcode314 build/all.gyp:314: 'dependencies': [ add: '../app_installer/app_installer.gyp:app_installer', here so that your target is built on the bots. https://codereview.chromium.org/423293004/diff/20001/build/all.gyp#newcode1125 build/all.gyp:1125: '../base/base.gyp:base_unittests', add: '../app_installer/app_installer.gyp:app_installer', here so that your target is built on the official builder. https://codereview.chromium.org/423293004/diff/20001/chrome/installer/launche... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/423293004/diff/20001/chrome/installer/launche... chrome/installer/launcher_support/chrome_launcher_support.cc:112: base::FilePath GetChromeSxSPathForInstallationLevel(InstallationLevel level) { please move this out of the header file and into the unnamed namespace in this file since it isn't called from elsewhere. https://codereview.chromium.org/423293004/diff/20001/chrome/installer/launche... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://codereview.chromium.org/423293004/diff/20001/chrome/installer/launche... chrome/installer/launcher_support/chrome_launcher_support.h:25: // Returns the path to an installed SxS chrome.exe at the specified level, if sorry for deleting your code. :-) https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... File third_party/omaha/base/extractor.h (right): https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. have you made any modifications to this file or to extractor.cc? https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. since this is windows-specific, i think the right thing to do is to check it in to deps/third_party and pull it into src/third_party on windows checkouts via DEPS. see the deps_os=win section in src/DEPS to see what i mean. https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. put the files pulled from the omaha repository into third_party/omaha/src as per http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Modify-DEPS. https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. add /third_party/omaha to .gitignore and do the svn:ignore dance in http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Setting-up-.... https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. please have this reviewed as per: http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:2: // have you run "src/tools/licenses.py scan" and "src/tools/checklicenses/checklicenses.py" as per http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Add-a-LICEN...
https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... File app_installer/app_installer.gyp (right): https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer.gyp:6: 'targets': [ On 2014/07/30 15:05:36, grt wrote: > 'includes': [ > '../build/win_precompile.gypi', > ], Done. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer.gyp:6: 'targets': [ On 2014/07/30 15:05:36, grt wrote: > 'variables': { > 'chromium_code': 1, > }, Done. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer.gyp:9: { On 2014/07/30 15:05:36, grt wrote: > put this in a 'conditions': [['OS="win"', {}]] block since it's windows-only. Done. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer.gyp:24: 'app_installer_main.cc', On 2014/07/30 15:05:36, grt wrote: > consider putting this in a win/ sub-directory. this will make it easier to add > other platforms in the future. Done. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... File app_installer/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/07/30 15:05:36, grt wrote: > is there any difference between this and the last one i reviewed elsewhere? IsNotPrintable down to (and including) GetUntrustedDataValueFromTag are new. They were previously in google_update_util. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:23: #include "chrome/common/chrome_switches.h" On 2014/07/30 15:05:36, grt wrote: > add a dependency in the .gyp file for chrome.gyp:common_constants for this Done. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:119: LOG(ERROR) << "Tag extractor could not open file: " << file_name; On 2014/07/30 15:05:36, grt wrote: > if each log message will contain file and line (i think they will), consider > removing the explanatory text since they add to the binary size and don't > provide any info over the source code. Removed most of these since the exit code will indicate the error. Changed VLOG to DVLOG. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:166: VLOG(1) << "Untrusted data string: " << data_string; On 2014/07/30 15:05:37, grt wrote: > do these really need to be in the production code, or can they be DVLOG? Done. https://codereview.chromium.org/423293004/diff/20001/app_installer/app_instal... app_installer/app_installer_main.cc:220: app_installer::ExitCode GetChrome(bool is_canary) { On 2014/07/30 15:05:36, grt wrote: > remove app_installer:: here and elsewhere since this is within a "namespace > app_installer {}" block Done. https://codereview.chromium.org/423293004/diff/20001/build/all.gyp File build/all.gyp (right): https://codereview.chromium.org/423293004/diff/20001/build/all.gyp#newcode314 build/all.gyp:314: 'dependencies': [ On 2014/07/30 15:05:37, grt wrote: > add: > '../app_installer/app_installer.gyp:app_installer', > here so that your target is built on the bots. Done. https://codereview.chromium.org/423293004/diff/20001/build/all.gyp#newcode1125 build/all.gyp:1125: '../base/base.gyp:base_unittests', On 2014/07/30 15:05:37, grt wrote: > add: > '../app_installer/app_installer.gyp:app_installer', > here so that your target is built on the official builder. Done. https://codereview.chromium.org/423293004/diff/20001/chrome/installer/launche... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/423293004/diff/20001/chrome/installer/launche... chrome/installer/launcher_support/chrome_launcher_support.cc:112: base::FilePath GetChromeSxSPathForInstallationLevel(InstallationLevel level) { On 2014/07/30 15:05:37, grt wrote: > please move this out of the header file and into the unnamed namespace in this > file since it isn't called from elsewhere. Done. https://codereview.chromium.org/423293004/diff/20001/chrome/installer/launche... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://codereview.chromium.org/423293004/diff/20001/chrome/installer/launche... chrome/installer/launcher_support/chrome_launcher_support.h:25: // Returns the path to an installed SxS chrome.exe at the specified level, if On 2014/07/30 15:05:37, grt wrote: > sorry for deleting your code. :-) Acceptable collateral damage from a worthwhile cleanup. =) https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... File third_party/omaha/base/extractor.h (right): https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. On 2014/07/30 15:05:37, grt wrote: > put the files pulled from the omaha repository into third_party/omaha/src as per > http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Modify-DEPS. Done. https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. On 2014/07/30 15:05:37, grt wrote: > since this is windows-specific, i think the right thing to do is to check it in > to deps/third_party and pull it into src/third_party on windows checkouts via > DEPS. see the deps_os=win section in src/DEPS to see what i mean. Filed a bug for a repo. http://crbug.com/399167 https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. On 2014/07/30 15:05:37, grt wrote: > please have this reviewed as per: > http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review I'll update this CL when the new repo is available. I guess the review will happen on a new CL that checks the third party code into the new repo. https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. On 2014/07/30 15:05:37, grt wrote: > have you made any modifications to this file or to extractor.cc? I needed to make a slight change in order to put the code into third_party/omaha/src (as opposed to third_party/omaha/). I'm not sure if it's possible to achieve the same using some gyp incantation. Added a local_modifications.patch. https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. On 2014/07/30 15:05:37, grt wrote: > add /third_party/omaha to .gitignore and do the svn:ignore dance in > http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Setting-up-.... Done. svn part committed at r286698. https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:2: // On 2014/07/30 15:05:37, grt wrote: > have you run "src/tools/licenses.py scan" and > "src/tools/checklicenses/checklicenses.py" as per > http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Add-a-LICEN... These pass. BTW checklicenses.py doesn't seem to run on Windows. I ran it on Linux over Patch Set 3.
looks good. to be continued after the third_party bits are ready? https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... File third_party/omaha/base/extractor.h (right): https://codereview.chromium.org/423293004/diff/20001/third_party/omaha/base/e... third_party/omaha/base/extractor.h:1: // Copyright 2005-2009 Google Inc. On 2014/07/31 03:24:56, jackhou1 wrote: > On 2014/07/30 15:05:37, grt wrote: > > please have this reviewed as per: > > http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review > > I'll update this CL when the new repo is available. I guess the review will > happen on a new CL that checks the third party code into the new repo. I've never done that, so I'm not sure if the third-party commit needs to be in its own CL or if it can be done in this CL. https://codereview.chromium.org/423293004/diff/40001/app_installer/win/app_in... File app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/40001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:8: #include <windows.h> generally, windows.h is extra special and is meant to be included before all other MS headers. the presubmit check for header ordering is aware of this specialness. https://codereview.chromium.org/423293004/diff/40001/third_party/omaha/omaha.gyp File third_party/omaha/omaha.gyp (right): https://codereview.chromium.org/423293004/diff/40001/third_party/omaha/omaha.... third_party/omaha/omaha.gyp:11: 'src/base/extractor.cc', to avoid the patch, you could put these in src/omaha/base (yes, it's repetitive and repetitive) and add 'src' to include_dirs.
> looks good. to be continued after the third_party bits are ready? Yup, I'll ping this CL when that's ready. https://codereview.chromium.org/423293004/diff/40001/app_installer/win/app_in... File app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/40001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:8: #include <windows.h> On 2014/07/31 20:48:06, grt (no reviews Aug 9 - 27) wrote: > generally, windows.h is extra special and is meant to be included before all > other MS headers. the presubmit check for header ordering is aware of this > specialness. Done. https://codereview.chromium.org/423293004/diff/40001/third_party/omaha/omaha.gyp File third_party/omaha/omaha.gyp (right): https://codereview.chromium.org/423293004/diff/40001/third_party/omaha/omaha.... third_party/omaha/omaha.gyp:11: 'src/base/extractor.cc', On 2014/07/31 20:48:06, grt (no reviews Aug 9 - 27) wrote: > to avoid the patch, you could put these in src/omaha/base (yes, it's repetitive > and repetitive) and add 'src' to include_dirs. Done.
An update on this CL: I requested a repo for the third_party stuff (http://crbug.com/399167). I initially intended to check in the files from this CL, but after discussing this with szager, he recommended that it's generally better to mirror the external repo (omaha.googlecode.com) as there would be less maintenance burden. I've updated the bug to request this. The infra team is busy at the moment with the svn->git migration so will be unresponsive for a while, but I'll ping them next week. In the mean time, what do you think of checking in the third_party part (Patch Set 5) and replacing it via DEPS once the repo is ready?
Apologies for the delay, Jack. The code still looks good. It sounds like the mirror is up, so can the third-party stuff be fixed up now before landing? https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... File app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:71: DownloadAndEulaHTMLDialog(bool is_canary) { explicit https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:79: // the |Outcome| values and any form of failure maps to REJECTED. please update comment https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:89: virtual void OnBeforeCreation(wchar_t** extra) { OVERRIDE https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:93: virtual void OnBeforeDisplay(void* window) { OVERRIDE
https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... File app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:71: DownloadAndEulaHTMLDialog(bool is_canary) { On 2014/09/05 16:47:30, grt wrote: > explicit Done. https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:79: // the |Outcome| values and any form of failure maps to REJECTED. On 2014/09/05 16:47:30, grt wrote: > please update comment Done. https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:89: virtual void OnBeforeCreation(wchar_t** extra) { On 2014/09/05 16:47:30, grt wrote: > OVERRIDE Done. https://codereview.chromium.org/423293004/diff/80001/app_installer/win/app_in... app_installer/win/app_installer_main.cc:93: virtual void OnBeforeDisplay(void* window) { On 2014/09/05 16:47:30, grt wrote: > OVERRIDE Done.
lgtm with comment fix in README.chromium below. https://codereview.chromium.org/423293004/diff/120001/third_party/omaha/READM... File third_party/omaha/README.chromium (right): https://codereview.chromium.org/423293004/diff/120001/third_party/omaha/READM... third_party/omaha/README.chromium:12: This is a subset of the Omaha project used to extract tags from signed binaries. this is no longer a subset, is it?
https://codereview.chromium.org/423293004/diff/120001/third_party/omaha/READM... File third_party/omaha/README.chromium (right): https://codereview.chromium.org/423293004/diff/120001/third_party/omaha/READM... third_party/omaha/README.chromium:12: This is a subset of the Omaha project used to extract tags from signed binaries. On 2014/09/08 13:50:22, grt wrote: > this is no longer a subset, is it? Done.
jackhou@chromium.org changed reviewers: + ben@chromium.org, cpu@chromium.org
cpu, please review for OWNERS in third_party/ ben, please review for adding app_installer to src/
please follow the instructions here http://www.chromium.org/developers/adding-3rd-party-libraries in particular getting the open source license folks to review the license.
jackhou@chromium.org changed reviewers: + open-source-third-party-reviews@google.com, security@chromium.org
OSTPR and security, could you please review stuff added to third_party?
lgtm in terms of the code in third party.
Lgtm for ospr On Sep 11, 2014 1:37 PM, <cpu@chromium.org> wrote: > lgtm in terms of the code in third party. > > https://codereview.chromium.org/423293004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
jackhou@chromium.org changed reviewers: + palmer@chromium.org
Ben, ping. Also +palmer as I'm not sure if someone on security noticed this.
So it sounds like the new (or, not even new) attack surface is
src/omaha/base/extractor.{h,cc}, right? So, really, security people should
review that. Everything in this CL LGTM.
Can you please file a child bug that depends on 341353, and assign to one of us?
Probably one of the Windows people (wfh; jschuh is out) or one of the weirdos
who likes to audit things just for fun (palmer, maybe tsepez).
On 2014/09/13 00:05:47, Chromium Palmer wrote:
> So it sounds like the new (or, not even new) attack surface is
> src/omaha/base/extractor.{h,cc}, right? So, really, security people should
> review that. Everything in this CL LGTM.
>
> Can you please file a child bug that depends on 341353, and assign to one of
us?
> Probably one of the Windows people (wfh; jschuh is out) or one of the weirdos
> who likes to audit things just for fun (palmer, maybe tsepez).
Done. http://crbug.com/414165
jackhou@chromium.org changed reviewers: + darin@chromium.org
ben, ping. +darin in case ben is busy. Please review for adding src/app_installer.
https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... File app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:129: return c < 32 || c >= 127; Nit: It feels better to stick to the standard ctype.h functions here and in |IsIllegalUntrustedDataKeyChar|, and to reverse the names and logic of |IsNotPrintable| and |IsIllegalUntrustedDataKeyChar|. That way the code reads as "asserting goodness" rather than "checking for badness". So, just: bool IsStringPrintable(const std::string& s) { return std::find_if_not(s.begin(), s.end(), isprint) == s.end(); } and bool IsLegalDataKeyChar(int c) { return isalnum(c) || c == '-' || c == '_' || c == '$'; } bool IsDataKeyValid(const std::string& key) { return std::find_if_not(key.begin(), key.end(), IsLegalDataeyChar) == key.end(); } https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:148: // Parses |data_string| as key-value pairs and overwrites |untrusted_data| with Document the format this function expects the key-value pairs to be in. https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:152: std::map<std::string, std::string>* untrusted_data) { I'm confused about the terminology here. To security engineers, "trusted" means "can violate our security guarantee/expectations"; "trustworthy" means "we have some reason to believe it will not violate our security expectations", and "untrusted" means "it can't hurt us; a check for trustworthiness might be nice to have but is not strictly necessary". The out-param |untrusted_data| should be called something like |parsed_pairs|, and this function should (if necessary) assert a trustworthiness guarantee: If the function returns true, the pairs are trustworthy (in whatever way matters to the caller). https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:154: if (data_string.length() > kUntrustedDataMaxLength || Similarly, just |kMaxDataLength| or |kMaxValidDataLength|. https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:156: LOG(ERROR) << "Invalid value in untrusted data string."; Might be nice to log each failure separately, and to be more specific. if (data_string.length() > kMaxValidDataLength) { LOG(ERROR) << "Data string length exceeds maximum (" << kMaxDataLength << ")"; return false; } if (!IsStringPrintable(data_string)) { LOG(ERROR) << "Data string contains non-printable characters"; return false; } That having been said, below you call validation functions after on the keys and values, so maybe you can save time by just checking there, and skipping the !IsStringPrintable(data_string) call? https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:176: LOG(ERROR) << "Illegal character found in untrusted data."; Should return false here? https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:277: app_id = GetUntrustedDataValueFromTag(tag, switches::kAppId); Since you call |GetUntrustedDataValueFromTag| twice, you are parsing |tag| twice. Would be better to just parse it once, and cache the resulting map.
https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... File app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:129: return c < 32 || c >= 127; On 2014/10/13 23:47:53, Chromium Palmer wrote: > Nit: It feels better to stick to the standard ctype.h functions here and in > |IsIllegalUntrustedDataKeyChar|, and to reverse the names and logic of > |IsNotPrintable| and |IsIllegalUntrustedDataKeyChar|. That way the code reads as > "asserting goodness" rather than "checking for badness". > > So, just: > > bool IsStringPrintable(const std::string& s) { > return std::find_if_not(s.begin(), s.end(), isprint) == s.end(); > } > > and > > bool IsLegalDataKeyChar(int c) { > return isalnum(c) || c == '-' || c == '_' || c == '$'; > } > > bool IsDataKeyValid(const std::string& key) { > return std::find_if_not(key.begin(), key.end(), IsLegalDataeyChar) == > key.end(); > } Oo, nice. Done. https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:148: // Parses |data_string| as key-value pairs and overwrites |untrusted_data| with On 2014/10/13 23:47:53, Chromium Palmer wrote: > Document the format this function expects the key-value pairs to be in. Done. https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:152: std::map<std::string, std::string>* untrusted_data) { On 2014/10/13 23:47:53, Chromium Palmer wrote: > I'm confused about the terminology here. To security engineers, "trusted" means > "can violate our security guarantee/expectations"; "trustworthy" means "we have > some reason to believe it will not violate our security expectations", and > "untrusted" means "it can't hurt us; a check for trustworthiness might be nice > to have but is not strictly necessary". > > The out-param |untrusted_data| should be called something like |parsed_pairs|, > and this function should (if necessary) assert a trustworthiness guarantee: If > the function returns true, the pairs are trustworthy (in whatever way matters to > the caller). This code, along with IsUntrustedDataKeyValid above, was copied from some code that used to be in Chrome. It refers to a string passed in by Omaha called "untrusted_data". Hence the naming throughout the code. It's untrusted because it's passed in as a url parameter to dl.google.com, which adds the tag to the binary. We use this to pass the app-id, which is checked by IsValidAppId. Should I change it to not say "untrusted"? This function just checks that the values are printable strings. Is there anything else that could be checked? https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:154: if (data_string.length() > kUntrustedDataMaxLength || On 2014/10/13 23:47:54, Chromium Palmer wrote: > Similarly, just |kMaxDataLength| or |kMaxValidDataLength|. Done. https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:156: LOG(ERROR) << "Invalid value in untrusted data string."; On 2014/10/13 23:47:54, Chromium Palmer wrote: > Might be nice to log each failure separately, and to be more specific. > > if (data_string.length() > kMaxValidDataLength) { > LOG(ERROR) << "Data string length exceeds maximum (" << kMaxDataLength << > ")"; > return false; > } > if (!IsStringPrintable(data_string)) { > LOG(ERROR) << "Data string contains non-printable characters"; > return false; > } > > That having been said, below you call validation functions after on the keys and > values, so maybe you can save time by just checking there, and skipping the > !IsStringPrintable(data_string) call? Done. https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:176: LOG(ERROR) << "Illegal character found in untrusted data."; On 2014/10/13 23:47:53, Chromium Palmer wrote: > Should return false here? Done. https://codereview.chromium.org/423293004/diff/200001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:277: app_id = GetUntrustedDataValueFromTag(tag, switches::kAppId); On 2014/10/13 23:47:54, Chromium Palmer wrote: > Since you call |GetUntrustedDataValueFromTag| twice, you are parsing |tag| > twice. Would be better to just parse it once, and cache the resulting map. Done.
> This code, along with IsUntrustedDataKeyValid above, was copied from some code > that used to be in Chrome. It refers to a string passed in by Omaha called > "untrusted_data". Hence the naming throughout the code. > > It's untrusted because it's passed in as a url parameter to http://dl.google.com, which > adds the tag to the binary. We use this to pass the app-id, which is checked by > IsValidAppId. Should I change it to not say "untrusted"? Yeah, I had a feeling the terminology might be that way for historical reasons. You might want to give it some more specific name in this code (|app_id_tag|, perhaps?), and document the fact that it comes from Omaha with the historical name "untrusted_data". > This function just checks that the values are printable strings. Is there > anything else that could be checked? Printable ASCII, + your maximum length, is already admirably tight. What all contexts can the string be used in, though? Does it ever get used as XML, HTML, JavaScript, shell commands, JSON, ...? It should be properly escaped before being interpolated into any of those contexts, but any further input validation is always nice if you can swing it.
On 2014/10/14 02:10:32, Chromium Palmer wrote: > > This code, along with IsUntrustedDataKeyValid above, was copied from some code > > that used to be in Chrome. It refers to a string passed in by Omaha called > > "untrusted_data". Hence the naming throughout the code. > > > > It's untrusted because it's passed in as a url parameter to > http://dl.google.com, which > > adds the tag to the binary. We use this to pass the app-id, which is checked > by > > IsValidAppId. Should I change it to not say "untrusted"? > > Yeah, I had a feeling the terminology might be that way for historical reasons. > You might want to give it some more specific name in this code (|app_id_tag|, > perhaps?), and document the fact that it comes from Omaha with the historical > name "untrusted_data". Done. Changed terminology to "tag". Also moved the kMaxTagLength check to immediately after ExtractTag(). > > This function just checks that the values are printable strings. Is there > > anything else that could be checked? > > Printable ASCII, + your maximum length, is already admirably tight. What all > contexts can the string be used in, though? Does it ever get used as XML, HTML, > JavaScript, shell commands, JSON, ...? It should be properly escaped before > being interpolated into any of those contexts, but any further input validation > is always nice if you can swing it. It gets passed to Chrome as a string, but IsValidAppId is quite strict (only contains characters 'a' to 'p').
The third-party code has been security reviewed and committed here: https://chromium-review.googlesource.com/#/c/223041/
On 2014/10/14 03:02:46, jackhou1 wrote: > The third-party code has been security reviewed and committed here: > https://chromium-review.googlesource.com/#/c/223041/ Is https://chromium.googlesource.com/external/omaha/+/098c7a3d157218dab4eed595e8... being committed upstream?
https://codereview.chromium.org/423293004/diff/310001/app_installer/win/app_i... File app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/310001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:127: if (tag_buffer_size > kMaxTagLength) { can't this check be moved up to line 122 so that it happens before the allocation? https://codereview.chromium.org/423293004/diff/310001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:165: for (it = kv_pairs.begin(); it != kv_pairs.end(); ++it) { this can now be simplified to: for (const auto& it : kv_pairs) { } https://codereview.chromium.org/423293004/diff/310001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:267: if (!ParseTag(tag, &parsed_pairs)) { nit: omit braces
https://codereview.chromium.org/423293004/diff/310001/app_installer/win/app_i... File app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/310001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:127: if (tag_buffer_size > kMaxTagLength) { On 2014/10/14 13:04:08, grt wrote: > can't this check be moved up to line 122 so that it happens before the > allocation? Done. https://codereview.chromium.org/423293004/diff/310001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:165: for (it = kv_pairs.begin(); it != kv_pairs.end(); ++it) { On 2014/10/14 13:04:08, grt wrote: > this can now be simplified to: > for (const auto& it : kv_pairs) { > } Done. (Yay C++11!) https://codereview.chromium.org/423293004/diff/310001/app_installer/win/app_i... app_installer/win/app_installer_main.cc:267: if (!ParseTag(tag, &parsed_pairs)) { On 2014/10/14 13:04:08, grt wrote: > nit: omit braces Done.
On 2014/10/14 12:55:33, grt wrote: > On 2014/10/14 03:02:46, jackhou1 wrote: > > The third-party code has been security reviewed and committed here: > > https://chromium-review.googlesource.com/#/c/223041/ > > Is > https://chromium.googlesource.com/external/omaha/+/098c7a3d157218dab4eed595e8... > being committed upstream? No, but I'll talk to Omaha team about it.
lgtm
jackhou@chromium.org changed reviewers: - ben@chromium.org, darin@chromium.org
grt, PTAL Hopefully the last round. I emailed darin@ about adding app_installer and he thinks chrome/app_installer is better. I cc'd you on that thread. Looks like it works fine as a separate .gyp, is there any benefit to making it a .gypi and part of chrome.gyp? (Like chrome_installer.gypi)
On 2014/10/21 06:08:17, jackhou1 wrote: > grt, PTAL > > Hopefully the last round. I emailed darin@ about adding app_installer and he > thinks chrome/app_installer is better. I cc'd you on that thread. > > Looks like it works fine as a separate .gyp, is there any benefit to making it a > .gypi and part of chrome.gyp? (Like chrome_installer.gypi) If it's being considered a part of Chrome (hence living in src/chrome), I think a .gypi is the right choice.
On 2014/10/21 13:46:07, grt wrote: > On 2014/10/21 06:08:17, jackhou1 wrote: > > grt, PTAL > > > > Hopefully the last round. I emailed darin@ about adding app_installer and he > > thinks chrome/app_installer is better. I cc'd you on that thread. > > > > Looks like it works fine as a separate .gyp, is there any benefit to making it > a > > .gypi and part of chrome.gyp? (Like chrome_installer.gypi) > > If it's being considered a part of Chrome (hence living in src/chrome), I think > a .gypi is the right choice. Done. Could you PTAL, particularly with build/all.gyp?
https://codereview.chromium.org/423293004/diff/390001/build/all.gyp File build/all.gyp (right): https://codereview.chromium.org/423293004/diff/390001/build/all.gyp#newcode201 build/all.gyp:201: '../chrome/chrome.gyp:app_installer', not needed due to chrome.gyp:* on line 101. https://codereview.chromium.org/423293004/diff/390001/build/all.gyp#newcode339 build/all.gyp:339: '../chrome/chrome.gyp:app_installer', In the new world of recipes, I don't know for sure that this will build on try runs or on the chromium waterfall builders. If it doesn't, you could easily land a breaking change that will only appear on the official builders. Please ask on infra-dev@chromium.org what the right way is to have this new target get built there. In the old world, it was as simple as adding it to targets like this one in this file, but I'm not sure that's the case now. For the moment, how about commenting out the addition to chrome_official_builder with a TODO saying it'll be de-commented once the target is building on trybots and the waterfall. https://codereview.chromium.org/423293004/diff/390001/chrome/app_installer/ap... File chrome/app_installer/app_installer.gypi (right): https://codereview.chromium.org/423293004/diff/390001/chrome/app_installer/ap... chrome/app_installer/app_installer.gypi:7: 'chromium_code': 1, please remove this (other .gypi files included by chrome.gyp don't have it). https://codereview.chromium.org/423293004/diff/390001/chrome/app_installer/ap... chrome/app_installer/app_installer.gypi:19: 'chrome.gyp:installer_util', remove 'chrome.gyp:' from these dependencies since this .gypi file is now in the context of chrome.gyp, then reorder this list to something along the lines of: 'installer_util', 'installer_util_strings', 'launcher_support', 'common_constants.gyp:common_constants', '../base/base.gyp:base', '../third_party/omaha/omaha.gyp:omaha_extractor', https://codereview.chromium.org/423293004/diff/390001/chrome/app_installer/ap... chrome/app_installer/app_installer.gypi:26: '../../build/win_precompile.gypi', remove this
https://codereview.chromium.org/423293004/diff/390001/build/all.gyp File build/all.gyp (right): https://codereview.chromium.org/423293004/diff/390001/build/all.gyp#newcode201 build/all.gyp:201: '../chrome/chrome.gyp:app_installer', On 2014/10/22 13:33:57, grt (expect delays oct 27-29) wrote: > not needed due to chrome.gyp:* on line 101. Done. https://codereview.chromium.org/423293004/diff/390001/build/all.gyp#newcode339 build/all.gyp:339: '../chrome/chrome.gyp:app_installer', On 2014/10/22 13:33:57, grt (expect delays oct 27-29) wrote: > In the new world of recipes, I don't know for sure that this will build on try > runs or on the chromium waterfall builders. If it doesn't, you could easily land > a breaking change that will only appear on the official builders. Please ask on > mailto:infra-dev@chromium.org what the right way is to have this new target get built > there. In the old world, it was as simple as adding it to targets like this one > in this file, but I'm not sure that's the case now. For the moment, how about > commenting out the addition to chrome_official_builder with a TODO saying it'll > be de-commented once the target is building on trybots and the waterfall. Done. https://codereview.chromium.org/423293004/diff/390001/chrome/app_installer/ap... File chrome/app_installer/app_installer.gypi (right): https://codereview.chromium.org/423293004/diff/390001/chrome/app_installer/ap... chrome/app_installer/app_installer.gypi:7: 'chromium_code': 1, On 2014/10/22 13:33:57, grt (expect delays oct 27-29) wrote: > please remove this (other .gypi files included by chrome.gyp don't have it). Done. https://codereview.chromium.org/423293004/diff/390001/chrome/app_installer/ap... chrome/app_installer/app_installer.gypi:19: 'chrome.gyp:installer_util', On 2014/10/22 13:33:57, grt (expect delays oct 27-29) wrote: > remove 'chrome.gyp:' from these dependencies since this .gypi file is now in the > context of chrome.gyp, then reorder this list to something along the lines of: > 'installer_util', > 'installer_util_strings', > 'launcher_support', > 'common_constants.gyp:common_constants', > '../base/base.gyp:base', > '../third_party/omaha/omaha.gyp:omaha_extractor', Done. https://codereview.chromium.org/423293004/diff/390001/chrome/app_installer/ap... chrome/app_installer/app_installer.gypi:26: '../../build/win_precompile.gypi', On 2014/10/22 13:33:57, grt (expect delays oct 27-29) wrote: > remove this Done.
lgtm
jackhou@chromium.org changed reviewers: + thestig@chromium.org
thestig, please review for OWNERS: - new directory chrome/app_installer/ - +base in chrome/app_installer/DEPS - changes to chrome.gyp
https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/DEPS File chrome/app_installer/DEPS (right): https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/DE... chrome/app_installer/DEPS:2: "+base", You shouldn't need this. src/DEPS already says everyone can use base. Similarly, chrome/common below is already stated in src/chrome/DEPS. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/ap... File chrome/app_installer/app_installer.gypi (right): https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/ap... chrome/app_installer/app_installer.gypi:12: 'target_name': 'app_installer', I don't know the status of GN on Windows. Do you need to write a GN version of this target? https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... File chrome/app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:89: virtual void OnBeforeCreation(wchar_t** extra) OVERRIDE { nit: OVERRIDE -> override, no virtual. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:157: std::vector<std::pair<std::string, std::string> > kv_pairs; nit: This can be a StringPairs. See string_split.h. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:222: if (!base::LaunchProcess(CommandLine(setup_file), options, NULL)) { nit: base::CommandLine. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:223: base::DeleteFile(setup_file, false); You call this whether you succeed or not. base::LaunchProcess() succeeds or not. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:269: app_id = parsed_pairs[switches::kAppId]; Is switches::kAppId guaranteed to be in |parsed_pairs| if ParseTag() succeeds? If it's not there, calling std::map::operator[] creates it.
https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/DEPS File chrome/app_installer/DEPS (right): https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/DE... chrome/app_installer/DEPS:2: "+base", On 2014/10/24 19:15:23, Lei Zhang wrote: > You shouldn't need this. src/DEPS already says everyone can use base. Similarly, > chrome/common below is already stated in src/chrome/DEPS. Done. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... File chrome/app_installer/win/app_installer_main.cc (right): https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:89: virtual void OnBeforeCreation(wchar_t** extra) OVERRIDE { On 2014/10/24 19:15:24, Lei Zhang wrote: > nit: OVERRIDE -> override, no virtual. Done. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:157: std::vector<std::pair<std::string, std::string> > kv_pairs; On 2014/10/24 19:15:24, Lei Zhang wrote: > nit: This can be a StringPairs. See string_split.h. Done. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:222: if (!base::LaunchProcess(CommandLine(setup_file), options, NULL)) { On 2014/10/24 19:15:24, Lei Zhang wrote: > nit: base::CommandLine. Done. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:223: base::DeleteFile(setup_file, false); On 2014/10/24 19:15:24, Lei Zhang wrote: > You call this whether you succeed or not. base::LaunchProcess() succeeds or not. Done. https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/wi... chrome/app_installer/win/app_installer_main.cc:269: app_id = parsed_pairs[switches::kAppId]; On 2014/10/24 19:15:24, Lei Zhang wrote: > Is switches::kAppId guaranteed to be in |parsed_pairs| if ParseTag() succeeds? > If it's not there, calling std::map::operator[] creates it. Changed to map::find().
jackhou@chromium.org changed reviewers: + brettw@chromium.org
https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/ap... File chrome/app_installer/app_installer.gypi (right): https://codereview.chromium.org/423293004/diff/410001/chrome/app_installer/ap... chrome/app_installer/app_installer.gypi:12: 'target_name': 'app_installer', On 2014/10/24 19:15:23, Lei Zhang wrote: > I don't know the status of GN on Windows. Do you need to write a GN version of > this target? brettw@, could I get your advice on this?
chrome/ lgtm I don't see any Windows GN bots, so I'm thinking GN on Windows isn't quite ready yet.
jackhou@chromium.org changed reviewers: - brettw@chromium.org
On 2014/10/27 21:22:52, Lei Zhang wrote: > chrome/ lgtm > > I don't see any Windows GN bots, so I'm thinking GN on Windows isn't quite ready > yet. Ah cool, thanks. That should be it. Thanks for the reviews everyone.
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/423293004/450001
Message was sent while issue was closed.
Committed patchset #22 (id:450001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/511a873fa5264a01780ee06dd4c19314031ff2a2 Cr-Commit-Position: refs/heads/master@{#301492}
Message was sent while issue was closed.
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/423293004/diff/450001/chrome/installer/launch... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/423293004/diff/450001/chrome/installer/launch... chrome/installer/launcher_support/chrome_launcher_support.cc:130: GetChromeSxSPathForInstallationLevel(USER_LEVEL_INSTALLATION); Is there a reason this prioritizes user-level installation, whereas GetAnyChromePath prioritizes system-level?
Message was sent while issue was closed.
https://codereview.chromium.org/423293004/diff/450001/chrome/installer/launch... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://codereview.chromium.org/423293004/diff/450001/chrome/installer/launch... chrome/installer/launcher_support/chrome_launcher_support.cc:130: GetChromeSxSPathForInstallationLevel(USER_LEVEL_INSTALLATION); Just asked the same question on: https://codereview.chromium.org/685103004 feel free to ignore this and answer it there. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
