Set prop app id for chromium/application shortcut.

Set prop app id for chromium/application shortcut.

This is a follow up change after andrew's patch for win7 shortcut to
properly set app id for chromium/application shortcut.
- Move PKEY_AppUserModel_ID and code to set it from app/win_util.cc to
  base/win_util.cc as SetAppIdForPropertyStore to share with file_util
  shortcut code;
- Add an app_id args to file_util's CreateShortcutLink and
  UpdateShortcutLink;
- Update code that calls the above two function in installer and
  UserDataManager so that the chromium shortcuts are created with proper
  app id (except the uninstall shortcut which is not tagged with any app
  id).
- Move ComputeApplicationNameFromURL from Browser to web_app namespace
  and use it as app id for application shortcut. This makes pinned
  shortcut and browser window use the same app id and Win7 correctly
  groups them;

- Rename ComputeApplicationNameFromURL to GenerateApplicationNameFromURL
  per Ben's comments;

- Add a DCHECK in SetAppIdForPropertyStore to ensure app id is less than
  128 chars and contains no space per msdn;
- Change default app id from IDS_PRODUCT_NAME to chrome::kBrowserAppName

BUG=28104
TEST=On Win7, pinned shortcut should no longer be separated from running instance of chrome for both chrome and web application.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32508

[Ben Goodger (Google), 2009-11-17 23:15:23]

I'll let brg/hbono drive the review here, I just have one terminology comment:

http://codereview.chromium.org/399045/diff/1/12
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/399045/diff/1/12#newcode17
Line 17: std::wstring ComputeApplicationNameFromURL(const GURL& url);
I prefer "Generate" to Compute.

[xiyuan, 2009-11-18 00:35:56]

Wait for brg and hbono's comments now.

http://codereview.chromium.org/399045/diff/1/12
File chrome/browser/web_applications/web_app.h (right):

http://codereview.chromium.org/399045/diff/1/12#newcode17
Line 17: std::wstring ComputeApplicationNameFromURL(const GURL& url);
On 2009/11/17 23:15:23, Ben Goodger wrote:
> I prefer "Generate" to Compute.

Done.

[brg, 2009-11-18 00:39:19]

LGTM

Thanks for cleaning up the COM usage.

[Hironori Bono, 2009-11-18 02:08:18]

Thank you for your hard work.
Even though this change mostly looks good, is it possible to fix some style
issues?

By the way, a MSDN document (*1) defines the format of an App ID as listed
below.

> An application must provide its AppUserModelID in the following form.
> It can have no more than 128 characters and cannot contain spaces.
> Each section should be camel-cased.
>
> CompanyName.ProductName.SubProduct.VersionInformation
>
> CompanyName and ProductName should always be used, while the
> SubProduct and VersionInformation portions are optional and
> depend on the application's requirements.

Maybe we will change our App ID string to make it compliant with this document.
(I forgot changing it for a long time. Sorry.)

(*1) <http://msdn.microsoft.com/en-us/library/dd378459(VS.85).aspx>

Regards,

Hironori Bono

http://codereview.chromium.org/399045/diff/1/5
File base/file_util_win.cc (right):

http://codereview.chromium.org/399045/diff/1/5#newcode297
Line 297: ScopedComPtr<IShellLink> i_shell_link;
nit: ResolveShortCut() uses 'shell' for a ScopedComPtr<IShellLink> variable. On
the other hand, this function uses 'i_shell_link' for a ScopedComPtr<IShellLink>
variable. I personally think it is better to use the same name for both
variables from the point of consistency.

http://codereview.chromium.org/399045/diff/1/5#newcode347
Line 347: CLSCTX_INPROC_SERVER)))
nit: it is better for this line to be aligned with CLSID_ShellLink.

http://codereview.chromium.org/399045/diff/1/5#newcode372
Line 372: if (app_id && (win_util::GetWinVersion() >=
win_util::WINVERSION_WIN7)) {
nit: internal braces are redundant.

http://codereview.chromium.org/399045/diff/1/5#newcode373
Line 373: ScopedComPtr<IPropertyStore> pps;
nit: I prefer using descriptive name (e.g. 'property_store') to an acronym
'pps'.

http://codereview.chromium.org/399045/diff/1/6
File base/win_util.cc (right):

http://codereview.chromium.org/399045/diff/1/6#newcode24
Line 24: EXTERN_C const PROPERTYKEY DECLSPEC_SELECTANY PKEY_AppUserModel_ID =
Since this variable is used only in SetAppIdForPropertyStore(), I prefer moving
this variable in the function.
By the way, what happens when we use EXTERN_C in a C++ namespace? To get access
to this variable, we need a namespace (i.e. win_util::PKEY_AppUserModel_ID) or
not? (This is just a personal interest, though.)

http://codereview.chromium.org/399045/diff/1/6#newcode392
Line 392: bool SetAppIdForPropertyStore(IPropertyStore* pps, const wchar_t*
app_id) {
Is it better to use ScopedComPtr<IPropertyStore> instead of IPropertyStore* to
maintain reference counts as WebKit does?

[xiyuan, 2009-11-18 06:31:21]

Good call for the app id format I changed the default app id from
IDS_PRODUCT_NAME to chrome::kBrowserAppName which for sure would have no space
in it. :)

All comments addressed except the one to use ScopedComPtr<IPropertyStore> in
SetAppIdForPropertyStore args. I think we should be okay because the
IPropertyStore object would only be used on one thread and caller would hold
reference for us.

I will upload the new snapshot shortly.

http://codereview.chromium.org/399045/diff/1/5
File base/file_util_win.cc (right):

http://codereview.chromium.org/399045/diff/1/5#newcode297
Line 297: ScopedComPtr<IShellLink> i_shell_link;
On 2009/11/18 02:08:18, hbono wrote:
> nit: ResolveShortCut() uses 'shell' for a ScopedComPtr<IShellLink> variable.
On
> the other hand, this function uses 'i_shell_link' for a
ScopedComPtr<IShellLink>
> variable. I personally think it is better to use the same name for both
> variables from the point of consistency.

Done.

http://codereview.chromium.org/399045/diff/1/5#newcode347
Line 347: CLSCTX_INPROC_SERVER)))
On 2009/11/18 02:08:18, hbono wrote:
> nit: it is better for this line to be aligned with CLSID_ShellLink.

Done.

http://codereview.chromium.org/399045/diff/1/5#newcode372
Line 372: if (app_id && (win_util::GetWinVersion() >=
win_util::WINVERSION_WIN7)) {
On 2009/11/18 02:08:18, hbono wrote:
> nit: internal braces are redundant.

Removed. I thought Chrome's coding style is to wrap condition expression in
parenthesis because the first a few files I worked on are doing this.

http://codereview.chromium.org/399045/diff/1/5#newcode373
Line 373: ScopedComPtr<IPropertyStore> pps;
On 2009/11/18 02:08:18, hbono wrote:
> nit: I prefer using descriptive name (e.g. 'property_store') to an acronym
> 'pps'.

Done.

http://codereview.chromium.org/399045/diff/1/6
File base/win_util.cc (right):

http://codereview.chromium.org/399045/diff/1/6#newcode24
Line 24: EXTERN_C const PROPERTYKEY DECLSPEC_SELECTANY PKEY_AppUserModel_ID =
On 2009/11/18 02:08:18, hbono wrote:
> Since this variable is used only in SetAppIdForPropertyStore(), I prefer
moving
> this variable in the function.
> By the way, what happens when we use EXTERN_C in a C++ namespace? To get
access
> to this variable, we need a namespace (i.e. win_util::PKEY_AppUserModel_ID) or
> not? (This is just a personal interest, though.)

Moved to SetAppIdForPropertyStore.

You question is very good. I don't have good answers for it even after a bit
google search. EXTERN_C defines C linkage and in this sense, I guess compiler
would generate a symbol name without namespace. To access it in C++, you still
need the namespace scope operator though. However, when you try to define
another EXTERN_C identifier with the same name outside the namespace, I guess
this would cause a linker error but have not tried it.

http://codereview.chromium.org/399045/diff/1/6#newcode392
Line 392: bool SetAppIdForPropertyStore(IPropertyStore* pps, const wchar_t*
app_id) {
On 2009/11/18 02:08:18, hbono wrote:
> Is it better to use ScopedComPtr<IPropertyStore> instead of IPropertyStore* to
> maintain reference counts as WebKit does?
I don't think this is necessary because this function would be called
synchronously and thus the caller would hold the reference for us.

[Hironori Bono, 2009-11-18 08:49:22]

http://codereview.chromium.org/399045/diff/1/6
File base/win_util.cc (right):

http://codereview.chromium.org/399045/diff/1/6#newcode24
Line 24: EXTERN_C const PROPERTYKEY DECLSPEC_SELECTANY PKEY_AppUserModel_ID =
On 2009/11/18 06:31:21, xiyuan wrote:
> On 2009/11/18 02:08:18, hbono wrote:
> > Since this variable is used only in SetAppIdForPropertyStore(), I prefer
> moving
> > this variable in the function.
> > By the way, what happens when we use EXTERN_C in a C++ namespace? To get
> access
> > to this variable, we need a namespace (i.e. win_util::PKEY_AppUserModel_ID)
or
> > not? (This is just a personal interest, though.)
> 
> Moved to SetAppIdForPropertyStore.
> 
> You question is very good. I don't have good answers for it even after a bit
> google search. EXTERN_C defines C linkage and in this sense, I guess compiler
> would generate a symbol name without namespace. To access it in C++, you still
> need the namespace scope operator though. However, when you try to define
> another EXTERN_C identifier with the same name outside the namespace, I guess
> this would cause a linker error but have not tried it.

Thank you for your description.
I just would like to clarify this EXTERN_C is intentional or not.

http://codereview.chromium.org/399045/diff/1/6#newcode392
Line 392: bool SetAppIdForPropertyStore(IPropertyStore* pps, const wchar_t*
app_id) {
On 2009/11/18 06:31:21, xiyuan wrote:
> On 2009/11/18 02:08:18, hbono wrote:
> > Is it better to use ScopedComPtr<IPropertyStore> instead of IPropertyStore*
to
> > maintain reference counts as WebKit does?
> I don't think this is necessary because this function would be called
> synchronously and thus the caller would hold the reference for us.

Thank you for your description. It's up to you whether or not using
ScopedComPtr<T> here. Nevertheless, ScopedComPtr<T> can prevent us from calling
T->AddRef() or T->Release() by mistake and I prefer using ScopedComPtr<T> even
though this function is called synchronously. For example, it can prevent me, a
stupid engineer, from adding T->Release() without considering deeply. :)

http://codereview.chromium.org/399045/diff/5002/6005
File base/file_util_win.cc (right):

http://codereview.chromium.org/399045/diff/5002/6005#newcode327
Line 327: ScopedComPtr<IPropertyStore> pps;
nit: a descriptive name, please.

http://codereview.chromium.org/399045/diff/5002/6006
File base/win_util.cc (right):

http://codereview.chromium.org/399045/diff/5002/6006#newcode401
Line 401: PROPVARIANT pv;
nit: a descriptive name, please.

http://codereview.chromium.org/399045/diff/5002/6008
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/399045/diff/5002/6008#newcode237
Line 237: std::wstring(chrome::kBrowserAppName),
I recommend to create a new function 'win_util::GetBrowserAppName()' (or
something) so other code that relies on this code can use it. (Every time this
App ID changes, we need to change all code that depends on this App ID. It is
nothing but a waste of time.)

[xiyuan, 2009-11-18 22:49:58]

http://codereview.chromium.org/399045/diff/1/6
File base/win_util.cc (right):

http://codereview.chromium.org/399045/diff/1/6#newcode24
Line 24: EXTERN_C const PROPERTYKEY DECLSPEC_SELECTANY PKEY_AppUserModel_ID =
On 2009/11/18 08:49:22, hbono wrote:
> On 2009/11/18 06:31:21, xiyuan wrote:
> > On 2009/11/18 02:08:18, hbono wrote:
> > > Since this variable is used only in SetAppIdForPropertyStore(), I prefer
> > moving
> > > this variable in the function.
> > > By the way, what happens when we use EXTERN_C in a C++ namespace? To get
> > access
> > > to this variable, we need a namespace (i.e.
win_util::PKEY_AppUserModel_ID)
> or
> > > not? (This is just a personal interest, though.)
> > 
> > Moved to SetAppIdForPropertyStore.
> > 
> > You question is very good. I don't have good answers for it even after a bit
> > google search. EXTERN_C defines C linkage and in this sense, I guess
compiler
> > would generate a symbol name without namespace. To access it in C++, you
still
> > need the namespace scope operator though. However, when you try to define
> > another EXTERN_C identifier with the same name outside the namespace, I
guess
> > this would cause a linker error but have not tried it.
> 
> Thank you for your description.
> I just would like to clarify this EXTERN_C is intentional or not.
> 
This is from my brainless copy-n-paste. :)

http://codereview.chromium.org/399045/diff/1/6#newcode392
Line 392: bool SetAppIdForPropertyStore(IPropertyStore* pps, const wchar_t*
app_id) {
On 2009/11/18 08:49:22, hbono wrote:
> On 2009/11/18 06:31:21, xiyuan wrote:
> > On 2009/11/18 02:08:18, hbono wrote:
> > > Is it better to use ScopedComPtr<IPropertyStore> instead of
IPropertyStore*
> to
> > > maintain reference counts as WebKit does?
> > I don't think this is necessary because this function would be called
> > synchronously and thus the caller would hold the reference for us.
> 
> Thank you for your description. It's up to you whether or not using
> ScopedComPtr<T> here. Nevertheless, ScopedComPtr<T> can prevent us from
calling
> T->AddRef() or T->Release() by mistake and I prefer using ScopedComPtr<T> even
> though this function is called synchronously. For example, it can prevent me,
a
> stupid engineer, from adding T->Release() without considering deeply. :)
I prefer not using it because construct/destruct of ScopedComPtr might involve
interlocked operations and could be slow. And we should be okay with google
engineers. :)

http://codereview.chromium.org/399045/diff/5002/6005
File base/file_util_win.cc (right):

http://codereview.chromium.org/399045/diff/5002/6005#newcode327
Line 327: ScopedComPtr<IPropertyStore> pps;
On 2009/11/18 08:49:22, hbono wrote:
> nit: a descriptive name, please.

Done.

http://codereview.chromium.org/399045/diff/5002/6006
File base/win_util.cc (right):

http://codereview.chromium.org/399045/diff/5002/6006#newcode401
Line 401: PROPVARIANT pv;
On 2009/11/18 08:49:22, hbono wrote:
> nit: a descriptive name, please.

Done.

http://codereview.chromium.org/399045/diff/5002/6008
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/399045/diff/5002/6008#newcode237
Line 237: std::wstring(chrome::kBrowserAppName),
On 2009/11/18 08:49:22, hbono wrote:
> I recommend to create a new function 'win_util::GetBrowserAppName()' (or
> something) so other code that relies on this code can use it. (Every time this
> App ID changes, we need to change all code that depends on this App ID. It is
> nothing but a waste of time.)

Since the app id is a static thing, I prefer to have it as a constant. I agree
with you that changing this every where is a waste of time and guess that your
concern is mostly about what if we could not use kBrowserAppName as app id. So
created a separated constant kBrowserAppId just for this. For now, it is the
same as kBrowserAppName and in the future, if we want to change the app id, we
could just change this constant.

[Hironori Bono, 2009-11-19 04:35:46]

LGTM with a couple of nits.
Thank you again for your hard work.

Regards,

Hironori Bono
E-mail: hbono@chromium.org

http://codereview.chromium.org/399045/diff/1/6
File base/win_util.cc (right):

http://codereview.chromium.org/399045/diff/1/6#newcode392
Line 392: bool SetAppIdForPropertyStore(IPropertyStore* pps, const wchar_t*
app_id) {
On 2009/11/18 22:49:58, xiyuan wrote:
> I prefer not using it because construct/destruct of ScopedComPtr might involve
> interlocked operations and could be slow. And we should be okay with Google
engineers. :)

Chromium is an open-source project, i.e. everyone can get its source code and
modify it. So, we need to write code not only for Google engineers but also for
all engineers. :)

http://codereview.chromium.org/399045/diff/24/1034#newcode397
Line 397: return false;
nit: the name of a const variable should use a k followed by mixed case:
kPKEYAppUserModelID.

http://codereview.chromium.org/399045/diff/24/1042
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/399045/diff/24/1042#newcode25
Line 25: extern const wchar_t kBrowserAppId[];
nit: kBrowserAppId -> kBrowserAppID (see kNotSignedInID).

[xiyuan, 2009-11-19 07:30:18]

All done. Will submit after try success.

Thanks for reviewing. :)

http://codereview.chromium.org/399045/diff/24/1034
File base/win_util.cc (right):

http://codereview.chromium.org/399045/diff/24/1034#newcode397
Line 397: static const PROPERTYKEY PKEY_AppUserModel_ID =
On 2009/11/19 04:35:47, hbono wrote:
> nit: the name of a const variable should use a k followed by mixed case:
> kPKEYAppUserModelID.

Done.

http://codereview.chromium.org/399045/diff/24/1042
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/399045/diff/24/1042#newcode25
Line 25: extern const wchar_t kBrowserAppId[];
On 2009/11/19 04:35:47, hbono wrote:
> nit: kBrowserAppId -> kBrowserAppID (see kNotSignedInID).

Done.

[daniel.fulda, 2010-01-26 19:13:39]

Hello!

When will this be updated in Google Chrome?

On 2009/11/19 07:30:18, xiyuan wrote:
> All done. Will submit after try success.
> 
> Thanks for reviewing. :)
> 
> http://codereview.chromium.org/399045/diff/24/1034
> File base/win_util.cc (right):
> 
> http://codereview.chromium.org/399045/diff/24/1034#newcode397
> Line 397: static const PROPERTYKEY PKEY_AppUserModel_ID =
> On 2009/11/19 04:35:47, hbono wrote:
> > nit: the name of a const variable should use a k followed by mixed case:
> > kPKEYAppUserModelID.
> 
> Done.
> 
> http://codereview.chromium.org/399045/diff/24/1042
> File chrome/common/chrome_constants.h (right):
> 
> http://codereview.chromium.org/399045/diff/24/1042#newcode25
> Line 25: extern const wchar_t kBrowserAppId[];
> On 2009/11/19 04:35:47, hbono wrote:
> > nit: kBrowserAppId -> kBrowserAppID (see kNotSignedInID).
> 
> Done.

[Hironori Bono, 2010-01-27 02:17:14]

Greetings,

Thank you for your question.
As far as I know, this change has been integrated into Google Chrome 4
(4.0.249.78), which is released this Monday. Is it possible to update your
Google Chrome and check if this issue is fixed on your PC? It is definitely
helpful to add a comment to our bug tracker <http://crbug.com/28104> if this
problem still happens with Google Chrome 4 on your PC.

Regards,

Hironori Bono

On 2010/01/26 19:13:39, daniel.fulda wrote:
> Hello!
> 
> When will this be updated in Google Chrome?
> 
> On 2009/11/19 07:30:18, xiyuan wrote:
> > All done. Will submit after try success.
> > 
> > Thanks for reviewing. :)
> > 
> > http://codereview.chromium.org/399045/diff/24/1034
> > File base/win_util.cc (right):
> > 
> > http://codereview.chromium.org/399045/diff/24/1034#newcode397
> > Line 397: static const PROPERTYKEY PKEY_AppUserModel_ID =
> > On 2009/11/19 04:35:47, hbono wrote:
> > > nit: the name of a const variable should use a k followed by mixed case:
> > > kPKEYAppUserModelID.
> > 
> > Done.
> > 
> > http://codereview.chromium.org/399045/diff/24/1042
> > File chrome/common/chrome_constants.h (right):
> > 
> > http://codereview.chromium.org/399045/diff/24/1042#newcode25
> > Line 25: extern const wchar_t kBrowserAppId[];
> > On 2009/11/19 04:35:47, hbono wrote:
> > > nit: kBrowserAppId -> kBrowserAppID (see kNotSignedInID).
> > 
> > Done.

[xiyuan, 2010-01-27 17:55:18]

As far as I know that this CL does not make into 4.0 release. :( It should be
included in 4.1.