Suffix Chrome's appid on user-level installs

Suffix Chrome's appid on user-level installs

ShellIntegration::GetAppId() --> ShellIntegration::GetProfileAppId() for clarity (i.e. having two functions called "GetAppId" is confusing imo).

Patch From Gabriel Charette <gab@chromium.org>;
BUG=125362, 133173
TEST=Make sure 125362 doesn't repro.
http://goo.gl/ZZ7gE
ShellIntegrationTest.GetAppModelIdForProfileTest
ShellUtilTest.BuildAppModelId*
SessionRestoreTest.RestoreAfterClosingTabbedBrowserWithAppAndLaunching
AppModeTest.EnableAppModeTest

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=142909

[gab, 2012-06-06 18:08:09]

Hey greg, so this is the last of the series for http://goo.gl/jxDSU :)!

Right now this is the situation:
ChromeHTML+Appname suffix
          |
          |
          |
Modify logic for appname suffixing
          |\
          | \
          |  \
          |   HKCU for Win8
          |
          This patch (suffix appid)

Where:
ChromeHTML+Appname suffix => http://codereview.chromium.org/10451074/

Modify logic for appname suffixing =>
http://codereview.chromium.org/10446111/#ps11001

HKCU for Win8 => http://codereview.chromium.org/10399054/

I have updated all patches to use a diff from their parent in the above tree.

Thank you very much for you time and effort reviewing this (+521, -343 lines!)!

I'm testing all of this together now and we can ask QA (?) to test all scenarios
when they are checked in (which I hope is soon so it's not too close to the next
dev-channel release to be safe).

Cheers,
Gab

[gab, 2012-06-13 20:22:07]

I have now based this CL on top of the HKCU CL.

This will make sure that we never have a non-suffix to suffix promotion on Win8
(as either you have the full unsuffixed install and we don't promote you; or you
only have ProgId unsuffixed in which case the HKCU patch will fully install
Chrome, suffixed, in HKCU).

 Right now this is the situation:
 ChromeHTML+Appname suffix
           |
           |
           |
 Modify logic for appname suffixing
           |
           | 
           |  
      HKCU for Win8
           |
           |
           |
  This patch (suffix appid)
 
> Where:
> ChromeHTML+Appname suffix => http://codereview.chromium.org/10451074/
> 
> Modify logic for appname suffixing =>
> http://codereview.chromium.org/10446111/#ps11001
> 
> HKCU for Win8 => http://codereview.chromium.org/10399054/


More changes to come to fix the fact that right now we are doing I/O on the UI
thread with this CL...

[gab, 2012-06-14 16:06:21]

Ready for review!

The final design decision was to always suffix the appid (which prevents
problems where the cached appid suffix could change in some scenarios).

It also means we no longer need to read the registry to know the current
installation's suffix and can just assume it's always suffixed :).

The only thing we still need to know is the path to chrome.exe to determine if
this is a user or system-level install (which aren't suffixed).

It turns out PathService already caches this value, so that we can get
chrome_exe without any I/O (provided it's already cached, but it seems to be and
other functions in shell_integration_win.cc seem to use PathService as well so I
guess it's safe...?), this call no longer requiring I/O, caching the suffix is
no longer necessary!

Cheers,
Gab

[grt (UTC plus 2), 2012-06-15 03:03:03]

looks good.

robert: please give us your thoughts on my comment in install.cc
carlos: please give us your thoughts on my comment in browser.cc

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/she...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/she...
chrome/browser/shell_integration.h:126: // Generates Win7 app id for given app
name and profile path. The returned app
"Generates an application user model ID (AppUserModelID) for a given..."

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/she...
chrome/browser/shell_integration.h:132: static string16 GetProfileAppId(const
string16& app_name,
"Profile" in the name of this function is misleading.  the point of this
function is to return a user model id that is specific to some app (not
necessarily chrome/chromium) and a profile.  i suggest changing this method to
GetAppModelId and the one below to GetChromiumModelId.  or something like that.

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/she...
chrome/browser/shell_integration.h:135: // Generates Win7 app id for Chromium by
calling GetProfileAppId with
"Generates an application user model ID (AppUserModelID) for Chromium..."

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/she...
File chrome/browser/shell_integration_win.cc (right):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/she...
chrome/browser/shell_integration_win.cc:565: // App id should be less than 128
chars.
this has shrunk in Win8: "The AppUserModelID may not be longer than 64
characters."  please update accordingly.

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/she...
chrome/browser/shell_integration_win.cc:566: DCHECK(app_id.length() < 128);
DCHECK_LT

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/she...
chrome/browser/shell_integration_win.cc:610: LOG(DFATAL) << "PathService::Get
failed.";
NOTREACHED();

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/ui/...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/ui/...
chrome/browser/ui/browser.cc:535: ui::win::SetAppIdForWindow(
the "developing a metro style enabled desktop browser" doc says "When running in
the Metro style experience, this AppUserModelID is immutable by the running
browser process." and "Metro style enabled desktop browsers should not set an
AppUserModelID at runtime when participating in the Metro style user experience"
Which makes me wonder if it's bad to ever do this when in metro mode.

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/ui/...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/browser/ui/...
chrome/browser/ui/views/extensions/shell_window_views.cc:208:
profile->GetPath()),
indent to align profile with UTF8 above

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
File chrome/installer/setup/install.cc (left):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
chrome/installer/setup/install.cc:472:
prefs.GetBool(master_preferences::kDoNotCreateShortcuts,
robert added this in http://crrev.com/25870.  it explicitly prevented
registration.  i'm not familiar with how it's used in practice, but this CL is a
significant departure from existing behavior.  robert: can you shed some light
on why this preference was added and under what circumstances it's used?

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
File chrome/installer/setup/install.cc (right):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
chrome/installer/setup/install.cc:94: const installer::InstallStatus&
install_status,
why pass a value of an enum by const ref?

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1078: string16 app_id =
dist->GetBaseAppId();
ctor-style rather than assignment-style

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1084: return app_id + suffix;
return app_id += suffix or app_id.append(suffix) to avoid the risk of an
unnecessary temporary (which the optimizer may remove, but why chance it)

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1448: DCHECK(QuickIsChromeRegistered(dist,
please explain why the shortcut can't be created before registration.  will the
shortcut simply not work at all otherwise?

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1453: const string16 chrome_path =
FilePath(chrome_exe).DirName().value();
while you're here, would you mind tightening up this func a bit with the
suggested changes below?
first change:
const FilePath chrome_path(FilePath(chrome_exe).DirName());

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1455: FilePath prefs_path(chrome_path);
second change:
FilePath prefs_path(chrome_path.AppendASCII(installer::kDefaultMasterPrefs))
(and delete line 1456)

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1468: chrome_path.c_str(),
third change:
chrome_path.value().c_str(),

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
File chrome/installer/util/shell_util.h (right):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
chrome/installer/util/shell_util.h:245: // Returns the appid of the program
under |dist|.
How about:
// Returns the AppUserModelID for |dist|. This identifier is unconditionally
suffixed with the user id for user-level installs (in contrast to other
registration entries which are suffixed as described in
GetCurrentInstallationSuffix above).

I like using the MS term for the thing here so that the reader (who will
understandably wonder what the heck this is for) can Google for it and be
educated by MSDN.

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
chrome/installer/util/shell_util.h:246: // This appid will *always* be suffixed
for user-level installs.
please avoid *bold* in comments (blink tag is okay if you can get it to work,
though)

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/u...
chrome/installer/util/shell_util.h:389: // This method should only be called
after Chrome is registered.
why?  because of the DCHECK, or is there a fundamental reason why registration
must be done before the shortcut is created?

[robertshield, 2012-06-15 03:27:59]

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
File chrome/installer/setup/install.cc (left):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
chrome/installer/setup/install.cc:472:
prefs.GetBool(master_preferences::kDoNotCreateShortcuts,
On 2012/06/15 03:03:03, grt wrote:
> robert added this in http://crrev.com/25870.  it explicitly prevented
> registration.  i'm not familiar with how it's used in practice, but this CL is
a
> significant departure from existing behavior.  robert: can you shed some light
> on why this preference was added and under what circumstances it's used?

This was added long ago for Chrome Frame, so that installing CF would not create
shortcuts to desktop Chrome. At one point, this argument was implied by the
presence of --chrome-frame, rendering the explicit command line flag obsolete,
but it looks like it never got scrubbed from our Omaha configs and still today,
this argument is passed to CF installers that users run, both stub and MSIs.

Chrome MSI and stub installers never get this, but I don't know whether any OEM
deals pass this explicitly for Chrome.

Several refactorings later, this appears to be the sole remaining usage and I
believe that a standalone CF install would be unaffected by this change due to
the "if (chrome_install)" check. 

Barring any OEM deals passing this explicitly for Chrome installations, making
this change is fine, but please try running a CF install, as well as a multi
CF+Chrome install to verify.

[grt (UTC plus 2), 2012-06-15 03:33:51]

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
File chrome/installer/setup/install.cc (left):

https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
chrome/installer/setup/install.cc:472:
prefs.GetBool(master_preferences::kDoNotCreateShortcuts,
On 2012/06/15 03:27:59, robertshield wrote:
> On 2012/06/15 03:03:03, grt wrote:
> > robert added this in http://crrev.com/25870.  it explicitly prevented
> > registration.  i'm not familiar with how it's used in practice, but this CL
is
> a
> > significant departure from existing behavior.  robert: can you shed some
light
> > on why this preference was added and under what circumstances it's used?
> 
> This was added long ago for Chrome Frame, so that installing CF would not
create
> shortcuts to desktop Chrome. At one point, this argument was implied by the
> presence of --chrome-frame, rendering the explicit command line flag obsolete,
> but it looks like it never got scrubbed from our Omaha configs and still
today,
> this argument is passed to CF installers that users run, both stub and MSIs.
> 
> Chrome MSI and stub installers never get this, but I don't know whether any
OEM
> deals pass this explicitly for Chrome.

Do you know who we should ask about that?

> Several refactorings later, this appears to be the sole remaining usage and I
> believe that a standalone CF install would be unaffected by this change due to
> the "if (chrome_install)" check. 
> 
> Barring any OEM deals passing this explicitly for Chrome installations, making
> this change is fine, but please try running a CF install, as well as a multi
> CF+Chrome install to verify. 

How about just removing all traces of kDoNotCreateShortcuts from the code?  It
wouldn't break any vestigial uses of the option unless there are deals that use
it for Chrome.

[robertshield, 2012-06-15 04:25:26]

On 2012/06/15 03:33:51, grt wrote:
>
https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
> File chrome/installer/setup/install.cc (left):
> 
>
https://chromiumcodereview.appspot.com/10542031/diff/11003/chrome/installer/s...
> chrome/installer/setup/install.cc:472:
> prefs.GetBool(master_preferences::kDoNotCreateShortcuts,
> On 2012/06/15 03:27:59, robertshield wrote:
> > On 2012/06/15 03:03:03, grt wrote:
> > > robert added this in http://crrev.com/25870.  it explicitly prevented
> > > registration.  i'm not familiar with how it's used in practice, but this
CL
> is
> > a
> > > significant departure from existing behavior.  robert: can you shed some
> light
> > > on why this preference was added and under what circumstances it's used?
> > 
> > This was added long ago for Chrome Frame, so that installing CF would not
> create
> > shortcuts to desktop Chrome. At one point, this argument was implied by the
> > presence of --chrome-frame, rendering the explicit command line flag
obsolete,
> > but it looks like it never got scrubbed from our Omaha configs and still
> today,
> > this argument is passed to CF installers that users run, both stub and MSIs.
> > 
> > Chrome MSI and stub installers never get this, but I don't know whether any
> OEM
> > deals pass this explicitly for Chrome.
> 
> Do you know who we should ask about that?

Let's start with Glenn. 

> 
> > Several refactorings later, this appears to be the sole remaining usage and
I
> > believe that a standalone CF install would be unaffected by this change due
to
> > the "if (chrome_install)" check. 
> > 
> > Barring any OEM deals passing this explicitly for Chrome installations,
making
> > this change is fine, but please try running a CF install, as well as a multi
> > CF+Chrome install to verify. 
> 
> How about just removing all traces of kDoNotCreateShortcuts from the code?  It
> wouldn't break any vestigial uses of the option unless there are deals that
use
> it for Chrome.

If we can determine that no one uses it then yes, by all means.

[gab, 2012-06-15 19:01:04]

Comments addressed, see below.

Cheers,
Gab

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/shell_integ...
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/shell_integ...
chrome/browser/shell_integration.h:126: // Generates Win7 app id for given app
name and profile path. The returned app
On 2012/06/15 03:03:03, grt wrote:
> "Generates an application user model ID (AppUserModelID) for a given..."

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/shell_integ...
chrome/browser/shell_integration.h:132: static string16 GetProfileAppId(const
string16& app_name,
On 2012/06/15 03:03:03, grt wrote:
> "Profile" in the name of this function is misleading.  the point of this
> function is to return a user model id that is specific to some app (not
> necessarily chrome/chromium) and a profile.  i suggest changing this method to
> GetAppModelId and the one below to GetChromiumModelId.  or something like
that.

I like :).

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/shell_integ...
chrome/browser/shell_integration.h:135: // Generates Win7 app id for Chromium by
calling GetProfileAppId with
On 2012/06/15 03:03:03, grt wrote:
> "Generates an application user model ID (AppUserModelID) for Chromium..."

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/shell_integ...
File chrome/browser/shell_integration_win.cc (right):

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/shell_integ...
chrome/browser/shell_integration_win.cc:565: // App id should be less than 128
chars.
On 2012/06/15 03:03:03, grt wrote:
> this has shrunk in Win8: "The AppUserModelID may not be longer than 64
> characters."  please update accordingly.

Good catch, msdn still says 128, but the Metro guidelines say 64, might as well
stick to the strictiest guideline.

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/shell_integ...
chrome/browser/shell_integration_win.cc:566: DCHECK(app_id.length() < 128);
On 2012/06/15 03:03:03, grt wrote:
> DCHECK_LT

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/shell_integ...
chrome/browser/shell_integration_win.cc:610: LOG(DFATAL) << "PathService::Get
failed.";
On 2012/06/15 03:03:03, grt wrote:
> NOTREACHED();

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:535: ui::win::SetAppIdForWindow(
On 2012/06/15 03:03:03, grt wrote:
> the "developing a metro style enabled desktop browser" doc says "When running
in
> the Metro style experience, this AppUserModelID is immutable by the running
> browser process." and "Metro style enabled desktop browsers should not set an
> AppUserModelID at runtime when participating in the Metro style user
experience"
> Which makes me wonder if it's bad to ever do this when in metro mode.

Would you be ok if I just log a bug for this for now? I'd rather not get
involved too far with this patch given the critical timing. (i.e. the fact that
its suffixed or not doesn't affect the comment made here)

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:208:
profile->GetPath()),
On 2012/06/15 03:03:03, grt wrote:
> indent to align profile with UTF8 above

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (left):

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:472:
prefs.GetBool(master_preferences::kDoNotCreateShortcuts,
On 2012/06/15 03:33:51, grt wrote:
> On 2012/06/15 03:27:59, robertshield wrote:
> > On 2012/06/15 03:03:03, grt wrote:
> > > robert added this in http://crrev.com/25870.  it explicitly prevented
> > > registration.  i'm not familiar with how it's used in practice, but this
CL
> is
> > a
> > > significant departure from existing behavior.  robert: can you shed some
> light
> > > on why this preference was added and under what circumstances it's used?
> > 
> > This was added long ago for Chrome Frame, so that installing CF would not
> create
> > shortcuts to desktop Chrome. At one point, this argument was implied by the
> > presence of --chrome-frame, rendering the explicit command line flag
obsolete,
> > but it looks like it never got scrubbed from our Omaha configs and still
> today,
> > this argument is passed to CF installers that users run, both stub and MSIs.
> > 
> > Chrome MSI and stub installers never get this, but I don't know whether any
> OEM
> > deals pass this explicitly for Chrome.
> 
> Do you know who we should ask about that?
> 
> > Several refactorings later, this appears to be the sole remaining usage and
I
> > believe that a standalone CF install would be unaffected by this change due
to
> > the "if (chrome_install)" check. 
> > 
> > Barring any OEM deals passing this explicitly for Chrome installations,
making
> > this change is fine, but please try running a CF install, as well as a multi
> > CF+Chrome install to verify. 
> 
> How about just removing all traces of kDoNotCreateShortcuts from the code?  It
> wouldn't break any vestigial uses of the option unless there are deals that
use
> it for Chrome.

This sounds like its wrong/unused, we should probably remove it in another
patch, but I don't need to do this here anymore and thus won't be changing this
in this CL.

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:94: const installer::InstallStatus&
install_status,
On 2012/06/15 03:03:03, grt wrote:
> why pass a value of an enum by const ref?

Good question, I somehow thought it was an Object and remember having a reason
wanting this call not to modify its state... my bad!

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1078: string16 app_id =
dist->GetBaseAppId();
On 2012/06/15 03:03:03, grt wrote:
> ctor-style rather than assignment-style

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1084: return app_id + suffix;
On 2012/06/15 03:03:03, grt wrote:
> return app_id += suffix or app_id.append(suffix) to avoid the risk of an
> unnecessary temporary (which the optimizer may remove, but why chance it)

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1448: DCHECK(QuickIsChromeRegistered(dist,
On 2012/06/15 03:03:03, grt wrote:
> please explain why the shortcut can't be created before registration.  will
the
> shortcut simply not work at all otherwise?

See comment on shell_util.h

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1453: const string16 chrome_path =
FilePath(chrome_exe).DirName().value();
On 2012/06/15 03:03:03, grt wrote:
> while you're here, would you mind tightening up this func a bit with the
> suggested changes below?
> first change:
> const FilePath chrome_path(FilePath(chrome_exe).DirName());

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1455: FilePath prefs_path(chrome_path);
On 2012/06/15 03:03:03, grt wrote:
> second change:
> FilePath prefs_path(chrome_path.AppendASCII(installer::kDefaultMasterPrefs))
> (and delete line 1456)

Even better, remove prefs_path altogether, inlining it below.

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1468: chrome_path.c_str(),
On 2012/06/15 03:03:03, grt wrote:
> third change:
> chrome_path.value().c_str(),

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:245: // Returns the appid of the program
under |dist|.
On 2012/06/15 03:03:03, grt wrote:
> How about:
> // Returns the AppUserModelID for |dist|. This identifier is unconditionally
> suffixed with the user id for user-level installs (in contrast to other
> registration entries which are suffixed as described in
> GetCurrentInstallationSuffix above).
> 
> I like using the MS term for the thing here so that the reader (who will
> understandably wonder what the heck this is for) can Google for it and be
> educated by MSDN.

Done.

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:246: // This appid will *always* be suffixed
for user-level installs.
On 2012/06/15 03:03:03, grt wrote:
> please avoid *bold* in comments (blink tag is okay if you can get it to work,
> though)

<blink>Done.</blink>

http://codereview.chromium.org/10542031/diff/11003/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:389: // This method should only be called
after Chrome is registered.
On 2012/06/15 03:03:03, grt wrote:
> why?  because of the DCHECK, or is there a fundamental reason why registration
> must be done before the shortcut is created?

This was before we decided to always suffix the appid (as we needed to know what
ended being the suffix registered as we have to hardcode it in the shortcut).

Now that we always suffix, this is a non-issue.

[grt (UTC plus 2), 2012-06-15 19:24:07]

lgtm w/ a nit and a question.

please check with carlos about setting the id on windows in metro mode.

http://codereview.chromium.org/10542031/diff/17001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10542031/diff/17001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:538: profile_->GetPath()) :
nit: indentation (does it fit on the previous line now?)

http://codereview.chromium.org/10542031/diff/17001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/10542031/diff/17001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:208:
profile->GetPath()),
does this fit on the line above now?

[gab, 2012-06-15 19:29:59]

Done.

http://codereview.chromium.org/10542031/diff/17001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10542031/diff/17001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:538: profile_->GetPath()) :
On 2012/06/15 19:24:07, grt wrote:
> nit: indentation (does it fit on the previous line now?)

Done (no doesn't fit above).

http://codereview.chromium.org/10542031/diff/17001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/10542031/diff/17001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:208:
profile->GetPath()),
On 2012/06/15 19:24:07, grt wrote:
> does this fit on the line above now?

Yep, 80 chars right on, good eye :)!

[gab, 2012-06-15 19:31:21]

sky: OWNER approval for chrome\browser\ui side-effects...

Thanks,
Gab

[gab, 2012-06-15 20:28:24]

Made a separate CL to address the comment below.

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10542031/diff/11003/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:535: ui::win::SetAppIdForWindow(
On 2012/06/15 19:01:04, gab wrote:
> On 2012/06/15 03:03:03, grt wrote:
> > the "developing a metro style enabled desktop browser" doc says "When
running
> in
> > the Metro style experience, this AppUserModelID is immutable by the running
> > browser process." and "Metro style enabled desktop browsers should not set
an
> > AppUserModelID at runtime when participating in the Metro style user
> experience"
> > Which makes me wonder if it's bad to ever do this when in metro mode.
> 
> Would you be ok if I just log a bug for this for now? I'd rather not get
> involved too far with this patch given the critical timing. (i.e. the fact
that
> its suffixed or not doesn't affect the comment made here)

https://chromiumcodereview.appspot.com/10557017/

[sky, 2012-06-15 21:23:09]

I'm not familiar enough with this code to do the review justice.

http://codereview.chromium.org/10542031/diff/27001/chrome/browser/ui/web_appl...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

http://codereview.chromium.org/10542031/diff/27001/chrome/browser/ui/web_appl...
chrome/browser/ui/web_applications/web_app_ui.cc:12: #include "base/string16.h"
sort

[gab, 2012-06-15 21:29:25]

Done.

http://codereview.chromium.org/10542031/diff/27001/chrome/browser/ui/web_appl...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

http://codereview.chromium.org/10542031/diff/27001/chrome/browser/ui/web_appl...
chrome/browser/ui/web_applications/web_app_ui.cc:12: #include "base/string16.h"
On 2012/06/15 21:23:09, sky wrote:
> sort

Done.

[sky, 2012-06-15 21:30:59]

chrome/browser/ui LGTM

[gab, 2012-06-15 21:32:12]

@grt: awaiting your final LGTM (given the first one is getting old)

Cheers,
Gab

[gab, 2012-06-16 00:56:07]

On 2012/06/15 21:32:12, gab wrote:
> @grt: awaiting your final LGTM (given the first one is getting old)

my bad greg, its not this lgtm im looking for you to confirm, but the one on the
HKCU patch.

> 
> Cheers,
> Gab

[gab, 2012-06-18 03:15:25]

Fixed compilation issues with the shell integration tests (see comment in the
shell_integration_unittests.cc).

http://codereview.chromium.org/10542031/diff/21008/chrome/browser/shell_integ...
File chrome/browser/shell_integration_unittest.cc (right):

http://codereview.chromium.org/10542031/diff/21008/chrome/browser/shell_integ...
chrome/browser/shell_integration_unittest.cc:368: TEST(ShellIntegrationTest,
GetAppModelIdTest) {
All of the logic tested below was actually the logic of
ShellIntegration::GetAppModelId().

ShellIntegration::GetChromiumModelId() is just a shortcut that first gets
Chrome's appid and then calls GetAppModelId().

It didn't really matter which one was tested before, but now that we are
sometimes suffixing Chrome's appid, GetChromiumModelId() involves PathService to
find the path to chrome.exe (which will not work in a test...).

Instead of doing anything complex to mock PathService, etc. it is much easier to
test GetAppModelId() straight up, passing in the unsuffixed app id.

[gab, 2012-06-18 03:20:48]

On 2012/06/18 03:15:25, gab wrote:
> Fixed compilation issues with the shell integration tests (see comment in the
> shell_integration_unittests.cc).

I want to commit this tonight so that it goes together with the other CLs on any
build that might be triggered tomorrow (for QA, canary, or wtv), so I'll go
ahead and launch the CQ, there is no fundamental change in the test for now
besides shortcuting ShellUtil::GetAppId() by testing
ShellIntegration::GetAppModelId() straight up instead.

If you want me to do it differently we can do this in a subsequent CL.

Cheers,
Gab



> 
>
http://codereview.chromium.org/10542031/diff/21008/chrome/browser/shell_integ...
> File chrome/browser/shell_integration_unittest.cc (right):
> 
>
http://codereview.chromium.org/10542031/diff/21008/chrome/browser/shell_integ...
> chrome/browser/shell_integration_unittest.cc:368: TEST(ShellIntegrationTest,
> GetAppModelIdTest) {
> All of the logic tested below was actually the logic of
> ShellIntegration::GetAppModelId().
> 
> ShellIntegration::GetChromiumModelId() is just a shortcut that first gets
> Chrome's appid and then calls GetAppModelId().
> 
> It didn't really matter which one was tested before, but now that we are
> sometimes suffixing Chrome's appid, GetChromiumModelId() involves PathService
to
> find the path to chrome.exe (which will not work in a test...).
> 
> Instead of doing anything complex to mock PathService, etc. it is much easier
to
> test GetAppModelId() straight up, passing in the unsuffixed app id.

[commit-bot: I haz the power, 2012-06-18 03:21:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10542031/21008

[commit-bot: I haz the power, 2012-06-18 05:54:53]

Try job failure for 10542031-21008 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-18 12:36:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10542031/21008

[grt (UTC plus 2), 2012-06-18 13:02:53]

still lgtm (good choice with regards to the unittest)

[commit-bot: I haz the power, 2012-06-18 14:34:20]

Try job failure for 10542031-21008 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[gab, 2012-06-18 16:01:21]

Fixed browser tests: under some circumstances with scoped_temp_dir the appid
generated would sometimes be longer than the maximum 64 chars allowed.

This also prevents bugs for users with usernames in the 50+ chars...

I don't like that the elision code is imported from ui/... most eliding
functions are based on fonts and pixel_width so they belong there, but the one
used here (ElideString) is based solely on character count and probably doesn't
belong in ui/ (i.e. probably goes in base/)... however refactoring this is
beyond the scope of this CL imo (especially given time constraints).

Let me know what you think.

Cheers,
Gab

[gab, 2012-06-18 18:57:25]

After a discussion offline, we decided it would be better if all
truncating/compositing logic for appid was in the same place, this patch set(14)
introduces ShellUtil::ConstructAppModelId() for that.

I also renamed all appid functions as it was getting confusing... we had
GetAppId, GetChromiumModelId, GetAppModelId.

Hopefully you also like the new names :)!

Cheers,
Gab

[gab, 2012-06-18 18:58:46]

On 2012/06/18 18:57:25, gab wrote:
> After a discussion offline, we decided it would be better if all
> truncating/compositing logic for appid unnecessary same place, this patch
set(14)
> introduces ShellUtil::ConstructAppModelId() for that.

Oh, also, I'm writing a test for this function as of this writing, but am
looking for initial review in parallel to speed things up and avoid unnecesseary
work if there is something you want me to do completely differently...

> 
> I also renamed all appid functions as it was getting confusing... we had
> GetAppId, GetChromiumModelId, GetAppModelId.
> 
> Hopefully you also like the new names :)!
> 
> Cheers,
> Gab

[gab, 2012-06-18 19:44:53]

On 2012/06/18 18:58:46, gab wrote:
> On 2012/06/18 18:57:25, gab wrote:
> > After a discussion offline, we decided it would be better if all
> > truncating/compositing logic for appid unnecessary same place, this patch
> set(14)
> > introduces ShellUtil::ConstructAppModelId() for that.
> 
> Oh, also, I'm writing a test for this function as of this writing, but am
> looking for initial review in parallel to speed things up and avoid
unnecesseary
> work if there is something you want me to do completely differently...

Done (patch set 15). I hardcoded the expected string results as opposed to using
logic to derive them in the test code as I figured as I wrote the original logic
myself I would have to partially rewrite the logic in the test and if I did
something wrong in the code I'm likely to repeat my mistake in the test (and
have the test pass when it shouldn't)... hopefully that's ok with you.

> 
> > 
> > I also renamed all appid functions as it was getting confusing... we had
> > GetAppId, GetChromiumModelId, GetAppModelId.
> > 
> > Hopefully you also like the new names :)!
> > 
> > Cheers,
> > Gab

[grt (UTC plus 2), 2012-06-18 19:45:07]

i liked the old method names in ShellIntegration better.  i don't see a benefit
to the longer names.  let's discuss the pros/cons if you feel strongly.

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/common/chro...
File chrome/common/chrome_constants.h (right):

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/common/chro...
chrome/common/chrome_constants.h:143: extern const size_t kMaxAppModelIdLength;
move this into util_constants.{cc,h} and make it an int.

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1088: std::vector<string16> components;
you can construct and populate the vector in one fell swoop with:
std::vector<string16> components(1, app_id.append(suffix));

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1102: // below breaks.
can you add something to the non-debug case to not cause too much breakage?

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1105: string16 app_id;
just after this line, add:
app_id.reserve(chrome::kMaxAppModelIdLength);
to avoid reallocations in the loop.

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1113: // Append a truncated version of this
component. Truncate in the middle
"to truncate" means to chop off the end.  i think you're safe using "to shorten"
instead, although maybe there's an even nicer verb.  in any case, please update
this comment and the one in the header accordingly.

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1116: app_id.append(component.substr(0,
max_component_length / 2));
app_id.append(component, 0, max_component_length / 2);

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/installer/u...
chrome/installer/util/shell_util.cc:1117: app_id.append(
app_id.append(component, component.length() - ((max_component_length + 1) / 2),
string16::npos);

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/installer/u...
File chrome/installer/util/shell_util.h (right):

https://chromiumcodereview.appspot.com/10542031/diff/26003/chrome/installer/u...
chrome/installer/util/shell_util.h:249: static string16
GetChromiumModelId(BrowserDistribution* dist,
Hmm.  I like GetAppId better, since |dist| might not represent a web browser
named Chromium.

[grt (UTC plus 2), 2012-06-18 19:59:33]

On 2012/06/18 19:45:07, grt wrote:
> i liked the old method names in ShellIntegration better.  i don't see a
benefit
> to the longer names.  let's discuss the pros/cons if you feel strongly.

discussed offline.  gab is very persuasive.  the new names are okay by me.

[gab, 2012-06-18 21:52:42]

Comments addressed. I added the unit tests that are relevant to this to the CL
description.

I'll do some installer tests locally to make sure I didn't screw anything over
and then check the CQ box (feel free to cancel it if you review this in time and
see something obviously wrong... otherwise we can patch up later).

Cheers,
Gab

http://codereview.chromium.org/10542031/diff/26003/chrome/common/chrome_const...
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/10542031/diff/26003/chrome/common/chrome_const...
chrome/common/chrome_constants.h:143: extern const size_t kMaxAppModelIdLength;
On 2012/06/18 19:45:08, grt wrote:
> move this into util_constants.{cc,h} and make it an int.

Moved.

As discussed offline, leaving it as size_t to avoid ugly typecasting all over
the code that uses it.

http://codereview.chromium.org/10542031/diff/26003/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10542031/diff/26003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1088: std::vector<string16> components;
On 2012/06/18 19:45:08, grt wrote:
> you can construct and populate the vector in one fell swoop with:
> std::vector<string16> components(1, app_id.append(suffix));

Nice! I'd looked for (and obviously didn't find) a constructor that would take
just a string16, but that'll do perfect :)!

http://codereview.chromium.org/10542031/diff/26003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1102: // below breaks.
On 2012/06/18 19:45:08, grt wrote:
> can you add something to the non-debug case to not cause too much breakage?

Done.

http://codereview.chromium.org/10542031/diff/26003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1105: string16 app_id;
On 2012/06/18 19:45:08, grt wrote:
> just after this line, add:
> app_id.reserve(chrome::kMaxAppModelIdLength);
> to avoid reallocations in the loop.

Done.

http://codereview.chromium.org/10542031/diff/26003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1113: // Append a truncated version of this
component. Truncate in the middle
On 2012/06/18 19:45:08, grt wrote:
> "to truncate" means to chop off the end.  i think you're safe using "to
shorten"
> instead, although maybe there's an even nicer verb.  in any case, please
update
> this comment and the one in the header accordingly.

http://www.thefreedictionary.com/truncate says "shorten" is a synonym for
"truncate", but I really don't care so..

Done :)

http://codereview.chromium.org/10542031/diff/26003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1116: app_id.append(component.substr(0,
max_component_length / 2));
On 2012/06/18 19:45:08, grt wrote:
> app_id.append(component, 0, max_component_length / 2);

Done.

http://codereview.chromium.org/10542031/diff/26003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1117: app_id.append(
On 2012/06/18 19:45:08, grt wrote:
> app_id.append(component, component.length() - ((max_component_length + 1) /
2),
> string16::npos);

Done.

http://codereview.chromium.org/10542031/diff/26003/chrome/installer/util/shel...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10542031/diff/26003/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:249: static string16
GetChromiumModelId(BrowserDistribution* dist,
On 2012/06/18 19:45:08, grt wrote:
> Hmm.  I like GetAppId better, since |dist| might not represent a web browser
> named Chromium.

I did this to reflect the call sites in ShellIntegration; also given this takes
|chrome_exe| I think |dist| should only be Chrome, Chromium, or one of its other
browser variants (i.e. not Chrome Frame, etc.)

On that note I agree this is not necessarily "Chromium" so I'll change it to
GetBrowserModelId.. ok?

http://codereview.chromium.org/10542031/diff/35001/chrome/installer/util/shel...
File chrome/installer/util/shell_util_unittest.cc (right):

http://codereview.chromium.org/10542031/diff/35001/chrome/installer/util/shel...
chrome/installer/util/shell_util_unittest.cc:404:
ASSERT_EQ(L"Chrome.a_user_wer_64_characters.A_crazy_profilethat_is_possible",
The shortened version of this is actually pretty unintentionally funny :)!

[commit-bot: I haz the power, 2012-06-18 22:29:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10542031/35001

[grt (UTC plus 2), 2012-06-18 22:48:21]

lgtm w/ a cursory look.

https://chromiumcodereview.appspot.com/10542031/diff/35001/chrome/installer/u...
File chrome/installer/util/shell_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10542031/diff/35001/chrome/installer/u...
chrome/installer/util/shell_util_unittest.cc:404:
ASSERT_EQ(L"Chrome.a_user_wer_64_characters.A_crazy_profilethat_is_possible",
On 2012/06/18 21:52:43, gab wrote:
> The shortened version of this is actually pretty unintentionally funny :)!

nice!

[commit-bot: I haz the power, 2012-06-19 03:06:51]

Try job failure for 10542031-35001 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, steps "chrome_frame_net_tests, unit_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-19 03:12:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10542031/35001