Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
53793002:
Initial implementation of Chrome Early Loading Framework (Closed)
|
Created:
7 years, 1 month ago by Cait (Slow) Modified:
4 years, 8 months ago Reviewers:
gab, cpu_(ooo_6.6-7.5), robertshield, darin (slow to review), Ben Goodger (Google), grt (UTC plus 2) CC:
grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChrome Early Loading Framework
chrome_elf.dll is shipped in Chrome's version directory to
ease updates, and is loaded early in chrome.exe's lifetime
by making it a private assembly in a subfolder of
chrome.exe's folder (see
http://msdn.microsoft.com/library/aa374224.aspx).
BUG= http://crosbug.com/p/23889
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234795
Patch Set 1 #
Total comments: 7
Patch Set 2 : Manifests #Patch Set 3 : Assemblies #
Total comments: 19
Patch Set 4 : Clean up #
Total comments: 8
Patch Set 5 : Generate manifest via gyp #
Total comments: 20
Patch Set 6 : Comments #Patch Set 7 : config fix #
Total comments: 3
Patch Set 8 : Actions and installer tests #
Total comments: 3
Patch Set 9 : Move InitChromeElf to after the fast notify #
Total comments: 22
Patch Set 10 : #
Total comments: 5
Patch Set 11 : Move elf init call back under fast notify #
Messages
Total messages: 35 (0 generated)
Some comments below. Cheers! Gab https://codereview.chromium.org/53793002/diff/1/chrome/app/chrome_exe_main_wi... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/53793002/diff/1/chrome/app/chrome_exe_main_wi... chrome/app/chrome_exe_main_win.cc:109: InitHelper(); rename to InitElf()? or InitChromeElf()? https://codereview.chromium.org/53793002/diff/1/chrome/chrome_elf/chrome_elf.... File chrome/chrome_elf/chrome_elf.manifest (right): https://codereview.chromium.org/53793002/diff/1/chrome/chrome_elf/chrome_elf.... chrome/chrome_elf/chrome_elf.manifest:3: <assemblyIdentity name='chrome_elf' version='0.0.0.0' Before using version 0.0.0.0 we probably want to understand what this field means exactly (I used 0.0.0.0 for the component build because "it works", but I have no idea whether it makes sense to ship this to a billion people ;)!) https://codereview.chromium.org/53793002/diff/1/chrome/chrome_elf/chrome_elf.... chrome/chrome_elf/chrome_elf.manifest:4: type='win32'/> This also needs investigation; win32 is probably wrong for 64-bit builds. https://codereview.chromium.org/53793002/diff/1/chrome/chrome_elf/chrome_elf.... chrome/chrome_elf/chrome_elf.manifest:5: <file name='chrome_elf.dll'/> "file name" is probably not required if the manifest is embedded. https://codereview.chromium.org/53793002/diff/1/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/53793002/diff/1/chrome/chrome_exe.gypi#newcod... chrome/chrome_exe.gypi:502: #'$(ProjectDir)\\chrome_elf\\chrome_elf.manifest', This doesn't belong in chrome.exe; rather in chrome_elf.gypi https://codereview.chromium.org/53793002/diff/1/chrome/installer/mini_install... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/1/chrome/installer/mini_install... chrome/installer/mini_installer/chrome.release:24: chrome_elf.dll: %(VersionDir)s\ alpha sort, i.e., put this above chrome_launcher.exe
https://codereview.chromium.org/53793002/diff/1/chrome/installer/mini_install... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/1/chrome/installer/mini_install... chrome/installer/mini_installer/chrome.release:24: chrome_elf.dll: %(VersionDir)s\ On 2013/10/31 13:44:31, gab wrote: > alpha sort, i.e., put this above chrome_launcher.exe That would be more of an 'elf' sort than an alpha sort: chrome_Elf.dll chrome_Launcher.exe chrome_Frame_helper.exe ^ My apologies, that was unhelpful. I would rather suggest alpha-sorting this whole list, but I would do it in a separate CL to avoid making the diff for this CL ugly. In fact: https://codereview.chromium.org/50673007 https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf.gypi File chrome/chrome_elf.gypi (right): https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf.gypi#n... chrome/chrome_elf.gypi:28: 'rules': [ Thinking about this a bit more, I believe there is some redundancy here. Instead of a 'rules' section, could this be accomplished by moving the 'action' up to an 'actions' section as with e.g. https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/ozone.gyp... https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_exe.gypi#n... chrome/chrome_exe.gypi:502: # TODO(caitkp): How do I refer to files in out\Release\gen here? Is this TODO now solved? https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:364: manifest_name = ("{version}.manifest".format(version=current_version)) nit: don't need the outer ()
Also, please add an OWNERs file under chrome_elf/
https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf.gypi File chrome/chrome_elf.gypi (right): https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf.gypi#n... chrome/chrome_elf.gypi:8: 'target_name': 'chrome_elf', does this target depend on anything in src/chrome? i wonder if chrome_elf should be its own top-level entity (in src/) rather than in src/chrome. what else might be in here? https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf/chrome... File chrome/chrome_elf/chrome_elf_main.h (right): https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf/chrome... chrome/chrome_elf/chrome_elf_main.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: there's no " (c)" in the most recent boilerplate. remove it here and elsewhere. https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf/chrome... chrome/chrome_elf/chrome_elf_main.h:8: extern "C" { nit: extern "C" void InitHelper(); https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf/chrome... chrome/chrome_elf/chrome_elf_main.h:9: void InitHelper(); is there a better name for this? maybe InitChromeElf()? https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:351: def AddVersionAssemblyManifest(staging_dir, current_version): nit: add a doc string https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:364: manifest_name = ("{version}.manifest".format(version=current_version)) feel free to ignore this nit: "%s.manifest" % current_version is way more readable to me for something simple like this https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:366: version_manifest_file = open( with open(os.path.join(version_dir, manifest_name), 'w') as f: f.write(version_manifest)
Thanks! -- Here is a more cleaned up version. https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf.gypi File chrome/chrome_elf.gypi (right): https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf.gypi#n... chrome/chrome_elf.gypi:8: 'target_name': 'chrome_elf', On 2013/11/05 03:08:36, grt wrote: > does this target depend on anything in src/chrome? i wonder if chrome_elf should > be its own top-level entity (in src/) rather than in src/chrome. what else might > be in here? I was wondering this too. I've moved it up a level. https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf.gypi#n... chrome/chrome_elf.gypi:28: 'rules': [ On 2013/11/05 02:23:55, robertshield wrote: > Thinking about this a bit more, I believe there is some redundancy here. Instead > of a 'rules' section, could this be accomplished by moving the 'action' up to an > 'actions' section as with e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/ozone.gyp... Done. https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf/chrome... File chrome/chrome_elf/chrome_elf_main.h (right): https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf/chrome... chrome/chrome_elf/chrome_elf_main.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/11/05 03:08:36, grt wrote: > nit: there's no " (c)" in the most recent boilerplate. remove it here and > elsewhere. Done. https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf/chrome... chrome/chrome_elf/chrome_elf_main.h:8: extern "C" { On 2013/11/05 03:08:36, grt wrote: > nit: > extern "C" void InitHelper(); Done. https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_elf/chrome... chrome/chrome_elf/chrome_elf_main.h:9: void InitHelper(); On 2013/11/05 03:08:36, grt wrote: > is there a better name for this? maybe InitChromeElf()? Done. https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/53793002/diff/100001/chrome/chrome_exe.gypi#n... chrome/chrome_exe.gypi:502: # TODO(caitkp): How do I refer to files in out\Release\gen here? On 2013/11/05 02:23:55, robertshield wrote: > Is this TODO now solved? Done. https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:351: def AddVersionAssemblyManifest(staging_dir, current_version): On 2013/11/05 03:08:36, grt wrote: > nit: add a doc string Done. https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:364: manifest_name = ("{version}.manifest".format(version=current_version)) On 2013/11/05 03:08:36, grt wrote: > feel free to ignore this nit: > "%s.manifest" % current_version > is way more readable to me for something simple like this Done. https://codereview.chromium.org/53793002/diff/100001/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:366: version_manifest_file = open( On 2013/11/05 03:08:36, grt wrote: > with open(os.path.join(version_dir, manifest_name), 'w') as f: > f.write(version_manifest) Done.
Looks great :)! https://codereview.chromium.org/53793002/diff/180001/chrome/app/DEPS File chrome/app/DEPS (right): https://codereview.chromium.org/53793002/diff/180001/chrome/app/DEPS#newcode11 chrome/app/DEPS:11: "+chrome_elf/chrome_elf_main.h", nit: chrome_ comes before chromeos in ASCII sort. This DEPS rule also feels weird; don't we usually instead add a chrome_elf/public/* interface? Or is that deemed overkill in this case? https://codereview.chromium.org/53793002/diff/180001/chrome/tools/build/win/c... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/53793002/diff/180001/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:369: with open(os.path.join(version_dir, manifest_name), 'w') as f: I'm guessing this automatically closes |f| when it goes out of scope? https://codereview.chromium.org/53793002/diff/180001/chrome_elf/chrome_elf.gypi File chrome_elf/chrome_elf.gypi (right): https://codereview.chromium.org/53793002/diff/180001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:4: '../chrome/version.gypi', This version.gypi stuff is pretty cool :)! https://codereview.chromium.org/53793002/diff/180001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:11: '..', nit: overly indented? https://codereview.chromium.org/53793002/diff/180001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:38: 'message': 'Generating additional manifest files' Seems you could do something like 'Generating <@(_outputs)', e.g., https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome.gyp&... https://codereview.chromium.org/53793002/diff/180001/chrome_elf/chrome_elf_ma... File chrome_elf/chrome_elf_main.h (left): https://codereview.chromium.org/53793002/diff/180001/chrome_elf/chrome_elf_ma... chrome_elf/chrome_elf_main.h:5: #include "chrome/common/importer/imported_favicon_usage.h" nit: perhaps upload with --similarity=100 for it to not consider copies when generating diffs for this CL as there are none.
https://codereview.chromium.org/53793002/diff/180001/chrome/tools/build/win/c... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/53793002/diff/180001/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:369: with open(os.path.join(version_dir, manifest_name), 'w') as f: On 2013/11/05 16:43:20, gab wrote: > I'm guessing this automatically closes |f| when it goes out of scope? at the end of the scope opened by the "with", yes.
Ok -- I think we're about ready for OWNERS. Greg: can you take a quick look at chrome.release? I'm currently grabbing anything that looks like *.*.*.*.manifest, but I suspect there is probably a less hacky way to do things. https://codereview.chromium.org/53793002/diff/180001/chrome_elf/chrome_elf.gypi File chrome_elf/chrome_elf.gypi (right): https://codereview.chromium.org/53793002/diff/180001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:11: '..', On 2013/11/05 16:43:20, gab wrote: > nit: overly indented? Done.
[+darin] for owners
Please augment the CL description as such: This CL makes it such that chrome_elf.dll is a load time dependency of chrome.exe; the tricky part relies in requiring that chrome_elf.dll live in the version_dir (having everything in the version_dir is much easier for auto-update; in-use updates in particular). Putting it in the version_dir makes chrome_elf.dll not found by the Windows loader when chrome.exe launches... To fix this, this CL takes advantage of the "Assembly Searching Sequence" (http://msdn.microsoft.com/library/aa374224.aspx) which states that private assemblies can be placed in a subfolder if that subfolder has the same name as the assembly itself. This is the best way to have chrome.exe find chrome_elf.dll at load-time without putting it besides chrome.exe. An even better way to do this would be to embed an application config in chrome.exe that would refer to the proper version_dir via a probing\privatePath (http://msdn.microsoft.com/library/aa374182.aspx) without having to make the version_dir the name of the assembly itself; this method however is unfortunately only available as of Windows 7... https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gypi File chrome_elf/chrome_elf.gypi (right): https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:21: # built. s/This will be embedded in chrome.exe when it is built./This will be merged into chrome.exe.manifest and embedded into chrome.exe when it is built. https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:52: '<(version_path)', Should <(version_full) be an input here? What about <(version_py_path)? https://codereview.chromium.org/53793002/diff/300001/chrome_elf/version_assem... File chrome_elf/version_assembly_manifest.template (right): https://codereview.chromium.org/53793002/diff/300001/chrome_elf/version_assem... chrome_elf/version_assembly_manifest.template:5: version='@MAJOR@.@MINOR@.@BUILD@.@PATCH@' As discussed offline, we have no choice but to make the name the VERSION string for the version dir to be considered an assembly; this raises name conflicts concerns if an assembly uses the same name in the GAC (Global Assembly Cache) which has priority over private assemblies. We could mitigate this by making the "version" field here a random version string; different in every build (or perhaps even identical, but Chrome-unique; i.e., some kind of "key")!
On 2013/11/06 16:07:24, gab wrote: > Please augment the CL description as such: Personally, I find that too wordy and full of extraneous details. How about: Chrome Early Loading Framework. chrome_elf.dll is shipped in Chrome's version directory to ease updates, and is loaded early in chrome.exe's lifetime by making it a private assembly in a subfolder of chrome.exe's folder (see http://msdn.microsoft.com/library/aa374224.aspx).
https://codereview.chromium.org/53793002/diff/300001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/53793002/diff/300001/chrome/chrome.gyp#newcod... chrome/chrome.gyp:124: '../chrome_elf/chrome_elf.gypi', remove this as per comment in chrome_elf.gypi https://codereview.chromium.org/53793002/diff/300001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/53793002/diff/300001/chrome/chrome_exe.gypi#n... chrome/chrome_exe.gypi:465: 'chrome_elf', change this to '../chrome_elf/chrome_elf.gyp:chrome_elf' as per comment in chrome_elf.gypi (and adjust sort order) https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:25: *.*.*.*.manifest: %(VersionDir)s\ Now that I think about it, I wonder if we could get into trouble if there are multiple versions laying around. It would be a little cleaner to modify src/chrome/tools/build/win/create_installer_archive.py so that you could do: %(ChromeVersion).manifest: %(VersionDir)s\ This may be as simple as adding: variables['ChromeVersion'] = current_version on line 179 of create_installer_archive.py https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gypi File chrome_elf/chrome_elf.gypi (right): https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:1: { now that chrome_elf is its own top-level dir, i think this should become chrome_elf.gyp rather than a .gypi file only relevant within the context of chrome.gyp. see, for example, net/net.gyp, base/base.gyp, content/content.gyp. https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:1: { # Copyright 2013 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:2: remove spurious blank line
On 2013/11/06 18:46:49, grt wrote: > On 2013/11/06 16:07:24, gab wrote: > > Please augment the CL description as such: > > Personally, I find that too wordy and full of extraneous details. How about: > > Chrome Early Loading Framework. > > chrome_elf.dll is shipped in Chrome's version directory to ease updates, > and is loaded early in chrome.exe's lifetime by making it a private > assembly in a subfolder of chrome.exe's folder (see > http://msdn.microsoft.com/library/aa374224.aspx). Perhaps wording can be different, but I think it's important to document all the things we've looked at + links to the documentation we found... since there is no other documentation and it took us a while to arrive to this conclusion...
On 2013/11/06 19:48:53, gab wrote: > On 2013/11/06 18:46:49, grt wrote: > > On 2013/11/06 16:07:24, gab wrote: > > > Please augment the CL description as such: > > > > Personally, I find that too wordy and full of extraneous details. How about: > > > > Chrome Early Loading Framework. > > > > chrome_elf.dll is shipped in Chrome's version directory to ease updates, > > and is loaded early in chrome.exe's lifetime by making it a private > > assembly in a subfolder of chrome.exe's folder (see > > http://msdn.microsoft.com/library/aa374224.aspx). > > Perhaps wording can be different, but I think it's important to document all the > things we've looked at + links to the documentation we found... since there is > no other documentation and it took us a while to arrive to this conclusion... Indeed, lots of hard work went into figuring this out. It would be unfortunate to lose the knowledge. How about a README in the directory or doc comments in chrome_elf_main? This both seem more discoverable than the CL description, and easier to keep up-to-date if things should change (like someday dropping XP support). In unrelated news, what do you think about adding a DEPS file to the dir with an include_rules stanza?
darin: friendly ping -- thanks! https://codereview.chromium.org/53793002/diff/300001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/53793002/diff/300001/chrome/chrome.gyp#newcod... chrome/chrome.gyp:124: '../chrome_elf/chrome_elf.gypi', On 2013/11/06 19:07:29, grt wrote: > remove this as per comment in chrome_elf.gypi Done. https://codereview.chromium.org/53793002/diff/300001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/53793002/diff/300001/chrome/chrome_exe.gypi#n... chrome/chrome_exe.gypi:465: 'chrome_elf', On 2013/11/06 19:07:29, grt wrote: > change this to '../chrome_elf/chrome_elf.gyp:chrome_elf' as per comment in > chrome_elf.gypi (and adjust sort order) Done. https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:25: *.*.*.*.manifest: %(VersionDir)s\ On 2013/11/06 19:07:29, grt wrote: > Now that I think about it, I wonder if we could get into trouble if there are > multiple versions laying around. It would be a little cleaner to modify > src/chrome/tools/build/win/create_installer_archive.py so that you could do: > > %(ChromeVersion).manifest: %(VersionDir)s\ > > This may be as simple as adding: > variables['ChromeVersion'] = current_version > on line 179 of create_installer_archive.py Hrm...For some reason wild cards (*.*.*.*.manifest) seem to work, but variables (%(ChromeDir)s.manifest) do not -- thoughts? https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gypi File chrome_elf/chrome_elf.gypi (right): https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:1: { On 2013/11/06 19:07:29, grt wrote: > # Copyright 2013 The Chromium Authors. All rights reserved. > # Use of this source code is governed by a BSD-style license that can be > # found in the LICENSE file. Done. https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:1: { On 2013/11/06 19:07:29, grt wrote: > now that chrome_elf is its own top-level dir, i think this should become > chrome_elf.gyp rather than a .gypi file only relevant within the context of > chrome.gyp. see, for example, net/net.gyp, base/base.gyp, content/content.gyp. Done. https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:2: On 2013/11/06 19:07:29, grt wrote: > remove spurious blank line Done. https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:21: # built. On 2013/11/06 16:07:25, gab wrote: > s/This will be embedded in chrome.exe when it is built./This will be merged into > chrome.exe.manifest and embedded into chrome.exe when it is built. Done. https://codereview.chromium.org/53793002/diff/300001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gypi:52: '<(version_path)', On 2013/11/06 16:07:25, gab wrote: > Should <(version_full) be an input here? What about <(version_py_path)? For reference: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/version.gypi version_py_path is the path to chrome/tools/build/version.py (the script that we're passing the inputs to). version_path is the path to chrome/VERSION which stores current version info (version.py takes this file as input) version_full is a string containing the current versionin MAJOR.MINOR.BUILD.PATCH form (I only use it for the manifest name). The 'inputs' section does seem a bit redundant though. It might be possible to either just pass '<@(_inputs)' to version_py_path, or remove the 'inputs' section altogether, and just pass in the variables -- I'll look into that.
https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:25: *.*.*.*.manifest: %(VersionDir)s\ On 2013/11/06 21:18:06, Cait Phillips wrote: > On 2013/11/06 19:07:29, grt wrote: > > Now that I think about it, I wonder if we could get into trouble if there are > > multiple versions laying around. It would be a little cleaner to modify > > src/chrome/tools/build/win/create_installer_archive.py so that you could do: > > > > %(ChromeVersion).manifest: %(VersionDir)s\ > > > > This may be as simple as adding: > > variables['ChromeVersion'] = current_version > > on line 179 of create_installer_archive.py > > Hrm...For some reason wild cards (*.*.*.*.manifest) seem to work, but variables > (%(ChromeDir)s.manifest) do not -- thoughts? Could it be that the script only currently expands variables on the RHS of these expressions?
Please augment the test framework in chrome/test/mini_installer so that it tests for the elf and various manifest files. I think this will be as simple as adjusting the *_installed.prop files. https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:25: *.*.*.*.manifest: %(VersionDir)s\ On 2013/11/06 22:28:27, gab wrote: > On 2013/11/06 21:18:06, Cait Phillips wrote: > > On 2013/11/06 19:07:29, grt wrote: > > > Now that I think about it, I wonder if we could get into trouble if there > are > > > multiple versions laying around. It would be a little cleaner to modify > > > src/chrome/tools/build/win/create_installer_archive.py so that you could do: > > > > > > %(ChromeVersion).manifest: %(VersionDir)s\ > > > > > > This may be as simple as adding: > > > variables['ChromeVersion'] = current_version > > > on line 179 of create_installer_archive.py > > > > Hrm...For some reason wild cards (*.*.*.*.manifest) seem to work, but > variables > > (%(ChromeDir)s.manifest) do not -- thoughts? > > Could it be that the script only currently expands variables on the RHS of these > expressions? I think you're right. Grn. So much for that idea. Looks like the choices are: 1) live with this 2) put a hack in create_installer_archive.py's CopySectionFilesToStagingDir to special-case options that are variables 3) modify create_installer_archive.py so it doesn't use python's ConfigParser 4) rewrite create_installer_archive.py so it doesn't use this ridiculous file format from the early 80s As long as we don't accidentally ship builds with multiple versions' manifests (the installer automation test suite should be given the brains to verify that the elf and manifests are preset, BTW), then this is okay as-is. https://codereview.chromium.org/53793002/diff/400002/chrome/tools/build/win/c... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/53793002/diff/400002/chrome/tools/build/win/c... chrome/tools/build/win/create_installer_archive.py:185: config.set('GENERAL', '%s.manifest' % current_version, '%(VersionDir)s\\') this scares me since it involves magic. reading chrome.release on its own is no longer sufficient to see what goes into the archive. https://codereview.chromium.org/53793002/diff/400002/chrome_elf/README File chrome_elf/README (right): https://codereview.chromium.org/53793002/diff/400002/chrome_elf/README#newcode21 chrome_elf/README:21: putting it besides chrome.exe in the application directory. besides -> beside https://codereview.chromium.org/53793002/diff/400002/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (right): https://codereview.chromium.org/53793002/diff/400002/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gyp:25: { # Construct a manifest file declaring chrome.exe's dependency on chrome_elf doesn't need these, does it? if i understand correctly, these manifests are needed by whatever it is that links to chrome_elf. perhaps chrome_elf/ could contain .gypi snippets meant to be included by targets that link to it? take a look at build\grit_action.gypi and how it's used for an idea of what i mean. apologies for not bringing this up earlier.
https://codereview.chromium.org/53793002/diff/520001/chrome/app/chrome_exe_ma... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/53793002/diff/520001/chrome/app/chrome_exe_ma... chrome/app/chrome_exe_main_win.cc:113: can we do the init after the fast notify? If we fast notify we unload so the work done is lost afaik.
Hi Carlos: PTAL -- thanks! https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/300001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:25: *.*.*.*.manifest: %(VersionDir)s\ On 2013/11/06 22:28:27, gab wrote: > On 2013/11/06 21:18:06, Cait Phillips wrote: > > On 2013/11/06 19:07:29, grt wrote: > > > Now that I think about it, I wonder if we could get into trouble if there > are > > > multiple versions laying around. It would be a little cleaner to modify > > > src/chrome/tools/build/win/create_installer_archive.py so that you could do: > > > > > > %(ChromeVersion).manifest: %(VersionDir)s\ > > > > > > This may be as simple as adding: > > > variables['ChromeVersion'] = current_version > > > on line 179 of create_installer_archive.py > > > > Hrm...For some reason wild cards (*.*.*.*.manifest) seem to work, but > variables > > (%(ChromeDir)s.manifest) do not -- thoughts? > > Could it be that the script only currently expands variables on the RHS of these > expressions? That appears to be the case. I've changed it so that this file is added to the config manually.
https://codereview.chromium.org/53793002/diff/520001/chrome/app/chrome_exe_ma... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/53793002/diff/520001/chrome/app/chrome_exe_ma... chrome/app/chrome_exe_main_win.cc:113: On 2013/11/07 19:29:32, cpu wrote: > can we do the init after the fast notify? > > If we fast notify we unload so the work done is lost afaik. This call doesn't actually do any work; all it does is force the addition of an entry in the IAT for chrome_elf.dll which forces the load time dependency. A comment should probably be added here to state this.
https://codereview.chromium.org/53793002/diff/520001/chrome/app/chrome_exe_ma... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/53793002/diff/520001/chrome/app/chrome_exe_ma... chrome/app/chrome_exe_main_win.cc:113: On 2013/11/07 20:19:40, gab wrote: > On 2013/11/07 19:29:32, cpu wrote: > > can we do the init after the fast notify? > > > > If we fast notify we unload so the work done is lost afaik. > > This call doesn't actually do any work; all it does is force the addition of an > entry in the IAT for chrome_elf.dll which forces the load time dependency. A > comment should probably be added here to state this. Ok,then so still can called after? I just want to avoid a year from now somebody putting code in the empty function. You can also bind to an exported variable afaik.
On 2013/11/07 22:10:37, cpu wrote: > https://codereview.chromium.org/53793002/diff/520001/chrome/app/chrome_exe_ma... > File chrome/app/chrome_exe_main_win.cc (right): > > https://codereview.chromium.org/53793002/diff/520001/chrome/app/chrome_exe_ma... > chrome/app/chrome_exe_main_win.cc:113: > On 2013/11/07 20:19:40, gab wrote: > > On 2013/11/07 19:29:32, cpu wrote: > > > can we do the init after the fast notify? > > > > > > If we fast notify we unload so the work done is lost afaik. > > > > This call doesn't actually do any work; all it does is force the addition of > an > > entry in the IAT for chrome_elf.dll which forces the load time dependency. A > > comment should probably be added here to state this. > > Ok,then so still can called after? I just want to avoid a year from now somebody > putting code in the empty function. Good idea, I moved the call to after the fast notify. > You can also bind to an exported variable afaik. Afaik, this uses the /INCLUDE linker directive (a.k.a ForceSymbolReferences) which the gyp generator doesn't support yet. I'll see about adding it, but do you object to the dummy call to force the import in this CL?
lg, comments below, mostly about opinions of where things should belong. Cheers, Gab https://codereview.chromium.org/53793002/diff/820001/chrome/app/chrome_exe_ma... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/53793002/diff/820001/chrome/app/chrome_exe_ma... chrome/app/chrome_exe_main_win.cc:112: InitChromeElf(); I hear we don't need this force an entry for chrome_elf.dll in the IAT, remove it (or replace it with the better solution, Robert?) https://codereview.chromium.org/53793002/diff/820001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/53793002/diff/820001/chrome/chrome_exe.gypi#n... chrome/chrome_exe.gypi:522: '../chrome_elf/chrome_exe_manifest_action.gypi', Make sure (and add the result to the TEST= section of this CL) that these actions get triggered if chrome/VERSION is touched (this is the only benefit I see of having these actions triggered from this target rather than from chrome_elf). https://codereview.chromium.org/53793002/diff/820001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/820001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:25: *.*.*.*.manifest: %(VersionDir)s\ This file was recently sorted by Robert (you'll want to merge from trunk). How much harder is it to make create_installer_script.py understand variables on the LHS of the ':' here? If that's not possible and the only reason for putting this here is Greg's concern that everything that lands in the archive should be referenced here, could we instead just put a comment and make the installer script pull the right manifest? I don't really like the glob... https://codereview.chromium.org/53793002/diff/820001/chrome/test/mini_install... File chrome/test/mini_installer/config/chrome_system_installed.prop (right): https://codereview.chromium.org/53793002/diff/820001/chrome/test/mini_install... chrome/test/mini_installer/config/chrome_system_installed.prop:6: "$PROGRAM_FILES\\$CHROME_DIR\\Application\\$MINI_INSTALLER_FILE_VERSION\\chrome_elf.dll": Please add as a TEST= section to this CL what command you ran to verify these tests (and please also make sure this change doesn't break the FYI bot). https://codereview.chromium.org/53793002/diff/820001/chrome_elf/DEPS File chrome_elf/DEPS (right): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/DEPS#newcode2 chrome_elf/DEPS:2: The only other cases of an empty include file in base/DEPS and it doesn't have an empty line here.. https://codereview.chromium.org/53793002/diff/820001/chrome_elf/README File chrome_elf/README (right): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/README#newcode5 chrome_elf/README:5: private assembly in a subfolder of chrome.exe's folder (see Technically I think the assembly is the manifest, which can have many DLLs. I would change this last sentence to: "This is done by turning the version directory into a private assembly which refers to chrome_elf.dll (http://msdn.microsoft.com/library/aa374224.aspx)." And I would remove the last two paragraphs (I know I originally proposed them, but I find your new wording for the first paragraph is better and more succinct, making the last two paragraphs redundant). Also consider adding a paragraph between the first one and the current second one as to the potential naming conflict issues with this approach and why we'd ideally prefer the approach described in the next paragraph. https://codereview.chromium.org/53793002/diff/820001/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (left): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/chrome_elf.gy... chrome_elf/chrome_elf.gyp:16: 'public/provider/web/web_state.h', The next time you upload, consider using "git cl upload --similarity=0" since no files in this CL should be considered copies of old files -- the current diffs are bogus. https://codereview.chromium.org/53793002/diff/820001/chrome_elf/chrome_elf_ma... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/chrome_elf_ma... chrome_elf/chrome_elf_main.cc:9: void InitChromeElf() {} Should this method be kept, I think we prefer no-op methods to still have a comment in their implement as to why, i.e.: void InitChromeElf() { // No-op, used by dependents to force a load-time dependency on chrome_elf.dll. } https://codereview.chromium.org/53793002/diff/820001/chrome_elf/version_assem... File chrome_elf/version_assembly_manifest_action.gypi (right): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/version_assem... chrome_elf/version_assembly_manifest_action.gypi:1: # Copyright 2013 The Chromium Authors. All rights reserved. This isn't really chrome_elf specific (beyond the fact that for now this assembly is only used to pull chrome_elf.dll). I feel this should live in chrome_exe land (i.e., it's not more chrome_elf'ish than the dependency of chrome_exe on chrome_elf itself). Should we want to add another DLL to the list of assemblies, if it's here we'll have to move it, if it's in chrome we'll just have to add that DLL to the files in the template. The same is true for the other manifest, to be embedded in chrome.exe, which is even less chrome_elf specific. I would put the manifests+templates+gyp actions+README all in their own directory, probably under chrome/, e.g. chrome/version_assembly (I don't have a good enough knowledge of the logic behind the chrome/ layout to know exactly where it should fit, but something like that). https://codereview.chromium.org/53793002/diff/820001/chrome_elf/version_assem... chrome_elf/version_assembly_manifest_action.gypi:15: # version_full: string: version string in WWWW.X.YYYY.Z form. Why WWWW.X.YYYY.Z? Nothing prevents X/Z to be more than 1 char AFAIK (I sure know Z has been 2 chars in many cases I've seen). Nor, does W/Y have to be 4 chars. I'd just say "W.X.Y.Z form".
https://codereview.chromium.org/53793002/diff/820001/chrome/app/chrome_exe_ma... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/53793002/diff/820001/chrome/app/chrome_exe_ma... chrome/app/chrome_exe_main_win.cc:112: InitChromeElf(); On 2013/11/11 20:11:38, gab wrote: > I hear we don't need this force an entry for chrome_elf.dll in the IAT, remove > it (or replace it with the better solution, Robert?) The better solution will require some gyp work (see my earlier comment). Suggest leaving this here until we can fix gyp.
I think a call is fine for forcing. lgtm
https://codereview.chromium.org/53793002/diff/820001/chrome/test/mini_install... File chrome/test/mini_installer/config/chrome_system_installed.prop (right): https://codereview.chromium.org/53793002/diff/820001/chrome/test/mini_install... chrome/test/mini_installer/config/chrome_system_installed.prop:6: "$PROGRAM_FILES\\$CHROME_DIR\\Application\\$MINI_INSTALLER_FILE_VERSION\\chrome_elf.dll": On 2013/11/11 20:11:38, gab wrote: > Please add as a TEST= section to this CL what command you ran to verify these > tests (and please also make sure this change doesn't break the FYI bot). TEST= is a place to put instructions for QA so that they can test the change, not a description of how the author tested the CL.
+Ben for OWNERs, as it looks like Darin is out this week.
Pulling in Robert's reordered chrome.release and cleaning up a few comments. https://codereview.chromium.org/53793002/diff/820001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/53793002/diff/820001/chrome/chrome_exe.gypi#n... chrome/chrome_exe.gypi:522: '../chrome_elf/chrome_exe_manifest_action.gypi', On 2013/11/11 20:11:38, gab wrote: > Make sure (and add the result to the TEST= section of this CL) that these > actions get triggered if chrome/VERSION is touched (this is the only benefit I > see of having these actions triggered from this target rather than from > chrome_elf). Done. https://codereview.chromium.org/53793002/diff/820001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/820001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:25: *.*.*.*.manifest: %(VersionDir)s\ On 2013/11/11 20:11:38, gab wrote: > This file was recently sorted by Robert (you'll want to merge from trunk). > > How much harder is it to make create_installer_script.py understand variables on > the LHS of the ':' here? > If that's not possible and the only reason for putting this here is Greg's > concern that everything that lands in the archive should be referenced here, > could we instead just put a comment and make the installer script pull the right > manifest? I don't really like the glob... I looked into it briefly, and I think it is non trivial (or at least, not obvious) how to make create_installer_script.py understand variables on the LHS. Since I don't think it makes sense to block the landing of this change on refactoring how chrome.release is parsed/managed, I think the options are wildcards in chrome.release or magic in the installer script. Eventually, I'd like to figure out how to delete old .manifests from the build dir, to avoid them cluttering up dev builds (installer script magic would negate the need for this, but would still be magic, and you'd have to run the script just to see the manifest, which seems dodgy). For the time being, I'll defer to Gab and Greg for the magic vs wildcards debate. https://codereview.chromium.org/53793002/diff/820001/chrome/test/mini_install... File chrome/test/mini_installer/config/chrome_system_installed.prop (right): https://codereview.chromium.org/53793002/diff/820001/chrome/test/mini_install... chrome/test/mini_installer/config/chrome_system_installed.prop:6: "$PROGRAM_FILES\\$CHROME_DIR\\Application\\$MINI_INSTALLER_FILE_VERSION\\chrome_elf.dll": On 2013/11/11 20:11:38, gab wrote: > Please add as a TEST= section to this CL what command you ran to verify these > tests (and please also make sure this change doesn't break the FYI bot). Done. https://codereview.chromium.org/53793002/diff/820001/chrome_elf/DEPS File chrome_elf/DEPS (right): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/DEPS#newcode2 chrome_elf/DEPS:2: On 2013/11/11 20:11:38, gab wrote: > The only other cases of an empty include file in base/DEPS and it doesn't have > an empty line here.. Done. https://codereview.chromium.org/53793002/diff/820001/chrome_elf/README File chrome_elf/README (right): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/README#newcode5 chrome_elf/README:5: private assembly in a subfolder of chrome.exe's folder (see On 2013/11/11 20:11:38, gab wrote: > Technically I think the assembly is the manifest, which can have many DLLs. > > I would change this last sentence to: > > "This is done by turning the version directory into a private assembly which > refers to chrome_elf.dll (http://msdn.microsoft.com/library/aa374224.aspx%29.%22 > > And I would remove the last two paragraphs (I know I originally proposed them, > but I find your new wording for the first paragraph is better and more succinct, > making the last two paragraphs redundant). > > Also consider adding a paragraph between the first one and the current second > one as to the potential naming conflict issues with this approach and why we'd > ideally prefer the approach described in the next paragraph. Done. https://codereview.chromium.org/53793002/diff/820001/chrome_elf/chrome_elf_ma... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/chrome_elf_ma... chrome_elf/chrome_elf_main.cc:9: void InitChromeElf() {} On 2013/11/11 20:11:38, gab wrote: > Should this method be kept, I think we prefer no-op methods to still have a > comment in their implement as to why, i.e.: > > void InitChromeElf() { > // No-op, used by dependents to force a load-time dependency on chrome_elf.dll. > } Done. https://codereview.chromium.org/53793002/diff/820001/chrome_elf/version_assem... File chrome_elf/version_assembly_manifest_action.gypi (right): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/version_assem... chrome_elf/version_assembly_manifest_action.gypi:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2013/11/11 20:11:38, gab wrote: > This isn't really chrome_elf specific (beyond the fact that for now this > assembly is only used to pull chrome_elf.dll). > > I feel this should live in chrome_exe land (i.e., it's not more chrome_elf'ish > than the dependency of chrome_exe on chrome_elf itself). > > Should we want to add another DLL to the list of assemblies, if it's here we'll > have to move it, if it's in chrome we'll just have to add that DLL to the files > in the template. > > > The same is true for the other manifest, to be embedded in chrome.exe, which is > even less chrome_elf specific. > > I would put the manifests+templates+gyp actions+README all in their own > directory, probably under chrome/, e.g. chrome/version_assembly (I don't have a > good enough knowledge of the logic behind the chrome/ layout to know exactly > where it should fit, but something like that). From discussing with grt, I have a follow up patch I'm working on to tidy up how and when these manifests are created (and generalize the actions a bit). Are you ok with these changes being incorporated into that patch? https://codereview.chromium.org/53793002/diff/820001/chrome_elf/version_assem... chrome_elf/version_assembly_manifest_action.gypi:15: # version_full: string: version string in WWWW.X.YYYY.Z form. On 2013/11/11 20:11:38, gab wrote: > Why WWWW.X.YYYY.Z? Nothing prevents X/Z to be more than 1 char AFAIK (I sure > know Z has been 2 chars in many cases I've seen). Nor, does W/Y have to be 4 > chars. > > I'd just say "W.X.Y.Z form". Done.
lgtm w/ comments below Also, regarding the debate with grt about TEST=; he did convince me that we should do what the guide says, I still think we should mention this somewhere in the CL description, I'll let him state what he thinks that should look like, but just wanted to state that I will not further oppose his opinion on this anymore :). Cheers! Gab https://codereview.chromium.org/53793002/diff/820001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/820001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:25: *.*.*.*.manifest: %(VersionDir)s\ On 2013/11/12 16:33:54, Cait Phillips wrote: > On 2013/11/11 20:11:38, gab wrote: > > This file was recently sorted by Robert (you'll want to merge from trunk). > > > > How much harder is it to make create_installer_script.py understand variables > on > > the LHS of the ':' here? > > If that's not possible and the only reason for putting this here is Greg's > > concern that everything that lands in the archive should be referenced here, > > could we instead just put a comment and make the installer script pull the > right > > manifest? I don't really like the glob... > I looked into it briefly, and I think it is non trivial (or at least, not > obvious) how to make create_installer_script.py understand variables on the LHS. > Since I don't think it makes sense to block the landing of this change on > refactoring how chrome.release is parsed/managed, I think the options are > wildcards in chrome.release or magic in the installer script. > > Eventually, I'd like to figure out how to delete old .manifests from the build > dir, to avoid them cluttering up dev builds (installer script magic would negate > the need for this, but would still be magic, and you'd have to run the script > just to see the manifest, which seems dodgy). > > For the time being, I'll defer to Gab and Greg for the magic vs wildcards > debate. Wlidcard w/ comment sgtm then (with script to delete old manifests). https://codereview.chromium.org/53793002/diff/820001/chrome_elf/version_assem... File chrome_elf/version_assembly_manifest_action.gypi (right): https://codereview.chromium.org/53793002/diff/820001/chrome_elf/version_assem... chrome_elf/version_assembly_manifest_action.gypi:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2013/11/12 16:33:54, Cait Phillips wrote: > On 2013/11/11 20:11:38, gab wrote: > > This isn't really chrome_elf specific (beyond the fact that for now this > > assembly is only used to pull chrome_elf.dll). > > > > I feel this should live in chrome_exe land (i.e., it's not more chrome_elf'ish > > than the dependency of chrome_exe on chrome_elf itself). > > > > Should we want to add another DLL to the list of assemblies, if it's here > we'll > > have to move it, if it's in chrome we'll just have to add that DLL to the > files > > in the template. > > > > > > The same is true for the other manifest, to be embedded in chrome.exe, which > is > > even less chrome_elf specific. > > > > I would put the manifests+templates+gyp actions+README all in their own > > directory, probably under chrome/, e.g. chrome/version_assembly (I don't have > a > > good enough knowledge of the logic behind the chrome/ layout to know exactly > > where it should fit, but something like that). > > From discussing with grt, I have a follow up patch I'm working on to tidy up how > and when these manifests are created (and generalize the actions a bit). Are you > ok with these changes being incorporated into that patch? What's that plan? It feels weird to dump everything here just to move it all to another directory late, but I could be convinced about this CL as a temporary step in a clear long-term plan (with TODOs for the remaining work in this CL). https://codereview.chromium.org/53793002/diff/1000001/chrome/app/chrome_exe_m... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/53793002/diff/1000001/chrome/app/chrome_exe_m... chrome/app/chrome_exe_main_win.cc:111: // for chrome_elf.dll to force the load time dependency. s/the load time.../a load time... Please also add a TODO here explaining how you plan to get to replace this by something better to force the dependency. https://codereview.chromium.org/53793002/diff/1000001/chrome/app/chrome_exe_m... chrome/app/chrome_exe_main_win.cc:112: InitChromeElf(); Why move this back before AttemptFastNotify()? https://codereview.chromium.org/53793002/diff/1000001/chrome/installer/mini_i... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/53793002/diff/1000001/chrome/installer/mini_i... chrome/installer/mini_installer/chrome.release:14: *.*.*.*.manifest: %(VersionDir)s\ I'd put this above this block (on line 11) and give it its own comment of what it is and why its a glob, etc. https://codereview.chromium.org/53793002/diff/1000001/chrome/installer/mini_i... chrome/installer/mini_installer/chrome.release:19: chrome_elf.dll: %(VersionDir)s\ Sort above chrome_frame_helper.dll. https://codereview.chromium.org/53793002/diff/1000001/chrome_elf/README File chrome_elf/README (right): https://codereview.chromium.org/53793002/diff/1000001/chrome_elf/README#newco... chrome_elf/README:13: conflicts (as the WinSxS cache may override the lookup path from a private I think it's the GAC and the WInSxS folder (two different things AFAIU) -- they don't really have the power to "override", they're simply first in the lookup sequence, so they have the power intercept lookups for private assemblies. I'm fine with either wording that mentions this more or less...
lgtm
lgtm with anything reasonably sane regarding the generated files. gab's latest comments make me think that some of my suggestions were misguided. we should discuss in person to figure out if a followup CL is called for.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/53793002/1230001
Retried try job too often on linux_rel for step(s) cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, gpu_unittests, ipc_tests, jingle_unittests, media_unittests, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/53793002/1230001
Message was sent while issue was closed.
Change committed as 234795 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
