Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10160011:
Create VisualElementsManifest.xml from template -- install VisualElements in the version directory (Closed)
|
Created:
8 years, 8 months ago by gab Modified:
8 years, 7 months ago CC:
chromium-reviews, erikwright (departed), grt+watch_chromium.org, brettw-cc_chromium.org, robertshield Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description1) Generate VisualElementsManifest.xml at install time from a pre-defined template.
This allows us to put images in the version directory (i.e. a path that changes each istall; rather than needing the images beside chrome.exe).
This also allows us to localize the Display Name of Google Chrome. This however does not fix bug #123376 (we have the same issue here).
2) Fixes create_installer_archive.py to not create empty directories for output directories specified in chrome.release for which the input is not present when packaging chrome.7z
3) Removes "Dictionaries\en-US.bdic: %(ChromeDir)s\Dictionaries" from chrome.release which has not been packaged since Chrome 17 (and maybe even before that).
4) (2+3) have the side-effect that there is no longer an empty Dictionaries/ directory created at install.
BUG=123319, 123376
TEST=Install Chrome on Win8 and watch VisualElementsManifest.xml land with the correct values.
Overinstall Chrome on a Win8 install with VisualElementsManifest.xml from an installer that doesn't have VisualElements --> ensure VisualElementsManifest.xml is deleted and icons go back to default-style.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135615
Patch Set 1 #Patch Set 2 : remove shortname property #
Total comments: 5
Patch Set 3 : simpler -- hardcoded template + SStringPrintf #
Total comments: 2
Patch Set 4 : lintttttt #
Total comments: 43
Patch Set 5 : fix the world #Patch Set 6 : rewording #
Total comments: 33
Patch Set 7 : fix nits #Patch Set 8 : refactored the new code --> template logic in install_util.cc #
Total comments: 19
Patch Set 9 : i love semicolons #Patch Set 10 : better when it compiles... #
Total comments: 8
Patch Set 11 : address comments and move to public interface for testing #
Total comments: 6
Patch Set 12 : gg #Patch Set 13 : untested tests... #
Total comments: 28
Patch Set 14 : test and gyp fixes #Patch Set 15 : add comment #
Total comments: 14
Patch Set 16 : greg's the man :) #Patch Set 17 : typo typi typa #
Total comments: 8
Patch Set 18 : nits + remove conditional win8 #Patch Set 19 : no win8 conditional for move #Patch Set 20 : oops dont include robert's gyp changes... #Patch Set 21 : new line at end of install_unittest.cc #
Total comments: 2
Patch Set 22 : single \n at EOF #Messages
Total messages: 34 (0 generated)
PTAL.
Here is an example of the manifest that would be passed to this mechanism:
<Application
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
<VisualElements
DisplayName="$DISPLAYNAME$"
Logo="$LOGO$"
SmallLogo="$SMALLLOGO$"
ForegroundText="light"
BackgroundColor="white">
<DefaultTile
ShowName="allLogos"
/>
<SplashScreen
Image="$SPLASH$"/>
</VisualElements>
</Application>
https://chromiumcodereview.appspot.com/10160011/diff/2001/base/file_util.h
File base/file_util.h (right):
https://chromiumcodereview.appspot.com/10160011/diff/2001/base/file_util.h#ne...
base/file_util.h:120: // If |to_path| already exists, it is overwritten.
This is already the current behavior, but I needed to ensure this remains true
so I'm adding it to the interface's definition.
https://chromiumcodereview.appspot.com/10160011/diff/2001/chrome/installer/mi...
File chrome/installer/mini_installer/chrome.release (right):
https://chromiumcodereview.appspot.com/10160011/diff/2001/chrome/installer/mi...
chrome/installer/mini_installer/chrome.release:40: theme_resources_metro_1x.pak:
%(VersionDir)s\
Maybe we should move all metro bits under this new [METRO] entry... just noticed
it and haven't checked what it is enough to know yet.
There is still a weird error in SStringPrintf, but please take a look at the overall design/code while I figure it out :)! Cheers, Gab http://codereview.chromium.org/10160011/diff/5002/chrome/installer/setup/inst... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10160011/diff/5002/chrome/installer/setup/inst... chrome/installer/setup/install_worker.cc:75: if (!file_util::PathExists(src_path.Append(images_dir + kLogoImage))) { No need to use a conditional work item list here, it's there or it's not... I would have preferred to just use src_path.Append(images_dir), but it appears the Images/ directory is always created even if the images aren't in the build output when packaging. http://codereview.chromium.org/10160011/diff/5002/chrome/installer/setup/inst... chrome/installer/setup/install_worker.cc:99: base::SStringPrintf(&manifest, manifest_template, display_name, logo_path, Testing the installer now, this call fails silently (i.e. setup.exe stops there with no dump...) Still putting out the patch for global review while I figure this one out.
On Mon, Apr 30, 2012 at 9:59 PM, <gab@chromium.org> wrote: > http://codereview.chromium.**org/10160011/diff/5002/chrome/** > installer/setup/install_**worker.cc#newcode99<http://codereview.chromium.org/10160011/diff/5002/chrome/installer/setup/install_worker.cc#newcode99> > chrome/installer/setup/**install_worker.cc:99: > base::SStringPrintf(&manifest, manifest_template, display_name, > logo_path, > Testing the installer now, this call fails silently (i.e. setup.exe > stops there with no dump...) > Try: base::SStringPrintf(&manifest, manifest_template, display_name.c_str(), logo_path.c_str(), small_logo_path.c_str(), splash_path.c_str());
me like. please see comments. good night. http://codereview.chromium.org/10160011/diff/2001/chrome/installer/mini_insta... File chrome/installer/mini_installer/chrome.release (right): http://codereview.chromium.org/10160011/diff/2001/chrome/installer/mini_insta... chrome/installer/mini_installer/chrome.release:40: theme_resources_metro_1x.pak: %(VersionDir)s\ On 2012/04/27 01:40:21, gab wrote: > Maybe we should move all metro bits under this new [METRO] entry... just noticed > it and haven't checked what it is enough to know yet. Sailesh added it in r133843. i kinda sorta think these belong there, but it probably won't work until Joi lands https://chromiumcodereview.appspot.com/10262012/. i suppose it makes sense to leave them where they are for the moment, and move them after Joi's CL lands. you know, i bet that'd make the Images directory not appear when the elements aren't present... http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:12: #include <string> style: c++ headers follow c headers, so move this to just above <vector>. i like to separate the two groups with a newline, but i could be anti-style when i do so. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:61: void AddVisualElementsInstallWorkItems(const FilePath& src_path, i think this should be split up into two parts. here's what i envision: the first part should just write the file out and return true/false accordingly. i think this should live in install.cc and be called early in that file's InstallOrUpdateProduct (before the call to InstallNewVersion). it should be possible to write a unit test for this new function. is the manifest required for metro Chrome to work at all? if so, do we want to abort installation if we can't make the manifest? in this case, we should have a new InstallStatus constant in util_constants.h for "failed to generate visual elements manifest" or perhaps a more generic "failed to generate installable files" so we can see an error when this fails. we also want to know if the installer outright crashes or times out during this operation. for that, we need a corresponding InstallerStage constant and a call to installer_state.UpdateStage() in InstallOrUpdateProduct just before calling this new function. the second part should be more or less what was previously in AddInstallWorkItems: if the manifest is present in src_path, add a work item to move it into the right place. no magic there. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:68: string16 images_dir = ASCIIToUTF16(new_version.GetString()); i'm told that this is the encouraged style rather than using = string16 images_dir(ASCIIToUTF16(new_version.GetString())); please switch to that for everything other than the fundamental types (ints, pointers, etc) http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:76: LOG(WARNING) << "No visual elements found, skipping related work items."; is VLOG(1) more appropriate here, or is this really something worth warning us about? http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:78: const wchar_t manifest_template[] = i think it'd be helpful to put a comment above the template listing what the expected inputs are; e.g. (you might want to read my other comments first): // A printf_p-style format string for generating the visual elements manifest. // - localized display name for the product // - version number http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:78: const wchar_t manifest_template[] = - make this static to be sure that uses of it are pointing straight into the .rdata segment - use char (and ASCIIToUTF16 below) rather than wchar_t in the interest of saving bytes http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:80: L" xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\">\r\n" remove this namespace decl since it isn't used. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:82: L" DisplayName=\"%ls\"\r\n" since the attributes are conceptually a continuation of the opening of the element, i think it's consistent with Chromium style to do four-space indent here. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:82: L" DisplayName=\"%ls\"\r\n" i think the template will look cleaner with single-quotes around the attribute values rather than escaped double-quotes. for example: L" DisplayName='%ls'\r\n" http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:83: L" Logo=\"%ls\"\r\n" the bits underlying StringPrintf appear to support positional parameters. if this is true, i think something like this should work: L" Logo='%2$ls\\Images\\Logo.png'\r\n" L" SmallLogo='%2$ls\\Images\\SmallLogo.png'\r\n" ... L" <SplashScreen Image='%2$ls\\Images\\splash-620x300.png'/>\r\n" ... string16 manifest(base::StringPrintf(ASCIIToUTF16(manifest_template), display_name.c_str(), ASCIIToUTF16(new_version.GetString()).c_str()); http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:87: L" <DefaultTile ShowName=\"allLogos\"/>\r\n" the two-space indent is good here and on the next line. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:94: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); this will return the distribution of whatever product happens to be getting installed. in the land of multi-install, this is rarely what we want in the installer itself (since it could be installing Chrome, Chrome Frame, or both). for a Chrome Frame installation, I think it's a good idea to lay down the manifest in support of a future Chrome Frame -> Chrome quick-enable scenario. since the manifest only applies to Chrome, change this to: BrowserDistribution* dist = BrowserDistribution::GetSpecificDistribution( BrowserDistribution::CHROME_BROWSER); i can explain in person tomorrow if this doesn't make any sense. :-) http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:95: const string16 display_name = dist->GetAppShortCutName(); XML attribute values may not contain '<' or '&'. to be safe, this value should be run through a filter that converts all '&' into "&" and all '<' into "<". http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:99: base::SStringPrintf(&manifest, manifest_template, display_name, logo_path, is there a benefit to using SStringPrintf over StringPrintf? i think the latter is more clear since the first arg isn't an out param (which looks odd in Chromium code). http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:104: file_util::WriteFile(src_path.Append(kVisualElementsManifest), check the return value and, in case of error, log it and return false. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/set... File chrome/installer/setup/setup_constants.cc (right): http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/set... chrome/installer/setup/setup_constants.cc:1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. 2012 http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/set... File chrome/installer/setup/setup_constants.h (right): http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/set... chrome/installer/setup/setup_constants.h:1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. 2012
one more comment. please update the CL description, too. thanks. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:95: const string16 display_name = dist->GetAppShortCutName(); oh yeah: // TODO(grt): http://crbug.com/75152 Write a reference to a localized resource. unless you'd like it to be TODO(gab)!
Addressed mostly all comments, but the moving of the function (I wanted you to see the diff inline rather than having to compare with a new file yourself). I feel I should maybe put the Dictionaries+create_installer_archive fix in a separate CL, but this CL's behavior depends on it so if it were to be reverted this CL would break (i.e. it would always generate a manifest)... thus I kept them together for now. I'll address the code placement issue tomorrow, feel free to ignore this patch set (I just felt it would be easier for you to review the changes and the move in 2 distinct steps so I put it out now). Cheers, Gab https://chromiumcodereview.appspot.com/10160011/diff/2001/chrome/installer/mi... File chrome/installer/mini_installer/chrome.release (right): https://chromiumcodereview.appspot.com/10160011/diff/2001/chrome/installer/mi... chrome/installer/mini_installer/chrome.release:40: theme_resources_metro_1x.pak: %(VersionDir)s\ On 2012/05/01 04:17:03, grt wrote: > On 2012/04/27 01:40:21, gab wrote: > > Maybe we should move all metro bits under this new [METRO] entry... just > noticed > > it and haven't checked what it is enough to know yet. > > Sailesh added it in r133843. i kinda sorta think these belong there, but it > probably won't work until Joi lands > https://chromiumcodereview.appspot.com/10262012/. i suppose it makes sense to > leave them where they are for the moment, and move them after Joi's CL lands. > you know, i bet that'd make the Images directory not appear when the elements > aren't present... Right, I think http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/tools/build/win/cre... creates the directories. The question is whether the [METRO] section above will be skipped when metro elements are not present. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:12: #include <string> On 2012/05/01 04:17:03, grt wrote: > style: c++ headers follow c headers, so move this to just above <vector>. i > like to separate the two groups with a newline, but i could be anti-style when i > do so. I agree that a space makes it clearer (and I'm pretty sure I wouldn't have made the mistake to misplace the new include in the first place had they been separated originally). https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:68: string16 images_dir = ASCIIToUTF16(new_version.GetString()); On 2012/05/01 04:17:03, grt wrote: > i'm told that this is the encouraged style rather than using = > string16 images_dir(ASCIIToUTF16(new_version.GetString())); > please switch to that for everything other than the fundamental types (ints, > pointers, etc) Done. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:68: string16 images_dir = ASCIIToUTF16(new_version.GetString()); On 2012/05/01 04:17:03, grt wrote: > i'm told that this is the encouraged style rather than using = > string16 images_dir(ASCIIToUTF16(new_version.GetString())); > please switch to that for everything other than the fundamental types (ints, > pointers, etc) Done. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:76: LOG(WARNING) << "No visual elements found, skipping related work items."; On 2012/05/01 04:17:03, grt wrote: > is VLOG(1) more appropriate here, or is this really something worth warning us > about? Technically all distributions should have visual elements imo, but chrome.exe itself can work without them (so it's not an error if they're absent, but should be brought to the attention; as discussed verbose logging should be sufficient). https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:78: const wchar_t manifest_template[] = On 2012/05/01 04:17:03, grt wrote: > i think it'd be helpful to put a comment above the template listing what the > expected inputs are; e.g. (you might want to read my other comments first): > // A printf_p-style format string for generating the visual elements manifest. > // - localized display name for the product > // - version number Done. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:78: const wchar_t manifest_template[] = On 2012/05/01 04:17:03, grt wrote: > - make this static to be sure that uses of it are pointing straight into the > .rdata segment Done. > - use char (and ASCIIToUTF16 below) rather than wchar_t in the interest of > saving bytes Done. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:80: L" xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\">\r\n" On 2012/05/01 04:17:03, grt wrote: > remove this namespace decl since it isn't used. Done. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:82: L" DisplayName=\"%ls\"\r\n" On 2012/05/01 04:17:03, grt wrote: > since the attributes are conceptually a continuation of the opening of the > element, i think it's consistent with Chromium style to do four-space indent > here. Makes sense, done. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:82: L" DisplayName=\"%ls\"\r\n" On 2012/05/01 04:17:03, grt wrote: > i think the template will look cleaner with single-quotes around the attribute > values rather than escaped double-quotes. for example: > L" DisplayName='%ls'\r\n" Didn't know that XML didn't care, good to know, and I definitely agree :)! https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:87: L" <DefaultTile ShowName=\"allLogos\"/>\r\n" On 2012/05/01 04:17:03, grt wrote: > the two-space indent is good here and on the next line. Right. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:94: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); On 2012/05/01 04:17:03, grt wrote: > this will return the distribution of whatever product happens to be getting > installed. in the land of multi-install, this is rarely what we want in the > installer itself (since it could be installing Chrome, Chrome Frame, or both). > for a Chrome Frame installation, I think it's a good idea to lay down the > manifest in support of a future Chrome Frame -> Chrome quick-enable scenario. > since the manifest only applies to Chrome, change this to: > BrowserDistribution* dist = BrowserDistribution::GetSpecificDistribution( > BrowserDistribution::CHROME_BROWSER); > i can explain in person tomorrow if this doesn't make any sense. :-) Wouldn't that trigger Chromium to have display name "Google Chrome"...?! https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:95: const string16 display_name = dist->GetAppShortCutName(); On 2012/05/01 04:23:24, grt wrote: > oh yeah: > // TODO(grt): http://crbug.com/75152 Write a reference to a localized resource. > unless you'd like it to be TODO(gab)! Done. I think however fixes the localization issue across the installer should also fix this. Given all the TODOs are under your name for now, I'll also put this one under your name. Whoever ends up owning bug #75152 can take ownership of the TODOs. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:95: const string16 display_name = dist->GetAppShortCutName(); On 2012/05/01 04:17:03, grt wrote: > XML attribute values may not contain '<' or '&'. to be safe, this value should > be run through a filter that converts all '&' into "&" and all '<' into > "<". Done. Not sure where you found documentation saying there was no need to escape '>' in this case, but I'll take your word for it. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:99: base::SStringPrintf(&manifest, manifest_template, display_name, logo_path, On 2012/05/01 04:17:03, grt wrote: > is there a benefit to using SStringPrintf over StringPrintf? i think the latter > is more clear since the first arg isn't an out param (which looks odd in > Chromium code). Looks like I overlooked the code and assumed StringPrintf was just like printf (i.e. printing to stdout). I definitely also prefer StringPrintf (i.e. with no out param). https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:104: file_util::WriteFile(src_path.Append(kVisualElementsManifest), On 2012/05/01 04:17:03, grt wrote: > check the return value and, in case of error, log it and return false. Done. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... File chrome/installer/setup/setup_constants.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/setup_constants.cc:1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. On 2012/05/01 04:17:03, grt wrote: > 2012 Done. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... File chrome/installer/setup/setup_constants.h (right): https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/setup_constants.h:1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. On 2012/05/01 04:17:03, grt wrote: > 2012 Done.
On 2012/05/01 02:29:08, grt wrote: > On Mon, Apr 30, 2012 at 9:59 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > > > http://codereview.chromium.**org/10160011/diff/5002/chrome/** > > > installer/setup/install_**worker.cc#newcode99<http://codereview.chromium.org/10160011/diff/5002/chrome/installer/setup/install_worker.cc#newcode99> > > chrome/installer/setup/**install_worker.cc:99: > > base::SStringPrintf(&manifest, manifest_template, display_name, > > logo_path, > > Testing the installer now, this call fails silently (i.e. setup.exe > > stops there with no dump...) > > > > Try: > > base::SStringPrintf(&manifest, manifest_template, display_name.c_str(), > logo_path.c_str(), > small_logo_path.c_str(), splash_path.c_str()); Oh ya and this worked :)! Too bad I lost time trying to figure out windbg when this was the only issue... (also too bad the code is too bad to catch this at compile time :( )
Looks pretty good. Please add to the description the bit about the Dictionaries directory no longer being copied from the archive into the target dir. Also, please check the bug numbers in the description. One of them doesn't seem right. Thanks. https://chromiumcodereview.appspot.com/10160011/diff/2001/chrome/installer/mi... File chrome/installer/mini_installer/chrome.release (right): https://chromiumcodereview.appspot.com/10160011/diff/2001/chrome/installer/mi... chrome/installer/mini_installer/chrome.release:40: theme_resources_metro_1x.pak: %(VersionDir)s\ On 2012/05/01 22:19:02, gab wrote: > On 2012/05/01 04:17:03, grt wrote: > > On 2012/04/27 01:40:21, gab wrote: > > > Maybe we should move all metro bits under this new [METRO] entry... just > > noticed > > > it and haven't checked what it is enough to know yet. > > > > Sailesh added it in r133843. i kinda sorta think these belong there, but it > > probably won't work until Joi lands > > https://chromiumcodereview.appspot.com/10262012/. i suppose it makes sense to > > leave them where they are for the moment, and move them after Joi's CL lands. > > you know, i bet that'd make the Images directory not appear when the elements > > aren't present... > > Right, I think > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/tools/build/win/cre... > creates the directories. The question is whether the [METRO] section above will > be skipped when metro elements are not present. It's the intention that the [METRO] section is included when enable_metro=1 is set in the gyp configuration (although this is broken right now). I'm not sure if ultimately those things are really more touchy than metroy, though. I suggest leaving the visual elements the way you have them now. We can move them into the [METRO] section later if that seems appropriate. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:94: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); On 2012/05/01 22:19:02, gab wrote: > On 2012/05/01 04:17:03, grt wrote: > > this will return the distribution of whatever product happens to be getting > > installed. in the land of multi-install, this is rarely what we want in the > > installer itself (since it could be installing Chrome, Chrome Frame, or both). > > > for a Chrome Frame installation, I think it's a good idea to lay down the > > manifest in support of a future Chrome Frame -> Chrome quick-enable scenario. > > since the manifest only applies to Chrome, change this to: > > BrowserDistribution* dist = BrowserDistribution::GetSpecificDistribution( > > BrowserDistribution::CHROME_BROWSER); > > i can explain in person tomorrow if this doesn't make any sense. :-) > > Wouldn't that trigger Chromium to have display name "Google Chrome"...?! No. The base BrowserDistribution class (rather than the GoogleChromeBrowserDistribution class) will be selected in that case. https://chromiumcodereview.appspot.com/10160011/diff/10002/chrome/installer/s... chrome/installer/setup/install_worker.cc:95: const string16 display_name = dist->GetAppShortCutName(); On 2012/05/01 22:19:02, gab wrote: > On 2012/05/01 04:17:03, grt wrote: > > XML attribute values may not contain '<' or '&'. to be safe, this value > should > > be run through a filter that converts all '&' into "&" and all '<' into > > "<". > > Done. Not sure where you found documentation saying there was no need to escape > '>' in this case, but I'll take your word for it. http://www.w3.org/TR/2008/REC-xml-20081126/#NT-AttValue https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/m... File chrome/installer/mini_installer/chrome.release (right): https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/m... chrome/installer/mini_installer/chrome.release:52: splash-620x300.png: %(VersionDir)s\VisualElements nit: sort these and move them below PepperFlash so that the items in this section are ordered primarily by target directory (VisualElements comes after PepperFlash) and secondarily by filename. https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:64: // Construct the relative path to the Images directory. "Images" -> "versioned VisualElements" https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:65: string16 images_dir(ASCIIToUTF16(new_version.GetString())); images_dir -> elements_dir https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:77: // - Relative path to the Images directory. Images -> VisualElements https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:78: static const char manifest_template[] = manifest_template -> kManifestTemplate https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:92: const string16 manifest_template16 = ASCIIToUTF16(manifest_template); manifest_template(ASCIIToUTF16(kManifestTemplate)) https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:96: string16 display_name = dist->GetAppShortCutName(); display_name(dist->GetAppShortCutName()) https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:97: // XMLEscape |display_name| to be safe. We do not need to escape double // Escape the display name as per the XML AttValue production // (http://www.w3.org/TR/2008/REC-xml-20081126/#NT-AttValue) for a value in // single quotes. https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:100: ReplaceChars(display_name, L"&", string16(L"&"), &display_name); string16(L"&") -> L"&" https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:101: ReplaceChars(display_name, L"'", string16(L"&apos"), &display_name); string16(L"&apos") -> L"'" https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:102: ReplaceChars(display_name, L"<", string16(L"<"), &display_name); string16(L"<") -> L"<" https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:103: string16 manifest16 = base::StringPrintf(manifest_template16.c_str(), manifest16(base::StringPrintf(...)) https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:108: const std::string manifest = UTF16ToUTF8(manifest16); manifest(UTF... https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:111: LOG(ERROR) << "Error writing " << kVisualElementsManifest PLOG(ERROR)
there's always a straggler... https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/18002/chrome/installer/s... chrome/installer/setup/install_worker.cc:93: // TODO(grt): http://crbug.com/75152 Write a reference to a localized please move this comment down one line so that it immediately precedes the definition of display_name.
Comments addressed in patch set 7. CL refactored in patch set 8 to address code placement suggestions. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:61: void AddVisualElementsInstallWorkItems(const FilePath& src_path, On 2012/05/01 04:17:03, grt wrote: > i think this should be split up into two parts. here's what i envision: > > the first part should just write the file out and return true/false accordingly. > i think this should live in install.cc and be called early in that file's > InstallOrUpdateProduct (before the call to InstallNewVersion). it should be > possible to write a unit test for this new function. > Done. unit test to come in next patch set. > is the manifest required for metro Chrome to work at all? if so, do we want to > abort installation if we can't make the manifest? in this case, we should have > a new InstallStatus constant in util_constants.h for "failed to generate visual > elements manifest" or perhaps a more generic "failed to generate installable > files" so we can see an error when this fails. We do not need to abort if the manifest fails to be created (nor do we want to.. as discussed with Robert, it appears we don't stop when failing to create shortcuts either which is a similar case). The manifest not being present simply means we get the default icons instead of Metro tiles (as if we were non-default browser), but with the full functionality (including metro launch) of the default browser. > > we also want to know if the installer outright crashes or times out during this > operation. for that, we need a corresponding InstallerStage constant and a call > to installer_state.UpdateStage() in InstallOrUpdateProduct just before calling > this new function. > Done. Is it normal that there isn't an InstallerStage for InstallNewVersion()? I guess we are only adding work items in it, not executing them, but if it somehow fails the stage will be on CREATING_VISUAL_MANIFEST in the current code. > the second part should be more or less what was previously in > AddInstallWorkItems: if the manifest is present in src_path, add a work item to > move it into the right place. no magic there. Some magic actually, need to delete it if it's not present in the update (could be an error creating it in the archive_dir). Otherwise we keep an old manifest pointing to an old version dir bound to be deleted =S! http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:83: L" Logo=\"%ls\"\r\n" On 2012/05/01 04:17:03, grt wrote: > the bits underlying StringPrintf appear to support positional parameters. if > this is true, i think something like this should work: > L" Logo='%2$ls\\Images\\Logo.png'\r\n" > L" SmallLogo='%2$ls\\Images\\SmallLogo.png'\r\n" > ... > L" <SplashScreen Image='%2$ls\\Images\\splash-620x300.png'/>\r\n" > ... > string16 manifest(base::StringPrintf(ASCIIToUTF16(manifest_template), > display_name.c_str(), > ASCIIToUTF16(new_version.GetString()).c_str()); This works, done in previous patchset. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:94: BrowserDistribution* dist = BrowserDistribution::GetDistribution(); On 2012/05/02 15:09:43, grt wrote: > On 2012/05/01 22:19:02, gab wrote: > > On 2012/05/01 04:17:03, grt wrote: > > > this will return the distribution of whatever product happens to be getting > > > installed. in the land of multi-install, this is rarely what we want in the > > > installer itself (since it could be installing Chrome, Chrome Frame, or > both). > > > > > for a Chrome Frame installation, I think it's a good idea to lay down the > > > manifest in support of a future Chrome Frame -> Chrome quick-enable > scenario. > > > since the manifest only applies to Chrome, change this to: > > > BrowserDistribution* dist = > BrowserDistribution::GetSpecificDistribution( > > > BrowserDistribution::CHROME_BROWSER); > > > i can explain in person tomorrow if this doesn't make any sense. :-) > > > > Wouldn't that trigger Chromium to have display name "Google Chrome"...?! > > No. The base BrowserDistribution class (rather than the > GoogleChromeBrowserDistribution class) will be selected in that case. Ah, I see, CHROME_BROWSER != GOOGLE_CHROME :)! Done. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:95: const string16 display_name = dist->GetAppShortCutName(); On 2012/05/02 15:09:43, grt wrote: > On 2012/05/01 22:19:02, gab wrote: > > On 2012/05/01 04:17:03, grt wrote: > > > XML attribute values may not contain '<' or '&'. to be safe, this value > > should > > > be run through a filter that converts all '&' into "&" and all '<' into > > > "<". > > > > Done. Not sure where you found documentation saying there was no need to > escape > > '>' in this case, but I'll take your word for it. > > http://www.w3.org/TR/2008/REC-xml-20081126/#NT-AttValue Great, thanks, added it to the comment. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/mini_inst... File chrome/installer/mini_installer/chrome.release (right): http://codereview.chromium.org/10160011/diff/18002/chrome/installer/mini_inst... chrome/installer/mini_installer/chrome.release:52: splash-620x300.png: %(VersionDir)s\VisualElements On 2012/05/02 15:09:44, grt wrote: > nit: sort these and move them below PepperFlash so that the items in this > section are ordered primarily by target directory (VisualElements comes after > PepperFlash) and secondarily by filename. Right, that was the original idea with Images (which came before Pepperflash, forgot to move after rename. As for splash-620x300.png: I put it first because it's lowercase (as I've done in previous patches, see items in VersionDir above). http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:64: // Construct the relative path to the Images directory. On 2012/05/02 15:09:44, grt wrote: > "Images" -> "versioned VisualElements" Done. (I Know patch set 7 says "version" --> fixed for "versioned" in patch set 8). http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:65: string16 images_dir(ASCIIToUTF16(new_version.GetString())); On 2012/05/02 15:09:44, grt wrote: > images_dir -> elements_dir Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:77: // - Relative path to the Images directory. On 2012/05/02 15:09:44, grt wrote: > Images -> VisualElements Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:78: static const char manifest_template[] = On 2012/05/02 15:09:44, grt wrote: > manifest_template -> kManifestTemplate Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:92: const string16 manifest_template16 = ASCIIToUTF16(manifest_template); On 2012/05/02 15:09:44, grt wrote: > manifest_template(ASCIIToUTF16(kManifestTemplate)) Arg! I always forget about this style rule! http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:93: // TODO(grt): http://crbug.com/75152 Write a reference to a localized On 2012/05/02 15:13:09, grt wrote: > please move this comment down one line so that it immediately precedes the > definition of display_name. Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:96: string16 display_name = dist->GetAppShortCutName(); On 2012/05/02 15:09:44, grt wrote: > display_name(dist->GetAppShortCutName()) Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:97: // XMLEscape |display_name| to be safe. We do not need to escape double On 2012/05/02 15:09:44, grt wrote: > // Escape the display name as per the XML AttValue production > // (http://www.w3.org/TR/2008/REC-xml-20081126/#NT-AttValue) for a value in > // single quotes. I like. Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:100: ReplaceChars(display_name, L"&", string16(L"&"), &display_name); On 2012/05/02 15:09:44, grt wrote: > string16(L"&") -> L"&" I was under the impression that Chromium style didn't like implicit cast, but I guess that's only when making new classes we want to make sure they have explicit single-param constructors, but that we are allowed to use currently existing implicit casts? Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:101: ReplaceChars(display_name, L"'", string16(L"&apos"), &display_name); On 2012/05/02 15:09:44, grt wrote: > string16(L"&apos") -> L"'" Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:102: ReplaceChars(display_name, L"<", string16(L"<"), &display_name); On 2012/05/02 15:09:44, grt wrote: > string16(L"<") -> L"<" Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:103: string16 manifest16 = base::StringPrintf(manifest_template16.c_str(), On 2012/05/02 15:09:44, grt wrote: > manifest16(base::StringPrintf(...)) Done. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:108: const std::string manifest = UTF16ToUTF8(manifest16); On 2012/05/02 15:09:44, grt wrote: > manifest(UTF... Done :). http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:111: LOG(ERROR) << "Error writing " << kVisualElementsManifest On 2012/05/02 15:09:44, grt wrote: > PLOG(ERROR) Right, done.
looking really good. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:61: void AddVisualElementsInstallWorkItems(const FilePath& src_path, On 2012/05/02 20:55:58, gab wrote: > Done. Is it normal that there isn't an InstallerStage for InstallNewVersion()? it's inside InstallNewVersion. > Some magic actually, need to delete it if it's not present in the update (could > be an error creating it in the archive_dir). excellent point. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/mini_inst... File chrome/installer/mini_installer/chrome.release (right): http://codereview.chromium.org/10160011/diff/18002/chrome/installer/mini_inst... chrome/installer/mini_installer/chrome.release:52: splash-620x300.png: %(VersionDir)s\VisualElements On 2012/05/02 20:55:58, gab wrote: > As for splash-620x300.png: I put it first because it's lowercase (as I've done > in previous patches, see items in VersionDir above). is it normal for lower-case to come before upper when sorting? this seems backwards to me. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10160011/diff/18002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:100: ReplaceChars(display_name, L"&", string16(L"&"), &display_name); On 2012/05/02 20:55:58, gab wrote: > we are allowed to use currently existing implicit casts? yeah. iirc, the guidance is not to introduce implicit ctors unless there's a really compelling reason to do so. if there is one in a class you're using, i think you're free to use it. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/mini_inst... File chrome/installer/mini_installer/chrome.release (right): http://codereview.chromium.org/10160011/diff/28002/chrome/installer/mini_inst... chrome/installer/mini_installer/chrome.release:54: splash-620x300.png: %(VersionDir)s\VisualElements nit: all other entries have a trailing '\'. please add it to these at least for consistency's sake, if not for correctness. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:265: const Version& new_version) { nit: within the context of this function, i think just "version" is appropriate for this parameter. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:296: const string16 manifest_template16(ASCIIToUTF16(kManifestTemplate)); manifest_template16 -> manifest_template http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:306: ReplaceChars(display_name, L"&", L"&", &display_name); & -> & http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:307: ReplaceChars(display_name, L"'", L"&apos", &display_name); &apos -> ' http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:308: ReplaceChars(display_name, L"<", L"<", &display_name); < -> < http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:461: << ". Proceeding with the install anyways..."; the log message in CreateVisualElementsManifest is sufficient, no? http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (left): http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:826: // TODO(grt): Use AddMoveTreeWorkItem in a conditional WorkItemList, which thanks for getting rid of a TODO for me! http://codereview.chromium.org/10160011/diff/28002/chrome/installer/util/inst... File chrome/installer/util/install_util.cc (right): http://codereview.chromium.org/10160011/diff/28002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:55: const wchar_t kStageCreateVisualManifest[] = L"creating_visual_manifest"; please make the constant and its value the same (Create -> Creating) http://codereview.chromium.org/10160011/diff/28002/chrome/installer/util/util... File chrome/installer/util/util_constants.h (right): http://codereview.chromium.org/10160011/diff/28002/chrome/installer/util/util... chrome/installer/util/util_constants.h:116: CREATING_VISUAL_MANIFEST, // 17: Create VisualElementsManifest.xml Create -> Creating
Uploaded fix to comments while I'm writing tests. http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): http://codereview.chromium.org/10160011/diff/10002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:61: void AddVisualElementsInstallWorkItems(const FilePath& src_path, On 2012/05/03 17:49:36, grt wrote: > On 2012/05/02 20:55:58, gab wrote: > > Done. Is it normal that there isn't an InstallerStage for InstallNewVersion()? > > it's inside InstallNewVersion. Oh, how did I miss that...! I'll change CreateVisualElementsManifest to do this too. > > > Some magic actually, need to delete it if it's not present in the update > (could > > be an error creating it in the archive_dir). > > excellent point. http://codereview.chromium.org/10160011/diff/18002/chrome/installer/mini_inst... File chrome/installer/mini_installer/chrome.release (right): http://codereview.chromium.org/10160011/diff/18002/chrome/installer/mini_inst... chrome/installer/mini_installer/chrome.release:52: splash-620x300.png: %(VersionDir)s\VisualElements On 2012/05/03 17:49:37, grt wrote: > On 2012/05/02 20:55:58, gab wrote: > > As for splash-620x300.png: I put it first because it's lowercase (as I've done > > in previous patches, see items in VersionDir above). > > is it normal for lower-case to come before upper when sorting? this seems > backwards to me. Right, in ASCII upper-case is before lower-case. Fixed. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/mini_inst... File chrome/installer/mini_installer/chrome.release (right): http://codereview.chromium.org/10160011/diff/28002/chrome/installer/mini_inst... chrome/installer/mini_installer/chrome.release:54: splash-620x300.png: %(VersionDir)s\VisualElements On 2012/05/03 17:49:37, grt wrote: > nit: all other entries have a trailing '\'. please add it to these at least for > consistency's sake, if not for correctness. Done. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:265: const Version& new_version) { On 2012/05/03 17:49:37, grt wrote: > nit: within the context of this function, i think just "version" is appropriate > for this parameter. Done. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:296: const string16 manifest_template16(ASCIIToUTF16(kManifestTemplate)); On 2012/05/03 17:49:37, grt wrote: > manifest_template16 -> manifest_template Done. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:306: ReplaceChars(display_name, L"&", L"&", &display_name); On 2012/05/03 17:49:37, grt wrote: > & -> & Done. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:307: ReplaceChars(display_name, L"'", L"&apos", &display_name); On 2012/05/03 17:49:37, grt wrote: > &apos -> ' Done. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:308: ReplaceChars(display_name, L"<", L"<", &display_name); On 2012/05/03 17:49:37, grt wrote: > < -> < Done. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:461: << ". Proceeding with the install anyways..."; On 2012/05/03 17:49:37, grt wrote: > the log message in CreateVisualElementsManifest is sufficient, no? I debated that. If we say the log is sufficient in CreateVisualElementsManifest then that function does not need to return bool. Having a log here shows how we handled the error reported by CreateVisualElementsManifest. We could potentially decide in the future that we want to stop the install if the function fails in which case we still want the error log inside CreateVisualElementsManifest, but need to change this part of the code. i.e. this separates concerns: CreateVisualElementsManifest => reports error writing file InstallOrUpdateProduct => reports how it proceeded given the error Let me know if you still think I should proceed otherwise (and thus probably make CreateVisualElementsManifest return void).
lookin' good http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:461: << ". Proceeding with the install anyways..."; On 2012/05/03 19:03:48, gab wrote: > I debated that. If we say the log is sufficient in CreateVisualElementsManifest > then that function does not need to return bool. It doesn't need to return bool in the current use, but it's a nice interface. > Having a log here shows how we handled the error reported by CreateVisualElementsManifest. I argue that it adds nothing but noise to the log file. The previous line would have detailed error information, and the next line would make it clear that the installation was proceeding anyway. That aside, anyone reading the log can (and will) consult the code. The log is meant to be used in conjunction with the code to diagnose problems. > We could potentially decide in the future that we want to stop the install if > the function fails in which case we still want the error log inside > CreateVisualElementsManifest, but need to change this part of the code. > > i.e. this separates concerns: > CreateVisualElementsManifest => reports error writing file > InstallOrUpdateProduct => reports how it proceeded given the error > > Let me know if you still think I should proceed otherwise (and thus probably > make CreateVisualElementsManifest return void). I say leave it as-is and remove the logging here. The func returning bool is a good interface. That doesn't mean that the caller must take action based on the return value. http://codereview.chromium.org/10160011/diff/39002/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/39002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:268: installer_state.UpdateStage(installer::CREATING_VISUAL_MANIFEST); i think you should move this back to where it was. i don't think a helper function like this one should be in the business of doing the stage update. and you won't want the stage stuff to happen in a unit test. http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/inst... File chrome/installer/util/install_util.cc (right): http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:55: const wchar_t kStageCreateVisualManifest[] = L"creating_visual_manifest"; Create -> Creating http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:75: kStageCreateVisualManifest, Create -> Creating http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/util... File chrome/installer/util/util_constants.h (right): http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/util... chrome/installer/util/util_constants.h:116: CREATING_VISUAL_MANIFEST, // 17: Create VisualElementsManifest.xml Create -> Creating
Addressed comments and made CreateVisualElementsManifest public for testing. http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/28002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:461: << ". Proceeding with the install anyways..."; On 2012/05/03 19:37:58, grt wrote: > On 2012/05/03 19:03:48, gab wrote: > > I debated that. If we say the log is sufficient in > CreateVisualElementsManifest > > then that function does not need to return bool. > > It doesn't need to return bool in the current use, but it's a nice interface. > > > Having a log here shows how we handled the error reported by > CreateVisualElementsManifest. > > I argue that it adds nothing but noise to the log file. The previous line would > have detailed error information, and the next line would make it clear that the > installation was proceeding anyway. That aside, anyone reading the log can (and > will) consult the code. The log is meant to be used in conjunction with the > code to diagnose problems. > > > We could potentially decide in the future that we want to stop the install if > > the function fails in which case we still want the error log inside > > CreateVisualElementsManifest, but need to change this part of the code. > > > > i.e. this separates concerns: > > CreateVisualElementsManifest => reports error writing file > > InstallOrUpdateProduct => reports how it proceeded given the error > > > > Let me know if you still think I should proceed otherwise (and thus probably > > make CreateVisualElementsManifest return void). > > I say leave it as-is and remove the logging here. The func returning bool is a > good interface. That doesn't mean that the caller must take action based on the > return value. Done. http://codereview.chromium.org/10160011/diff/39002/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/39002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:268: installer_state.UpdateStage(installer::CREATING_VISUAL_MANIFEST); > you won't want the stage stuff to happen in a unit test. I just realized this writing the test :). Done. http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/inst... File chrome/installer/util/install_util.cc (right): http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:55: const wchar_t kStageCreateVisualManifest[] = L"creating_visual_manifest"; On 2012/05/03 19:37:58, grt wrote: > Create -> Creating Done. http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/inst... chrome/installer/util/install_util.cc:75: kStageCreateVisualManifest, On 2012/05/03 19:37:58, grt wrote: > Create -> Creating Done. http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/util... File chrome/installer/util/util_constants.h (right): http://codereview.chromium.org/10160011/diff/39002/chrome/installer/util/util... chrome/installer/util/util_constants.h:116: CREATING_VISUAL_MANIFEST, // 17: Create VisualElementsManifest.xml On 2012/05/03 19:37:58, grt wrote: > Create -> Creating Done.
snip snip http://codereview.chromium.org/10160011/diff/38011/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/38011/chrome/installer/setup/ins... chrome/installer/setup/install.cc:356: remove empty line http://codereview.chromium.org/10160011/diff/38011/chrome/installer/setup/ins... File chrome/installer/setup/install.h (right): http://codereview.chromium.org/10160011/diff/38011/chrome/installer/setup/ins... chrome/installer/setup/install.h:14: #include "base/file_path.h" remove this (there's a forward decl on line 19) http://codereview.chromium.org/10160011/diff/38011/chrome/installer/setup/ins... chrome/installer/setup/install.h:29: // If |installer_state| is not NULL, updates the stage to remove this part of the comment
Please update the reference to bug 123376 in the text of the CL description (it's right in the BUG line).
On 2012/05/03 20:27:34, grt wrote: > Please update the reference to bug 123376 in the text of the CL description > (it's right in the BUG line). Done.
I added some tests now. I have yet to test the tests as I'm having crazy build issues... 1) Forced to move to chrome.sln to build setup_unittests => Implies rebuild of a bunch of stuff because of: http://crbug.com/119528 2) Building setup_unittests: => installer\setup\install.cc(40): fatal error C1083: Cannot open include file: 'registered_dlls.h': No such file or directory 3) Played with gyp to figure it out... failed... tried to build all of "setup" project to trigger this to be built... it didn't happen... (or at least setup didn't fail to build, but building setup_unittests after still fails). 4) Trying a build all of chrome.sln to see if something else triggers this dependency to be built. 5) Ran into my "build world" issue a couple times in all of this... 6) I'm about to go nuts... I HATE BUILDSSSSSSSSSSS! Thanks for reviewing! Gab https://chromiumcodereview.appspot.com/10160011/diff/38011/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/38011/chrome/installer/s... chrome/installer/setup/install.cc:356: On 2012/05/03 20:01:50, grt wrote: > remove empty line Done. https://chromiumcodereview.appspot.com/10160011/diff/38011/chrome/installer/s... File chrome/installer/setup/install.h (right): https://chromiumcodereview.appspot.com/10160011/diff/38011/chrome/installer/s... chrome/installer/setup/install.h:14: #include "base/file_path.h" On 2012/05/03 20:01:50, grt wrote: > remove this (there's a forward decl on line 19) Ah kk, I'll put it back in the .cc then. https://chromiumcodereview.appspot.com/10160011/diff/38011/chrome/installer/s... chrome/installer/setup/install.h:29: // If |installer_state| is not NULL, updates the stage to On 2012/05/03 20:01:50, grt wrote: > remove this part of the comment Indeed..
On 2012/05/04 00:04:11, gab wrote: > I added some tests now. I have yet to test the tests as I'm having crazy build > issues... > > 1) Forced to move to chrome.sln to build setup_unittests > => Implies rebuild of a bunch of stuff because of: http://crbug.com/119528 > > 2) Building setup_unittests: > => installer\setup\install.cc(40): fatal error C1083: Cannot open include > file: 'registered_dlls.h': No such file or directory > > 3) Played with gyp to figure it out... failed... tried to build all of "setup" > project to trigger this to be built... it didn't happen... (or at least setup > didn't fail to build, but building setup_unittests after still fails). > > 4) Trying a build all of chrome.sln to see if something else triggers this > dependency to be built. > > 5) Ran into my "build world" issue a couple times in all of this... > > 6) I'm about to go nuts... I HATE BUILDSSSSSSSSSSS! Oh... and now that I'm building the world... my IDE is frozen... so I can't see if I've made stupid mistakes... feel free to postpone your review... but I put it out there in hopes of having one review cycle done when my build issues are out of the way! Cheers, Gab
looks good. i think the test can be simplified quite a bit; see below. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:393: // Escape the display name as per the XML AttValue production would you extract lines 393-398 into a helper function that can be tested independently (and write a test)? http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... File chrome/installer/setup/install_unittest.cc (right): http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:19: virtual void SetUp() { SetUp() OVERRIDE { http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:24: virtual void TearDown() { TearDown() OVERRIDE { http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:31: void InitVisualElementsTest(bool create_elements_folder) { move everything in this function through line 43 into SetUp. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:33: ASSERT_TRUE(file_util::PathExists(work_dir_)); there's no need to check this since the assert on line 21 assures that this will never be false. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:38: ASSERT_FALSE(file_util::PathExists(version_dir_)); there's no need to check for this since test_dir_ is always created from scratch. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:40: ASSERT_TRUE(file_util::PathExists(version_dir_)); there's no need to check for this since we assume CreateDirectory has been tested elsewhere. if a lack of a version_dir_ is enough to make the test fail outright, change the EXPECT on line 39 to an ASSERT. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:43: ASSERT_FALSE(file_util::PathExists(elements_dir_)); remove this, too. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:44: if (create_elements_folder) { move directory creation into VisualElementsManifestCreated http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:51: FilePath work_dir_; remove this and use test_dir_.path() where needed http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:62: private: i was motivated by a reviewer long ago to make everything in tests protected. maybe some other TEST_F will be added that needs access to test_dir_. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:83: const FilePath manifest_path( since this path is used by both tests, i think it makes sense to make a "manifest_path_" member and initialize it in SetUp http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:88: file_util::ReadFileToString(manifest_path, &read_manifest); ASSERT_TRUE? http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:99: " <SplashScreen Image='0.0.0.0\\VisualElements\\splash-620x300.png'/>" does this wrap more nicely if you break it just after SplashScreen?
Thanks Greg for your test wisdom. Thanks Robert for your gyp wisdom. Discussed the registered_dlls.h issue with robert and it turns out copying the rule (which I had tried yesterday) was not sufficient... it is only triggered when a .release file is part of the input... so I added that to.... there already is a TODO to make the common sections between setup and setup_unittests in one common lib, added this to the TODO. (oh and robert also said we probably want to remove registered_dlls.h, but not as part of this CL) Yay! Gab http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install.cc:393: // Escape the display name as per the XML AttValue production On 2012/05/04 13:19:14, grt wrote: > would you extract lines 393-398 into a helper function that can be tested > independently (and write a test)? Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... File chrome/installer/setup/install_unittest.cc (right): http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:19: virtual void SetUp() { On 2012/05/04 13:19:14, grt wrote: > SetUp() OVERRIDE { Interesting, I copied the format from setup_util_unittest.cc... guess it's wrong :)! http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:24: virtual void TearDown() { On 2012/05/04 13:19:14, grt wrote: > TearDown() OVERRIDE { Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:31: void InitVisualElementsTest(bool create_elements_folder) { On 2012/05/04 13:19:14, grt wrote: > move everything in this function through line 43 into SetUp. Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:33: ASSERT_TRUE(file_util::PathExists(work_dir_)); On 2012/05/04 13:19:14, grt wrote: > there's no need to check this since the assert on line 21 assures that this will > never be false. Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:38: ASSERT_FALSE(file_util::PathExists(version_dir_)); On 2012/05/04 13:19:14, grt wrote: > there's no need to check for this since test_dir_ is always created from > scratch. Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:40: ASSERT_TRUE(file_util::PathExists(version_dir_)); On 2012/05/04 13:19:14, grt wrote: > there's no need to check for this since we assume CreateDirectory has been > tested elsewhere. if a lack of a version_dir_ is enough to make the test fail > outright, change the EXPECT on line 39 to an ASSERT. Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:43: ASSERT_FALSE(file_util::PathExists(elements_dir_)); On 2012/05/04 13:19:14, grt wrote: > remove this, too. Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:44: if (create_elements_folder) { On 2012/05/04 13:19:14, grt wrote: > move directory creation into VisualElementsManifestCreated Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:51: FilePath work_dir_; On 2012/05/04 13:19:14, grt wrote: > remove this and use test_dir_.path() where needed Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:62: private: On 2012/05/04 13:19:14, grt wrote: > i was motivated by a reviewer long ago to make everything in tests protected. > maybe some other TEST_F will be added that needs access to test_dir_. Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:83: const FilePath manifest_path( On 2012/05/04 13:19:14, grt wrote: > since this path is used by both tests, i think it makes sense to make a > "manifest_path_" member and initialize it in SetUp Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:88: file_util::ReadFileToString(manifest_path, &read_manifest); On 2012/05/04 13:19:14, grt wrote: > ASSERT_TRUE? Done. http://codereview.chromium.org/10160011/diff/49002/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:99: " <SplashScreen Image='0.0.0.0\\VisualElements\\splash-620x300.png'/>" On 2012/05/04 13:19:14, grt wrote: > does this wrap more nicely if you break it just after SplashScreen? Ya I guess, wasn't sure where to break it...
[+cc robertshield] Note: I also changed the title to reflect the new state of this CL.
lg http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... File chrome/installer/setup/install.h (right): http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install.h:29: void EscapeXmlAttributeValueInSingleQuotes(string16& att_value); pass by pointer since att_value is modified. http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... File chrome/installer/setup/install_unittest.cc (right): http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:84: "Image='0.0.0.0\\VisualElements\\splash-620x300.png'/>\r\n" maybe indent this four spaces? http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:91: void EscapeXmlAttributeValueTestHelper(const string16& expected_escaped_val, i think this helper function makes it harder to read the tests. remove it? http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:99: static const char kExpectedEscapedVal[] = you could use wchar_t here since binary size isn't an issue http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:100: "This has 'crazy' \"chars\" && < and > signs."; four-space indent
See below, weekend commits are great :). http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... File chrome/installer/setup/install.h (right): http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/05/04 21:22:42, grt wrote: > 2012 Done. http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install.h:29: void EscapeXmlAttributeValueInSingleQuotes(string16& att_value); On 2012/05/04 21:22:42, grt wrote: > pass by pointer since att_value is modified. Done. http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... File chrome/installer/setup/install_unittest.cc (right): http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:84: "Image='0.0.0.0\\VisualElements\\splash-620x300.png'/>\r\n" On 2012/05/04 21:22:42, grt wrote: > maybe indent this four spaces? Hmmm this is already 4 spaces... unless you mean with respect to <SplashScreen. I can't indent the actual string inside otherwise it won't match obviously. http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:91: void EscapeXmlAttributeValueTestHelper(const string16& expected_escaped_val, On 2012/05/04 21:22:42, grt wrote: > i think this helper function makes it harder to read the tests. remove it? Done. http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:99: static const char kExpectedEscapedVal[] = On 2012/05/04 21:22:42, grt wrote: > you could use wchar_t here since binary size isn't an issue Done. http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:100: "This has 'crazy' \"chars\" && < and > signs."; On 2012/05/04 21:22:42, grt wrote: > four-space indent Done.
final nits (i hope ;-) ) https://chromiumcodereview.appspot.com/10160011/diff/58004/chrome/installer/s... File chrome/installer/setup/install_unittest.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/58004/chrome/installer/s... chrome/installer/setup/install_unittest.cc:84: "Image='0.0.0.0\\VisualElements\\splash-620x300.png'/>\r\n" On 2012/05/04 23:02:09, gab wrote: > On 2012/05/04 21:22:42, grt wrote: > > maybe indent this four spaces? > > Hmmm this is already 4 spaces... unless you mean with respect to <SplashScreen. > > I can't indent the actual string inside otherwise it won't match obviously. this is a tough one. aesthetically, i think the word "Image" would look good four spaces in from the '<' of the line above. on the other hand, all of these lines are part of the same literal. looking at them purely from that standpoint, there's no difference between the line above and this one. what do you think of this: " <SplashScreen Image='0.0.0.0\\VisualElements\\splash-620x300.png'/>" "\r\n" https://chromiumcodereview.appspot.com/10160011/diff/54005/chrome/installer/s... File chrome/installer/setup/install.h (right): https://chromiumcodereview.appspot.com/10160011/diff/54005/chrome/installer/s... chrome/installer/setup/install.h:26: // Escape the display name as per the XML AttValue production sorry i missed this before: please replace "the display name" with "|att_value|". https://chromiumcodereview.appspot.com/10160011/diff/54005/chrome/installer/s... File chrome/installer/setup/install_unittest.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/54005/chrome/installer/s... chrome/installer/setup/install_unittest.cc:75: "<Application>\r\n" oops. i missed this before, too: indent two more spaces, please
Looks awesome, just one question from me pending Sir Greg's final approval: http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... chrome/installer/setup/install.cc:448: // when calling InstallNewVersion() below. Considering the OS upgrade path, should we not do this on all Windows versions?
See comments below, testing robert's suggestion now. http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... File chrome/installer/setup/install_unittest.cc (right): http://codereview.chromium.org/10160011/diff/58004/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:84: "Image='0.0.0.0\\VisualElements\\splash-620x300.png'/>\r\n" On 2012/05/05 02:16:59, grt wrote: > On 2012/05/04 23:02:09, gab wrote: > > On 2012/05/04 21:22:42, grt wrote: > > > maybe indent this four spaces? > > > > Hmmm this is already 4 spaces... unless you mean with respect to > <SplashScreen. > > > > I can't indent the actual string inside otherwise it won't match obviously. > > this is a tough one. aesthetically, i think the word "Image" would look good > four spaces in from the '<' of the line above. on the other hand, all of these > lines are part of the same literal. looking at them purely from that > standpoint, there's no difference between the line above and this one. what do > you think of this: > > " <SplashScreen Image='0.0.0.0\\VisualElements\\splash-620x300.png'/>" > "\r\n" Me likee :). Done. http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... chrome/installer/setup/install.cc:448: // when calling InstallNewVersion() below. On 2012/05/05 05:13:20, robertshield wrote: > Considering the OS upgrade path, should we not do this on all Windows versions? I agree, I don't think having the VisualElementsManifest.xml around will have any side-effects on Win7 (ref: as registering the DelegateExecute verb handler did). Furthermore, it's easier to register a couple things on Win7-->Win8 upgrade then it is to change files in the install. Having all the files already present is a plus. I'll test: 1) That it doesn't affect Win7 2) That it's fine on Win8 if we are default and all Win8 files are present, but registration is not yet done for DelegateExecute verb handler (i.e. just after Win7-->Win8 upgrade). http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... File chrome/installer/setup/install.h (right): http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... chrome/installer/setup/install.h:26: // Escape the display name as per the XML AttValue production On 2012/05/05 02:16:59, grt wrote: > sorry i missed this before: please replace "the display name" with > "|att_value|". Done. http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... File chrome/installer/setup/install_unittest.cc (right): http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:75: "<Application>\r\n" On 2012/05/05 02:16:59, grt wrote: > oops. i missed this before, too: indent two more spaces, please Done. http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... chrome/installer/setup/install_unittest.cc:75: "<Application>\r\n" On 2012/05/05 02:16:59, grt wrote: > oops. i missed this before, too: indent two more spaces, please Done.
Testing done (see below) => no need to conditional on Win8 :). Ignore patchset 19 I mistakenly committed Robert's gyp changes from src-internal which leave a dirty git tree (i.e. git commit -a takes them in...)... we'll have to find a way to fix this I think, but in the mean time please have a look at patch set 20! Cheers, Gab http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): http://codereview.chromium.org/10160011/diff/54005/chrome/installer/setup/ins... chrome/installer/setup/install.cc:448: // when calling InstallNewVersion() below. On 2012/05/05 16:19:51, gab wrote: > On 2012/05/05 05:13:20, robertshield wrote: > > Considering the OS upgrade path, should we not do this on all Windows > versions? > > I agree, I don't think having the VisualElementsManifest.xml around will have > any side-effects on Win7 (ref: as registering the DelegateExecute verb handler > did). > > Furthermore, it's easier to register a couple things on Win7-->Win8 upgrade then > it is to change files in the install. Having all the files already present is a > plus. > > I'll test: > 1) That it doesn't affect Win7 Having the VisualElementsManifest.xml present on Win7 installs has no effect as expected and desired. > 2) That it's fine on Win8 if we are default and all Win8 files are present, but > registration is not yet done for DelegateExecute verb handler (i.e. just after > Win7-->Win8 upgrade). This works too. Having the VisualElementsManifest.xml on Win8 when we are the default browser but have yet to register a DelegateExecute verb handler makes it so that we get our nice Metro tile and nice Chrome icon in Metro search, but clicking those opens Chrome desktop as expected.
LGTM!!!111!18 (with one question/nit) https://chromiumcodereview.appspot.com/10160011/diff/60006/chrome/installer/s... File chrome/installer/setup/install_unittest.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/60006/chrome/installer/s... chrome/installer/setup/install_unittest.cc:105: it looks like there's a blank line at the end of this file (i say "looks" because sometimes rietveld lies). please check that there isn't one in the real file (the file should end with the newline for line 104). thanks.
lgtm
lgtm++
https://chromiumcodereview.appspot.com/10160011/diff/60006/chrome/installer/s... File chrome/installer/setup/install_unittest.cc (right): https://chromiumcodereview.appspot.com/10160011/diff/60006/chrome/installer/s... chrome/installer/setup/install_unittest.cc:105: On 2012/05/07 01:37:32, grt wrote: > it looks like there's a blank line at the end of this file (i say "looks" > because sometimes rietveld lies). please check that there isn't one in the real > file (the file should end with the newline for line 104). thanks. Right, I got confused by the bad lint error in rietveld in patch set 20, thanks for clarification over chat!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/10160011/62005
Change committed as 135615 |
