Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
487693002:
ShellUtil: Add generic methods to add/delete Windows file associations. (Closed)
|
Created:
6 years, 4 months ago by Matt Giuca Modified:
6 years, 1 month ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionShellUtil: Add generic methods to add/delete Windows file associations.
The existing file handler registration code in ShellUtil was highly
specific to Chrome (and the necessary actions to register a web
browser). Refactored the code and split it up so that there is reusable
code for general (non-browser app) file handler registrations.
Added AddFileAssociations and DeleteFileAssociations, which can register
and unregister file handlers for any command line and for any file
extensions. These are intended to be used when installing/uninstalling
Chrome apps with file_handlers in their manifest.
BUG=130455
Committed: https://crrev.com/33dfd20c12513837f23d41452c7f8cfdec9a9e6a
Cr-Commit-Position: refs/heads/master@{#297814}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix include order (see crbug.com/404978) and lint errors. #
Total comments: 9
Patch Set 3 : Rebase. #Patch Set 4 : DeleteWindowsFileAssociations: Don't ignore errors. #Patch Set 5 : Full refactor, moving implementation into ShellUtil and using RegistryEntry. #Patch Set 6 : Rebase. #Patch Set 7 : Rebase. #Patch Set 8 : Deeper refactoring of the existing file associations code for code reuse. #
Total comments: 3
Patch Set 9 : Do write app_id even without delegateexecute. But not if it's empty. #Patch Set 10 : Rebase. #
Total comments: 43
Patch Set 11 : Respond to gab's comments. #Patch Set 12 : Added a TODO. #
Total comments: 22
Patch Set 13 : Respond to grt's comments. #
Total comments: 12
Patch Set 14 : Rebase. #Patch Set 15 : Reword. #Patch Set 16 : Fix call to OverrideRegistry. #Patch Set 17 : Deleted win_file_associator and just use shell_util. #Patch Set 18 : Added TODO. #
Total comments: 31
Patch Set 19 : Lots of changes. #Patch Set 20 : Added comments as a result of Windows 8 default association investigation. #Patch Set 21 : Minor comment change. #
Total comments: 7
Messages
Total messages: 56 (8 generated)
jackhou: Maybe you can take a first look at this in Sydney before I send it out to other reviewers elsewhere.
https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... chrome/browser/file_associations_win.cc:40: LOG(ERROR) << "Could not create HKEY_CURRENT_USER\\" << key_name; I think in general logging that will not be seen is discouraged. Maybe make these DLOGs. https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... chrome/browser/file_associations_win.cc:182: bool DeleteWindowsFileAssociations(const std::string& progid) { Is there a way to delete the entries added by AssociateExtension? If not, would that be a problem? https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... File chrome/browser/file_associations_win.h (right): https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... chrome/browser/file_associations_win.h:27: // |progid| is the unique internal name Windows will use for file associations Is there a convention for how to specify a progid? If so, it's probably worth mentioning it here.
https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... chrome/browser/file_associations_win.cc:40: LOG(ERROR) << "Could not create HKEY_CURRENT_USER\\" << key_name; Hmm OK. Well the problem is that these errors are likely to be caused by transient problems like registry being unavailable for some reason. So I think we do want it to be logged somewhere, but it shouldn't crash the program or interrupt the user. What is the reason for DLOG instead of LOG? It just means if it's happening to a user they'll be less likely to be able to diagnose it. https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... chrome/browser/file_associations_win.cc:182: bool DeleteWindowsFileAssociations(const std::string& progid) { We could, but it would mean either: a) Taking a list of extensions. Then we would have to make sure that the list of extensions we give during delete is different to the list we give during add (which it might not be, if for example the app changes or our internal list of extensions corresponding to a mime type changes), or b) Iterate over all extensions registered and find the ones mentioning our progid. (Seems unscalable.) This makes me think about an interesting problem: say an app changes its manifest and removes an extension from file handlers. Should we go in and delete that extension's association (without deleting the progid)? Sounds hard... (but a challenge for another CL.) https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... File chrome/browser/file_associations_win.h (right): https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... chrome/browser/file_associations_win.h:27: // |progid| is the unique internal name Windows will use for file associations Not really. Microsoft recommends "Vendor.Application" but nobody seems to follow it, including themselves. I intend to just use "chrome-APPID" for Chrome apps, and it doesn't make sense to document that here because this is more general than Chrome apps. So I think saying it's a "unique name" is sufficient.
https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... chrome/browser/file_associations_win.cc:182: bool DeleteWindowsFileAssociations(const std::string& progid) { On 2014/08/19 08:24:48, Matt Giuca wrote: > We could, but it would mean either: > a) Taking a list of extensions. Then we would have to make sure that the list of > extensions we give during delete is different to the list we give during add > (which it might not be, if for example the app changes or our internal list of > extensions corresponding to a mime type changes), or > b) Iterate over all extensions registered and find the ones mentioning our > progid. (Seems unscalable.) > > This makes me think about an interesting problem: say an app changes its > manifest and removes an extension from file handlers. Should we go in and delete > that extension's association (without deleting the progid)? Sounds hard... (but > a challenge for another CL.) What if we add a value to the progid key that contains the list of extensions? When updating, if this changes, we make the necessary additions/deletions.
Hi Carlos, can you take a look at this Windows-registry-heavy CL? (I've heard you're knowledgeable about these matters; I'll send it to OWNERS afterwards.) If you know someone more appropriate, please forward to them. https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/1/chrome/browser/file_associat... chrome/browser/file_associations_win.cc:182: bool DeleteWindowsFileAssociations(const std::string& progid) { Yeah that sounds reasonable. But I'd like to do it in a follow-up CL; it's a good separate chunk of functionality and it's non-trivial (updating and deleting should both be modified to respect this list). I *could* use SupportedTypes -- Windows already provides a mechanism for applications to list all of the extensions they support in a big list in the registry: http://msdn.microsoft.com/en-us/library/windows/desktop/ee872121.aspx but I tried to use it and it didn't seem to have any effect. Still, it can't hurt to use this for our record-keeping instead of inventing a new key.
The right person is grt@ but he is currently OOO but I imagine that robertshield is doing his deeds.
https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... chrome/browser/file_associations_win.cc:23: const wchar_t kRegClassesRootAlias[] = L"Software\\Classes"; I am going to say we have this kind of logic somewhere in our installer code already.
+grt, should be back on Thursday. It looks like the big issue is whether or not this code should be combined with shell_util. Comments welcome, but see my reply to cpu's comment inline. https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... chrome/browser/file_associations_win.cc:23: const wchar_t kRegClassesRootAlias[] = L"Software\\Classes"; See comment at the top of file_associations_win.h and in this CL's description. I looked at chrome/installer/util/shell_util and decided that that code was significantly different to this that it warranted a separate implementation. If I was to go the other route -- attempting to reuse the code in shell_util, it would be a big job. That code is very hard-coded for chrome.exe and specific to the Chrome install flow. It does a few things I don't want (like registering verbs), and it doesn't do some things I want (like registering as the default handler if and only if there isn't one already; and as Jack suggested, recording a list of registered file extensions so we can remove the association with those specific extensions later). If a reviewer thinks it's worth teasing apart these differences and building an abstraction that supports both use cases, then I'm happy to work through it. But I think it will be a big job so I didn't want to do it unless someone else thought it was worthwhile.
Added a few comments here. The code does look great, but generally I agree that this duplicates a fair amount of stuff in shell_util.cc and I think this should be redone in terms of the RegistryEntry / WorkItemList code used in shell_util. This said, I defer the final word on this review to Gab who has spent the most time building a mental model of how Chrome's shell integration code is structured and will have a stronger opinion on where this should live. https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... chrome/browser/file_associations_win.cc:5: #include "chrome/browser/file_associations_win.h" I think these files should be called shell_file_associations_win.* instead, to be consistent with other shell integration files. https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... chrome/browser/file_associations_win.cc:23: const wchar_t kRegClassesRootAlias[] = L"Software\\Classes"; On 2014/08/25 00:58:18, Matt Giuca wrote: > See comment at the top of file_associations_win.h and in this CL's description. > > I looked at chrome/installer/util/shell_util and decided that that code was > significantly different to this that it warranted a separate implementation. If > I was to go the other route -- attempting to reuse the code in shell_util, it > would be a big job. That code is very hard-coded for chrome.exe and specific to > the Chrome install flow. It does a few things I don't want (like registering > verbs), and it doesn't do some things I want (like registering as the default > handler if and only if there isn't one already; and as Jack suggested, recording > a list of registered file extensions so we can remove the association with those > specific extensions later). > > If a reviewer thinks it's worth teasing apart these differences and building an > abstraction that supports both use cases, then I'm happy to work through it. But > I think it will be a big job so I didn't want to do it unless someone else > thought it was worthwhile. There is a fair amount of duplication here with the stuff in shell_util.cc here, mostly the constants and the registry writing code. If not terribly onerous, I would suggest reusing as much of the stuff in shell_util as possible. One nice thing the code in shell_util.cc buys you is its use of WorkItemLists. That carries with it rollback support, meaning it makes a best effort to guarantee that either all or no registrations occur when there are multiple registrations. This code appears not to attempt to provide that guarantee. https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... chrome/browser/file_associations_win.cc:141: // Under OpenWithProgids, create an empty value with this class's name. Add a comment that this only for WinXP support. https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... chrome/browser/file_associations_win.cc:178: SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT, NULL, NULL); The CL makes no mention of when/how this will be called but please be aware that SHCNE_ASSOCCHANGED notifications can cause quite visible shell flicker on some machines/OS versions. It is best to do this as few times as possible, so if callers would need to call AddWindowsFileAssociations multiple times, it would be worth pulling this out and having some kind of queue-up-the-work-then-commit-it-once type interface. https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... chrome/browser/file_associations_win_unittest.cc:31: virtual void SetUp() OVERRIDE { Tests that write to the registry should use https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_reg... to avoid being flaky over time. Once in a while these tests will get killed half way through or will otherwise fail in unexpected ways leaving state left over. The ScopedRegistryOverride stuff helps deal with that.
Thanks for the response. I'll wait for some other people to chime in before doing the more structural changes. https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... chrome/browser/file_associations_win.cc:141: // Under OpenWithProgids, create an empty value with this class's name. I'm lost... this is the only place where I say "associate extension .foo with progid BAR." It certainly works on Windows 7. Am I supposed to set up this association some other way on newer versions of Windows?
https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/20001/chrome/browser/file_asso... chrome/browser/file_associations_win.cc:141: // Under OpenWithProgids, create an empty value with this class's name. On 2014/08/25 03:15:42, Matt Giuca wrote: > I'm lost... this is the only place where I say "associate extension .foo with > progid BAR." It certainly works on Windows 7. Am I supposed to set up this > association some other way on newer versions of Windows? Ick, sorry, I confused this vs OpenWithList which is deprecated, what you've got here is totally correct. Brainfart on my part :(
Seems it would be nice to have a generic class that uses WorkItemList and is eventually used both by this code and by shell_util for Chrome. It's not true that shell_util is strictly for machine wide registrations, it's also able to do user-level Chrome installs atm. Generalizing the shell_util code to not be specific to Chrome's progid and reusing it in two places sounds better than duplicating this logic. Cheers, Gab
Patchset #3 (id:40001) has been deleted
OK, given your collective feedback to reuse ShellUtil, I have refactored the whole patch to reuse the existing mechanisms in RegistryEntry. So an important property is that it now uses the work list concept and if one of them fails, they all roll back. Now this is a *very rough* patch to get an idea of whether I should proceed. I'm sure that I can massage the code a lot more, perhaps combine some cases, etc. So *please* don't give specific comments at this stage -- I know there are a number of issues with it and places that I can de-duplicate further. First matter: I think RegistryEntry is already really big and getting bigger. I'm not a fan of how monolithic it is (the fact that its constructors are private and ALL code that wants to construct one has to live inside it). I would like to break apart RegistryEntry, making its constructors public, moving all of its static methods outside the class and into the anonymous namespace in shell_util.cc. Now I can add new code that constructs RegistryEntries without having to bloat this already giant class. Let me know if this sounds good, and I'll do it in a precursor CL. Otherwise, I can live without it. Next: I can see two ways to go from here: a) Having broken up RegistryEntry, I can make it public and move all the code I've written back to file_associations_win.cc (so, not reusing the high-level logic from ShellUtil, just the low-level registry stuff). This isn't a huge amount of code now, but it would mean duplicating *some* registry logic. Again, as I said earlier, the registry logic for arbitrary file associations is quite different to the logic for Chrome itself, so even though there would be a bit of duplication, it also saves having to have complex logic for both cases in the same functions. or b) I can go further, deleting file_associations_win entirely and pushing the code deeper into ShellUtil. I can probably find a way to generalise GetChromeProgIdEntries and GetGenericProgIdEntries, as well as GetChromeAppRegistrationEntries and GetGenericAppRegistrationEntries. But the code will get quite hairy, as it now has to deal with very different use cases. For example, GetChromeAppRegistrationEntries adds "App Paths" which we don't want for the generic ones. And GetGenericAppRegistrationEntries avoids registering as the default if there is already an existing default. So the resulting combined code will be quite hairy as it needs to deal with both possibilities. Which of these do you think sounds like the better option? And do you think it's worth breaking up RegistryEntry?
On 2014/08/27 08:54:05, Matt Giuca wrote: > OK, given your collective feedback to reuse ShellUtil, I have refactored the > whole patch to reuse the existing mechanisms in RegistryEntry. So an important > property is that it now uses the work list concept and if one of them fails, > they all roll back. Yes, that's good, partial registry registrations are undesired. > > Now this is a *very rough* patch to get an idea of whether I should proceed. I'm > sure that I can massage the code a lot more, perhaps combine some cases, etc. So > *please* don't give specific comments at this stage -- I know there are a number > of issues with it and places that I can de-duplicate further. > > First matter: I think RegistryEntry is already really big and getting bigger. > I'm not a fan of how monolithic it is (the fact that its constructors are > private and ALL code that wants to construct one has to live inside it). I would > like to break apart RegistryEntry, making its constructors public, moving all of > its static methods outside the class and into the anonymous namespace in > shell_util.cc. Now I can add new code that constructs RegistryEntries without > having to bloat this already giant class. Let me know if this sounds good, and > I'll do it in a precursor CL. Otherwise, I can live without it. Yes, refactoring RegistryEntry makes sense to me. It was initially built like this so that only the static getters could actually generate RegistryEntry's, but I think we can achieve the same guarantees like this: 1) Extract the static getters to free methods in a new file (.h/.cc). 2) In the .h file have an abstract interface for RegistryEntry (so that users of this interface can use RegistryEntry, but not build it). 3) In the .cc file have a RegistryEntryImpl which is used when populating the lists. You can then reuse this generic file and its internal logic for your needs. > > Next: I can see two ways to go from here: > > a) Having broken up RegistryEntry, I can make it public and move all the code > I've written back to file_associations_win.cc (so, not reusing the high-level > logic from ShellUtil, just the low-level registry stuff). This isn't a huge > amount of code now, but it would mean duplicating *some* registry logic. Again, > as I said earlier, the registry logic for arbitrary file associations is quite > different to the logic for Chrome itself, so even though there would be a bit of > duplication, it also saves having to have complex logic for both cases in the > same functions. I would have to see the code, but it feels to me like you could write generic methods used both by Chrome and your code since they both do file association registration. > > or > > b) I can go further, deleting file_associations_win entirely and pushing the > code deeper into ShellUtil. I can probably find a way to generalise > GetChromeProgIdEntries and GetGenericProgIdEntries, as well as > GetChromeAppRegistrationEntries and GetGenericAppRegistrationEntries. But the > code will get quite hairy, as it now has to deal with very different use cases. > For example, GetChromeAppRegistrationEntries adds "App Paths" which we don't > want for the generic ones. And GetGenericAppRegistrationEntries avoids > registering as the default if there is already an existing default. So the > resulting combined code will be quite hairy as it needs to deal with both > possibilities. I don't think we want to make ShellUtil even bigger (I agree with you that's it's gotten too big already). Thus I support your desire to extract the RegistryEntry code out. As far as making generic calls, I'm fine with breaking down existing calls into many multiple generic calls (i.e. more calls from the existing Chrome callers, but re-using some of those calls in your code). > > Which of these do you think sounds like the better option? And do you think it's > worth breaking up RegistryEntry? At the EOD I have to see it in code to know for sure, but my preference for now is as described above. Happy to look at intermediate CLs as you move towards that goal. Cheers, Gab
Hi Gab, > Yes, refactoring RegistryEntry makes sense to me. It was initially built like > this so that only the static getters could actually generate RegistryEntry's, > but I think we can achieve the same guarantees like this: > 1) Extract the static getters to free methods in a new file (.h/.cc). > 2) In the .h file have an abstract interface for RegistryEntry (so that users > of this interface can use RegistryEntry, but not build it). > 3) In the .cc file have a RegistryEntryImpl which is used when populating the > lists. > > You can then reuse this generic file and its internal logic for your needs. I don't fully understand this plan. I don't think that having private constructors and only static getters is a desirable property for a class under normal circumstances. The suggestion I made above was designed to make RegistryEntry fully reusable, by two means: 1. any client can create a RegistryEntry object, and therefore 2. the logic of actually deciding *which* registry entries need to get created does not have to live inside the RegistryEntry class (the existing logic can be moved out). Therefore, we could move RegistryEntry to its own file, while keeping the Chrome installer specific logic in shell_util, and allowing me to write my Chrome app specific logic wherever I want. I'm not clear on how having a RegistryEntry and RegistryEntryImpl is going to achieve that goal. Let's make sure we're on the same page before refactoring begins. Matt
On 2014/09/01 13:07:30, Matt Giuca wrote: > Hi Gab, > > > Yes, refactoring RegistryEntry makes sense to me. It was initially built like > > this so that only the static getters could actually generate RegistryEntry's, > > but I think we can achieve the same guarantees like this: > > 1) Extract the static getters to free methods in a new file (.h/.cc). > > 2) In the .h file have an abstract interface for RegistryEntry (so that users > > of this interface can use RegistryEntry, but not build it). > > 3) In the .cc file have a RegistryEntryImpl which is used when populating the > > lists. > > > > You can then reuse this generic file and its internal logic for your needs. > > I don't fully understand this plan. I don't think that having private > constructors and only static getters is a desirable property for a class under > normal circumstances. The suggestion I made above was designed to make > RegistryEntry fully reusable, by two means: > 1. any client can create a RegistryEntry object, and therefore > 2. the logic of actually deciding *which* registry entries need to get created > does not have to live inside the RegistryEntry class (the existing logic can be > moved out). > > Therefore, we could move RegistryEntry to its own file, while keeping the Chrome > installer specific logic in shell_util, and allowing me to write my Chrome app > specific logic wherever I want. I'm not clear on how having a RegistryEntry and > RegistryEntryImpl is going to achieve that goal. > > Let's make sure we're on the same page before refactoring begins. > > Matt Ah ok, I see what you mean, you want to make RegistryEntry generic while keeping the actual registrations product specific. As I previously said the current design specifically prevents that "1. any client can create a RegistryEntry object" which is what I was trying to maintain with my internal impl suggestion. My suggestion though assumes that your code also doesn't handle building the RegistryEntry's but rather calls a "give me the registrations required to make app X a handler for file type Y" method. It feels to me like it's possible/useful to write such a generic method. In particular so that when OS updates come we only have to update one piece of code dealing with file handler registrations (as opposed to risking forgetting about the other one). You seem to think that each use case is unique enough that generalization is not required. I'm happy to review a CL in which you do not do this generalization and I'll assess whether I agree when faced with your proposal (or even better you could try to convince me a priori by just giving me a list of registration comparisons between what Chrome needs and apps need for this use case to show why they're not really as similar as I think they are). Cheers! Gab
> Ah ok, I see what you mean, you want to make RegistryEntry generic while keeping > the actual registrations product specific. > > As I previously said the current design specifically prevents that "1. any > client can create a RegistryEntry object" which is what I was trying to maintain > with my internal impl suggestion. So the refactoring of RegistryEntry is orthogonal to the generalization of the registry writing logic. The reason I want to refactor it is ideological; it isn't strictly necessary for my feature. I can avoid code duplication and generalize the registry writing logic without it. As I said above, I don't understand why you want this property of external clients being unable to create RegistryEntry objects. In my understanding of OO design, objects should provide a public interface for clients to create and manipulate them, and the code inside the object should be for manipulating that object's internal state. Ideally, there would be few or no static methods. As I see it, RegistryEntry is essentially a struct that holds a key/value pair to be written into the registry. It is an extremely general concept. It has nothing to do with file associations, and it should not have any logic inside itself relating to any specific registry keys. That code should be in the client that is accessing RegistryEntry from the outside, only it would require public access to the RegistryEntry constructor. This is a good thing: it means RegistryEntry doesn't need to be modified every time we want to add some new code that writes to the registry. Having said that, we don't *need* to refactor RegistryEntry: we can just keep going the way it has been going, adding more logic to the static methods in there. So let's discuss that separately. I will keep RegistryEntry as it is for this patch, and perhaps we can talk about refactoring it later. > My suggestion though assumes that your code also doesn't handle building the > RegistryEntry's but rather calls a "give me the registrations required to make > app X a handler for file type Y" method. > > It feels to me like it's possible/useful to write such a generic method. In > particular so that when OS updates come we only have to update one piece of code > dealing with file handler registrations (as opposed to risking forgetting about > the other one). You seem to think that each use case is unique enough that > generalization is not required. I don't think that at all. (That was my initial position but you've convinced me that it's worthwhile generalizing this code.) In Comment #14 I presented two paths forward: a) to keep the duplication and move it back into file_associations_win, or b) to generalize the existing logic and remove the new abstractions I created here. I mailed out Patch Set 5 as a stepping stone towards either of these approaches. It sounds like you strongly prefer b so I'll move further towards that.
SGTM, seeing the minimal patch without code duplication will help guide decisions as to what should really be moved outside of shell_util.cc. > In my understanding of OO design, objects should provide a public interface for clients to create and manipulate them, and the code inside the object should be for manipulating that object's internal state. Not always, there are patterns in which the public interface is access-only but the construction is private (e.g., factory design pattern, singleton design pattern). In this specific case it gives the nice property that all groups of registrations required to install Chrome are close together and I would like to keep this -- i.e., think of it as encapsulation of implementation. A world where Chrome registrations are sparse across the codebase is undesired and enforcing this by design, while restrictive, this. I'm happy to enforce that even generic file type associations have to go through this funnel as they are part of the "interesting registrations groups made by Chrome" that I would like to keep close together. As I previously said however, I do agree with you that the accumulation of static methods isn't very nice and I was instead proposing we move them to free methods in, e.g., chrome/installer/util/registration.h and have the public RegistryEntry interface accessible there for consumers to be able to read the generic entries (but not create them). I could also be convinced to have a separate class in registry_entry.[cc|h] used by those methods (and unit tested on its own -- which would be a plus), but I'm leery of this allowing other people to do one off registration implementations fragmenting the known registrations made by Chrome -- actually this makes me think that the best way to enforce who uses it would probably be to componentize this registration code out of chrome/ and control its users via DEPS but I don't think that's in the scope of this change. Anyways, I'll wait for you to put your proposal together and we'll assess what it looks like once in place. Cheers, Gab
Patchset #7 (id:140001) has been deleted
Hi, I've refactored the existing code so that there are now three generic methods for creating prog_ids, file associations, and default file associations. The existing methods now use these three, and my code also uses those three new methods, so there is no code duplication. I'd still like to do a bit of cleanup, but maybe you can have a look at the RegistryEntry code so I can get your initial comments. https://codereview.chromium.org/487693002/diff/180001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/180001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:379: if (app_info.set_delegate_execute) { Note: I've changed the behaviour here -- I don't think we need to write AppUserModelId if we aren't using the app_id elsewhere (i.e., if we aren't using DelegateExecute). If you think this is correct, I'll move this change out into a separate CL for clarity.
https://codereview.chromium.org/487693002/diff/180001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/180001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:379: if (app_info.set_delegate_execute) { Actually, I think this was a bad idea -- you probably still need AppUserModelId even without DelegateExecute. So I have reverted this so it always writes AppUserModelId regardless of whether set_delegate_execute is true.
lg! I agree with you that this code is growing out of control though! Extracting RegistryEntry to a public class and extracting the generic methods to shell_registration_util.[cc|h] sounds like a good code-health thing to do (shell_util.cc is now a huge mix of unrelated shortcut/registry/other stuff -- we should split registry/shortcut stuff out to their own files). But I don't think refactoring is absolutely necessary for this CL (a follow-up CL would be much appreciated though). Thanks a lot for your patience and commitment to avoid code duplication, I like this CL a lot more now. Cheers, Gab https://codereview.chromium.org/487693002/diff/180001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/180001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:379: if (app_info.set_delegate_execute) { Right, I forget the exact details here, but it feels safer to leave it as-is. https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:33: SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT, NULL, NULL); IIRC this will also produce a fly-in on Win8 telling the user that "a new app now supports file type X" and asking them to pick their desired default. Also note that it will clear the icon cache (forcing a taskbar/desktop flash of all icons) -- to be used with caution. I also don't think you want to use SHCNF_FLUSHNOWAIT (it has caused problems in the past: http://crbug.com/136567 and shouldn't be used only you can explicitly justify why you want it). https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win.h (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Isn't there a better (more specific) directory for this than chrome/browser? https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win.h:23: // default association, the given application becomes the default. Otherwise, Again, verify your claim about grabbing default on Win7 and Win8. https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:18: // these classes and extensions will be deleted from the real registry! Oh no no no! This will make for flaky tests and bots. Use the RegistryOverrideManager (test_reg_util_win.h) for tests. (essentially the scoped temp dir + ScopedPathOverride concept but for the registry -- it allows you to redirect say all of HKCU to a temp key, the rest of your tests still think they're using the real HKCU but they're not) https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:210: // TODO(mgiuca): |file_type_icon_path| should be a base::FilePath. Agreed as that makes types more explicit, what prevents you from doing this now (is it just a matter of avoiding extra plumbing in this first CL)? https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:227: base::string16 publisher_name; This is the core info, I'd say put this block first in the struct. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:293: app_info.publisher_name = dist->GetPublisherName(); As mentioned above, put this information first (and initialize all other members of the struct in the same order they're declared in the struct (makes it easier to see that nothing was missed). Also, I'd say it's more "The file type icon is the same as the Chrome app icon." -- i.e., the most likely to change one day would be the file type path, not the app icon path. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:394: // TODO(grt): http://crbug.com/75152 Write a reference to a localized This TODO now belongs in the Chrome specific method above. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:411: } Does it really hurt to set those if they're empty? I think it'd be fine and would make the code smaller. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:543: // class's name. s/name/prog_id https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:557: static void GetAppDefaultRegistrationEntries( Note in the method name that these are XP style defaults (I don't think they do anything beyond XP). https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:570: entries->push_back(default_association.release()); nit: {} since conditional is multiline https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:682: bool KeyExistsInRegistry(uint32 look_for_in) const { Seems your only caller of this cares only about HKCU, so they could directly do: StatusInRegistryUnderRoot(HKEY_CURRENT_USER) != DOES_NOT_EXIST (although I agree this method is nicer as a potential public API if we move RegistryEntry out of this file) https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2448: base::string16 ext(1, L'.'); Add this check above. DCHECK(!it->empty() && (*it)[0] != L'.'); https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2469: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx Right but you should still delete the OpenWith registrations (i.e. GetAppExt...()). https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.h:606: // existing default association, the given application becomes the default. Did you try this on Win8? Typically nothing becomes default automatically on Win8 (although maybe this is true for unique file assoc.).
Patchset #11 (id:240001) has been deleted
https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win.h (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Maybe -- I wanted to make it a peer of shell_integration_win, which is why it's here. Let's wait until the rest is sorted out. I may end up just deleting this file because it's essentially a thin wrapper around functions in shell_util now. https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:18: // these classes and extensions will be deleted from the real registry! Interesting. I looked for something like this but found that the win/registry_unittest tests just write directly to the real registry: https://code.google.com/p/chromium/codesearch#chromium/src/base/win/registry_... Anyway, I'll fix this to use RegistryOverrideManager, but not right now. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:210: // TODO(mgiuca): |file_type_icon_path| should be a base::FilePath. Yes, I want to avoid touching heaps of other code in this CL. I'm happy to do it in a follow-up. It's a simple change but touches a lot of things. Ditto for the |command_line| one. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:227: base::string16 publisher_name; What do you mean by "core info"? All of this is optional, and it's also ignored before Win8. The most important info is in the first block. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:293: app_info.publisher_name = dist->GetPublisherName(); See comment above. OK I've done the latter suggestion. (Also filed bug: http://crbug.com/414141.) https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:394: // TODO(grt): http://crbug.com/75152 Write a reference to a localized On 2014/09/12 13:32:36, gab wrote: > This TODO now belongs in the Chrome specific method above. Done. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:411: } I don't know, but I could imagine Windows treating it differently if it was empty vs missing (for example, somewhere it might display the filename if the ApplicationName is missing, but show an empty string if ApplicationName is ""). I'd rather play it safe and not write empty strings here. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:543: // class's name. On 2014/09/12 13:32:37, gab wrote: > s/name/prog_id Done. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:557: static void GetAppDefaultRegistrationEntries( They certainly work in Windows 7 (haven't tested in 8). For app registrations, I'm only using this (not the IApplicationAssociationRegistration interface), and it's working fine. Is there a reason I should be using both the old and the new approaches for file type registration? Anyway I'm happy to rename this to "XPStyle" but I'm wondering if I also need to use the new interface to register a file type as the default. From my brief reading of the docs, this new interface (which by the way is deprecated in Windows 8) is for user-mode apps to register handlers at the system level, which is not what I want for app registrations, so I think it's fine to continue just using the XP style. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:570: entries->push_back(default_association.release()); On 2014/09/12 13:32:36, gab wrote: > nit: {} since conditional is multiline Done. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:682: bool KeyExistsInRegistry(uint32 look_for_in) const { I think for the latter reason that it's best to keep this abstraction. (It's better for consistency with ExistsInRegistry). https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2448: base::string16 ext(1, L'.'); On 2014/09/12 13:32:36, gab wrote: > Add this check above. > > DCHECK(!it->empty() && (*it)[0] != L'.'); Done. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2469: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx While that document doesn't say *not* to remove those associations, it doesn't hurt to leave them (I've tested it, and Windows ignores it if the class has been removed). Jack originally suggested (in comment #4) that we record a list of all extensions in the progid key, so that later we can go to each extension and delete its association. Without doing that, we can't delete the association from each extension without doing a linear scan through the entire list of classes. I think I'll take Jack's advice but I don't want to do it in this CL (which is OK because we're not using this code just yet). Added a TODO.
Note: I haven't looked at the Windows-8-specific stuff yet. I'm testing it out now.
https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:18: // these classes and extensions will be deleted from the real registry! On 2014/09/15 03:49:45, Matt Giuca wrote: > Interesting. I looked for something like this but found that the > win/registry_unittest tests just write directly to the real registry: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/win/registry_... > > Anyway, I'll fix this to use RegistryOverrideManager, but not right now. These tests will be flaky if you don't do it now since unit_tests.exe is parallelized. It's may be as simple as: #include "base/test/test_reg_util_win.h" class FileAssociationsTest : public testing::Test { protected: virtual void SetUp() OVERRIDE { testing::Test::SetUp(); registry_overrides_.OverrideRegistry(HKEY_CURRENT_USER, base::string16()); } registry_util::RegistryOverrideManager registry_overrides_; }; The one hitch would be if the reads through HKEY_CLASSES_ROOT don't end up going through the virtualization. I honestly don't know if they will or not. It's worth a shot. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:7: #include <shlobj.h> i believe <windows.h> should always be included before all other windows-y headers. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:12: bool AddWindowsFileAssociations(const std::string& progid, what will call this? why is |progid| a string rather than a string16? same question for file_type_name. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:13: const base::CommandLine& command_line, #include "base/command_line.h" https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:15: const base::FilePath& icon_path, #include "base/files/file_path.h" https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:17: std::set<base::string16> file_extensions_16; #include "base/strings/string16.h" https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:17: // 'Chrome' and avoid clobbering common extensions. Note: By running this test, personally, i'd go with Chromium here https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2477: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx it's a trap! remove them anyway to avoid breaking links in the wonderfully compliant Microsoft Office. see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/s... and http://crbug.com/149089 and weep. or just take my word on it and do it.
Patchset #13 (id:300001) has been deleted
https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:18: // these classes and extensions will be deleted from the real registry! By "not right now", I meant not in this patch set (which is why I put a "DO NOT SUBMIT" comment here). Anyway, thanks for the tip. It's done now. As you suspected, this did break the mirroring to HKEY_CLASSES_ROOT, but I just removed the use of the mirror (I directly read HKEY_CURRENT_USER\Software\Classes), which is fine -- we don't need to test Windows here anyway. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:7: #include <shlobj.h> On 2014/09/15 13:38:37, grt wrote: > i believe <windows.h> should always be included before all other windows-y > headers. Done. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:12: bool AddWindowsFileAssociations(const std::string& progid, I'm trying to abstract over the implementation details. Using std::string for strings is standard practice throughout the Chromium code base, and they are converted to string16 only when we need to interface with some Windows API. The functions in shell_util don't follow this convention, so my functions there take string16, but in this new module, it's best to follow the convention. (Having said that, I might delete this module altogether depending on how simple it ends up being -- right now it doesn't look worthwhile.) https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:13: const base::CommandLine& command_line, I don't think this is needed because this file never touches the inside of a CommandLine, only passes it on by reference to another function. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:15: const base::FilePath& icon_path, Same. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:17: std::set<base::string16> file_extensions_16; On 2014/09/15 13:38:37, grt wrote: > #include "base/strings/string16.h" Done. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:17: // 'Chrome' and avoid clobbering common extensions. Note: By running this test, On 2014/09/15 13:38:37, grt wrote: > personally, i'd go with Chromium here Just removed the prefix (no longer necessary since RegistryOverrideManager means we won't conflict with any other keys). https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2477: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx I read over that. It seems that bug is not actually about deleting the link, but setting it back to IE or whatever it was before. Is that right? So that issue doesn't really affect us on Chrome Apps, since we only set the default if there was nothing else there already. Still, to be sure, I will delete the default association if it points to the existing prog_id. But I still won't do it in this CL (since we don't have the information about which extensions we need to remove it from).
LGTM. I don't care for the charset conversions, but I won't scream too loudly. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:12: bool AddWindowsFileAssociations(const std::string& progid, On 2014/09/16 07:29:12, Matt Giuca wrote: > I'm trying to abstract over the implementation details. Using std::string for > strings is standard practice throughout the Chromium code base, and they are > converted to string16 only when we need to interface with some Windows API. The reason I asked about the caller is that it would be unfortunate if the source of the string for the caller is a UFT16 string that they have to convert to UTF8 just so this code can switch it back. A prog_id is inherently a Windows-y thing, so I don't see why the caller would want it in UTF8. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:13: const base::CommandLine& command_line, On 2014/09/16 07:29:13, Matt Giuca wrote: > I don't think this is needed because this file never touches the inside of a > CommandLine, only passes it on by reference to another function. Acknowledged. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:15: const base::FilePath& icon_path, On 2014/09/16 07:29:13, Matt Giuca wrote: > Same. Acknowledged. https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:17: // 'Chrome' and avoid clobbering common extensions. Note: By running this test, On 2014/09/16 07:29:13, Matt Giuca wrote: > On 2014/09/15 13:38:37, grt wrote: > > personally, i'd go with Chromium here > > Just removed the prefix (no longer necessary since RegistryOverrideManager means > we won't conflict with any other keys). Acknowledged. https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2477: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx On 2014/09/16 07:29:13, Matt Giuca wrote: > I read over that. It seems that bug is not actually about deleting the link, but > setting it back to IE or whatever it was before. Is that right? Both are required to appease Office. At least if Chrome doesn't leave values behind we're less likely to be seen as shipping poorly written software (despite following the guidance on MSDN -- even an MS engineer was harping on Chrome for the bug I liked to). Please at least adjust the comment so that it makes it clear that while MSDN says Windows will ignore it, other software may not. a TOD for now is fine with me.
On 2014/09/16 13:34:56, grt wrote: > LGTM. I don't care for the charset conversions, but I won't scream too loudly. Please wait for Gab to continue his review before landing. Thanks.
https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:12: bool AddWindowsFileAssociations(const std::string& progid, Yeah, that would be unfortunate, but not a huge problem and I think the design is better if abstract interfaces like this are using the platform-neutral string type. All the call sites I plan to use this with have a std::string because they build it from platform-neutral sources (like extension manifest data). https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2477: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx On 2014/09/16 13:34:56, grt wrote: > On 2014/09/16 07:29:13, Matt Giuca wrote: > > I read over that. It seems that bug is not actually about deleting the link, > but > > setting it back to IE or whatever it was before. Is that right? > > Both are required to appease Office. If that's true, then the MSDN advice is completely correct. Deleting it is the same as leaving it pointing at an invalid program. So there's no point leaving a TODO here which says "don't follow the MSDN advice because it's wrong." (Really, you're asking me to delete it anyway, simply because it's cleaner, which is fine.) Furthermore, it doesn't sound Office has any bug at all. Perhaps bad UI, but it sounds like Office is basically saying "what application is associated with this extension -- oh, nothing. Error." Any other application, including the Windows shell, does the same. It's not a bug in any software, it's just what naturally happens if an extension is associated with program A, then you set program B as the default, then uninstall program B. Now nothing is the default association, so things will break. I think the core issue is when an application overwrites the default extension, which we never do for Chrome Apps.
lg, a few more things below. https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:33: SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT, NULL, NULL); On 2014/09/12 13:32:36, gab wrote: > IIRC this will also produce a fly-in on Win8 telling the user that "a new app > now supports file type X" and asking them to pick their desired default. > > Also note that it will clear the icon cache (forcing a taskbar/desktop flash of > all icons) -- to be used with caution. > > I also don't think you want to use SHCNF_FLUSHNOWAIT (it has caused problems in > the past: http://crbug.com/136567 and shouldn't be used only you can explicitly > justify why you want it). ping. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:557: static void GetAppDefaultRegistrationEntries( On 2014/09/15 03:49:45, Matt Giuca wrote: > They certainly work in Windows 7 (haven't tested in 8). For app registrations, > I'm only using this (not the IApplicationAssociationRegistration interface), and > it's working fine. Is there a reason I should be using both the old and the new > approaches for file type registration? > > Anyway I'm happy to rename this to "XPStyle" but I'm wondering if I also need to > use the new interface to register a file type as the default. From my brief > reading of the docs, this new interface (which by the way is deprecated in > Windows 8) is for user-mode apps to register handlers at the system level, which > is not what I want for app registrations, so I think it's fine to continue just > using the XP style. You're right that IApplicationAssociationRegistration doesn't work in Win8 as they changed the entire landscape. I also didn't think the registry method @ [1] worked post-XP (are you sure it doesn't just happen to work because there is only one program in the extension you're testing, making it an implicit default? In Win8 the registrations + SHCNE_ASSOCCHANGED call will most likely be enough to give the user a fly-in saying "a new application support file type foo", but setting the default won't do anything IMO. [1] http://msdn.microsoft.com/en-ca/library/windows/desktop/cc144148(v=vs.85).aspx https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2469: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx On 2014/09/15 03:49:46, Matt Giuca wrote: > While that document doesn't say *not* to remove those associations, it doesn't > hurt to leave them (I've tested it, and Windows ignores it if the class has been > removed). > > Jack originally suggested (in comment #4) that we record a list of all > extensions in the progid key, so that later we can go to each extension and > delete its association. > > Without doing that, we can't delete the association from each extension without > doing a linear scan through the entire list of classes. I think I'll take Jack's > advice but I don't want to do it in this CL (which is OK because we're not using > this code just yet). > > Added a TODO. The documentation *does* explicitly say that these should be removed: "When uninstalling an application, the ProgIDs and most other registry information associated with that application should be deleted as part of the uninstallation." @ http://msdn.microsoft.com/en-ca/library/windows/desktop/cc144148(v=vs.85).asp... I don't think recording extra state about what needs to be cleaned is necessary. The registry lives in memory on Windows and iterating through it to find what you need to remove is most likely not a problem (we do much worse already; on uninstall we iterate over all shortcuts on disk in known locations to delete the ones pointing to our chrome.exe; this requires IO over an unbounded amount of files...). https://codereview.chromium.org/487693002/diff/320001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/320001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:73: key.Open(HKEY_CURRENT_USER, L"Software\\Classes\\TestApp", KEY_READ)); Build these test strings from the constants (mostly to make this more readable as an expectation following the call using those constants, but also more reliable). https://codereview.chromium.org/487693002/diff/320001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:75: EXPECT_EQ(L"Test File Type", value); Same here/below, use constants. https://codereview.chromium.org/487693002/diff/320001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:127: // Delete them again. What do you mean by "again"; this is the first time you're deleting them AFAICT..? https://codereview.chromium.org/487693002/diff/320001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/487693002/diff/320001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:411: 'browser/file_associations_win.cc', How about we put this code in c/b/apps? We should try to avoid polluting chrome/browser further IMO. https://codereview.chromium.org/487693002/diff/320001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/320001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:232: bool set_delegate_execute; Since only Chrome should ever set this to true, how about leaving the code specific to this in the Chrome-specific method and not bothering making it part of the generic API?
https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/280001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2477: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx On 2014/09/18 00:07:04, Matt Giuca wrote: > On 2014/09/16 13:34:56, grt wrote: > > On 2014/09/16 07:29:13, Matt Giuca wrote: > > > I read over that. It seems that bug is not actually about deleting the link, > > but > > > setting it back to IE or whatever it was before. Is that right? > > > > Both are required to appease Office. > > If that's true, then the MSDN advice is completely correct. Deleting it is the > same as leaving it pointing at an invalid program. So there's no point leaving a > TODO here which says "don't follow the MSDN advice because it's wrong." (Really, > you're asking me to delete it anyway, simply because it's cleaner, which is > fine.) > > Furthermore, it doesn't sound Office has any bug at all. > > Perhaps bad UI, but it > sounds like Office is basically saying "what application is associated with this > extension -- oh, nothing. Error." Any other application, including the Windows > shell, does the same. It's not a bug in any software, it's just what naturally > happens if an extension is associated with program A, then you set program B as > the default, then uninstall program B. Now nothing is the default association, > so things will break. > > I think the core issue is when an application overwrites the default extension, > which we never do for Chrome Apps. If memory serves, the shell handles the case of the registered program going away gracefully. Office tries to replicate its behavior (rather than just calling ShellExecuteEx) and delivers a bad UX as a result, hence my belief that this is a bug in Office. If Chrome's breadcrumb trail is left behind in the registry, fingers get pointed at Chrome rather than at the application that isn't using standard functions for following links. Yes, the behavior is much worse for htmlfile than it would be for an arbitrary url scheme.
Patchset #14 (id:340001) has been deleted
Deleted file_associations_win module; the client will just directly use shell_util. This should be good to go now, except for a little TODO I put in GetProgIdEntries. PTAL. https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:33: SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT, NULL, NULL); On 2014/09/18 01:16:14, gab wrote: > On 2014/09/12 13:32:36, gab wrote: > > IIRC this will also produce a fly-in on Win8 telling the user that "a new app > > now supports file type X" and asking them to pick their desired default. I tried this on Windows 8 and I didn't see any fly-in. In fact it behaved exactly like it does in Windows XP/7: I had full programmatic control over the default association by changing the registry. What is the specific sequence of actions I can do (as a user) to trigger this fly-in? I've never seen it before. I'm speculating that this fly-in (not to mention all of this other "new style Windows 8" associations logic) is just for setting the default browser (which I understand is a Very Special Thing in Windows 8); setting other file type handlers is still the same as always. > > Also note that it will clear the icon cache (forcing a taskbar/desktop flash > of > > all icons) -- to be used with caution. Yeah but we already call SHChangeNotify upon installing or updating apps so it is already an issue. However, it does need to be taken out of here and made into something that's called once at the top level. Ideally we'd want it to be called once for both creating app shortcuts and associations (but that may require more refactoring). > > > > > > I also don't think you want to use SHCNF_FLUSHNOWAIT (it has caused problems > in > > the past: http://crbug.com/136567 and shouldn't be used only you can > explicitly > > justify why you want it). > > ping. OK well I can't find anywhere that tells me what FLUSHNOWAIT actually does. I must have copied it from web_app_win where we use it. We also use it in profile_shortcut_manager_win. The Microsoft documentation (http://msdn.microsoft.com/en-us/library/windows/desktop/bb762118.aspx) is really unclear: FLUSH is "The function should not return until the notification has been delivered to all affected components." FLUSHNOWAIT is "The function should begin delivering notifications to all affected components but should return as soon as the notification process has begun." It sounds like you're always going to get one of those two behaviours, so it is unclear what it means if you don't specify either of them. (Is there a third possibility? Or is one of those two the default?) I suppose it's safer to just take out FLUSHNOWAIT but I'd like to know what you think it's for. Anyway, this discussion will have to move to another CL, since I've just deleted these functions. The client code will be doing the SHChangeNotify now... https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:557: static void GetAppDefaultRegistrationEntries( On 2014/09/18 01:16:15, gab wrote: > On 2014/09/15 03:49:45, Matt Giuca wrote: > You're right that IApplicationAssociationRegistration doesn't work in Win8 as > they changed the entire landscape. > > I also didn't think the registry method @ [1] worked post-XP (are you sure it > doesn't just happen to work because there is only one program in the extension > you're testing, making it an implicit default? Where on that page ([1]) does it say it doesn't work post-XP? All I can find is that it says OpenWithList "is intended only for .exe applications on operating systems prior to Windows XP". But we aren't using OpenWithList; we're using OpenWithProgIds. It certainly does work as expected in Windows 7. My Windows 8 machine is out of commission at the moment (we recently moved offices). I will set it up tomorrow. So I can't confirm your question about making it the implicit default. But from memory, I'm fairly sure last time I tried it out on Windows 8 I played with setting and unsetting the default in the registry, and it worked just like in Windows 7. > > In Win8 the registrations + SHCNE_ASSOCCHANGED call will most likely be enough > to give the user a fly-in saying "a new application support file type foo", but > setting the default won't do anything IMO. As I said on the other comment, I haven't seen a fly-in and I don't see any evidence that setting the default doesn't work. As far as I can tell, this stuff has not changed since Windows XP or possibly even earlier than that. (However, again, browser registration is a different story because of how special browsers are in Windows 8.) > > [1] > http://msdn.microsoft.com/en-ca/library/windows/desktop/cc144148(v=vs.85).aspx https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2469: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx But remember I will be calling this code far more frequently than when uninstalling Chrome. It will be whenever an app is removed, which could be triggered by sync and could be done in bulk. I don't think that iterating over every single key in HKEY_CLASSES_ROOT is a good idea in this situation. Far better to keep our own index so we can delete exactly what we need. https://codereview.chromium.org/487693002/diff/320001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/320001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:73: key.Open(HKEY_CURRENT_USER, L"Software\\Classes\\TestApp", KEY_READ)); Now it's my turn to quote TOTT to you :) Episode 338: "Don't Put Logic in Tests" essentially gives this exact case as an example. We don't want bugs in the test such as missing or doubling up slashes. If I build the strings from the constants, my test would have the exact same logic as the code it's testing, and if that logic builds the wrong string, the test will still pass. It's best to fully write out the paths in the test even if it means duplicating them a lot. https://codereview.chromium.org/487693002/diff/320001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:127: // Delete them again. On 2014/09/18 01:16:15, gab wrote: > What do you mean by "again"; this is the first time you're deleting them > AFAICT..? I guess I meant "again" in the "All the king's horses and all the king's men couldn't put Humpty together again" sense --- to revert back to a previous state, as opposed to doing an action a second time. I'll reword it... https://codereview.chromium.org/487693002/diff/320001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/487693002/diff/320001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:411: 'browser/file_associations_win.cc', It's deleted now. Just directly use shell_util. https://codereview.chromium.org/487693002/diff/320001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/320001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:232: bool set_delegate_execute; I'm not sure... I prefer to keep all this logic together in one place. I'm happy to move it back if you feel strongly about it. Actually, if not, I need to make sure I generalize it a bit more, since it still has "Browser.Launch" in there. Added a TODO about that.
I've seen the fly-in for other types beyond html, but I have to say not very frequently, like maybe 3 times in the whole year. I imagine that the fly-in is tied to apps that generate the proper shell notification when they change an association as registry change notifications can be flaky. Other than that I want to say that the change is looking good.
On 2014/09/30 20:27:33, cpu wrote: > I've seen the fly-in for other types beyond html, but I have to say not very > frequently, like maybe 3 times in the whole year. > > I imagine that the fly-in is tied to apps that generate the proper shell > notification when they change an association as registry change notifications > can be flaky. > > Other than that I want to say that the change is looking good. Right, from playing a lot with it when working on registration for Chrome in Win8 I remember that the following logic triggered it fairly consistently: 1) App X registers for protocol Y for the first time (i.e. unregistering and re-registering app X doesn't work -- Windows seems to remember this somehow) 2) The shell is notified of the registration via SHChangeNotify. There is probably some other logic determining the main "protocol" for an app as for Chrome this would only bring the fly-in for HTTP when Chrome obviously supports much more than HTTP (e.g., FTP and many other protocols/file types). (so perhaps in this case there is no fly-in because HTTP is really Chrome's main thing and other file types are considered secondary by Windows)... Anyways, you're right that we shouldn't rely on this, but trying to do things right and give a shot at Windows notifying the user if it wants doesn't hurt. FWIW, the documentation only specifies default browser and is very vague (says that we should sleep(1000) before notifying... LOL!): http://msdn.microsoft.com/en-us/library/windows/desktop/cc144154(v=vs.85).asp... (looking at the rest of the CL now)
lgtm w/ nits/comments below (and follow up CLs for TODOs). I think you'll want to thoroughly test this on recent Windows platforms as some of the assumptions in the shell_util API regarding defaults appear to be wrong (i.e. to be based on browsers, but not applying to generic file types); clarifying those with your findings would be great (most of this API is based on experimentation which often trumps documentation..!). Thanks! Gab https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:33: SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT, NULL, NULL); On 2014/09/30 10:21:12, Matt Giuca wrote: > On 2014/09/18 01:16:14, gab wrote: > > On 2014/09/12 13:32:36, gab wrote: > > > IIRC this will also produce a fly-in on Win8 telling the user that "a new > app > > > now supports file type X" and asking them to pick their desired default. > > I tried this on Windows 8 and I didn't see any fly-in. In fact it behaved > exactly like it does in Windows XP/7: I had full programmatic control over the > default association by changing the registry. > > What is the specific sequence of actions I can do (as a user) to trigger this > fly-in? I've never seen it before. > > I'm speculating that this fly-in (not to mention all of this other "new style > Windows 8" associations logic) is just for setting the default browser (which I > understand is a Very Special Thing in Windows 8); setting other file type > handlers is still the same as always. > > > > Also note that it will clear the icon cache (forcing a taskbar/desktop flash > > of > > > all icons) -- to be used with caution. > > Yeah but we already call SHChangeNotify upon installing or updating apps so it > is already an issue. However, it does need to be taken out of here and made into > something that's called once at the top level. Ideally we'd want it to be called > once for both creating app shortcuts and associations (but that may require more > refactoring). > > > > > > > > > > > I also don't think you want to use SHCNF_FLUSHNOWAIT (it has caused problems > > in > > > the past: http://crbug.com/136567 and shouldn't be used only you can > > explicitly > > > justify why you want it). > > > > ping. > > OK well I can't find anywhere that tells me what FLUSHNOWAIT actually does. I > must have copied it from web_app_win where we use it. We also use it in > profile_shortcut_manager_win. The Microsoft documentation > (http://msdn.microsoft.com/en-us/library/windows/desktop/bb762118.aspx) is > really unclear: > > FLUSH is "The function should not return until the notification has been > delivered to all affected components." > FLUSHNOWAIT is "The function should begin delivering notifications to all > affected components but should return as soon as the notification process has > begun." > > It sounds like you're always going to get one of those two behaviours, so it is > unclear what it means if you don't specify either of them. (Is there a third > possibility? Or is one of those two the default?) The default is for the notification to be buffered up and issued normally. FLUSH makes it such that the shell is notified right-away and the method only returns when the OS is done handling the notification. FLUSHNOWAIT only notifies the OS right away but doesn't wait for it to be done handling the notification. From my experience, flushing can cause the OS to be notified of the change before the change itself is even registered with the OS (where as not flushing seems to ensure that the notification is queued up behind the recent changes). Code currently using FLUSHNOWAIT is probably incorrect. > > I suppose it's safer to just take out FLUSHNOWAIT but I'd like to know what you > think it's for. > > Anyway, this discussion will have to move to another CL, since I've just deleted > these functions. The client code will be doing the SHChangeNotify now... https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:557: static void GetAppDefaultRegistrationEntries( On 2014/09/30 10:21:12, Matt Giuca wrote: > On 2014/09/18 01:16:15, gab wrote: > > On 2014/09/15 03:49:45, Matt Giuca wrote: > > You're right that IApplicationAssociationRegistration doesn't work in Win8 as > > they changed the entire landscape. > > > > I also didn't think the registry method @ [1] worked post-XP (are you sure it > > doesn't just happen to work because there is only one program in the extension > > you're testing, making it an implicit default? > > Where on that page ([1]) does it say it doesn't work post-XP? All I can find is > that it says OpenWithList "is intended only for .exe applications on operating > systems prior to Windows XP". But we aren't using OpenWithList; we're using > OpenWithProgIds. > > It certainly does work as expected in Windows 7. > > My Windows 8 machine is out of commission at the moment (we recently moved > offices). I will set it up tomorrow. So I can't confirm your question about > making it the implicit default. But from memory, I'm fairly sure last time I > tried it out on Windows 8 I played with setting and unsetting the default in the > registry, and it worked just like in Windows 7. > > > > > In Win8 the registrations + SHCNE_ASSOCCHANGED call will most likely be enough > > to give the user a fly-in saying "a new application support file type foo", > but > > setting the default won't do anything IMO. > > As I said on the other comment, I haven't seen a fly-in and I don't see any > evidence that setting the default doesn't work. As far as I can tell, this stuff > has not changed since Windows XP or possibly even earlier than that. (However, > again, browser registration is a different story because of how special browsers > are in Windows 8.) Browsers are special indeed in that being default handler for HTTP gives super powers to the application. But I thought the act of making X default for Y was under the same restricted OS control for all Ys in Win8 (but I'm happy to be wrong, browsers may very well be even more special than I thought). I'd appreciate if you could update the API comments with your findings as you test this CL on Win8. > > > > > [1] > > http://msdn.microsoft.com/en-ca/library/windows/desktop/cc144148(v=vs.85).aspx > https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2469: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx On 2014/09/30 10:21:12, Matt Giuca wrote: > But remember I will be calling this code far more frequently than when > uninstalling Chrome. It will be whenever an app is removed, which could be > triggered by sync and could be done in bulk. I don't think that iterating over > every single key in HKEY_CLASSES_ROOT is a good idea in this situation. Far > better to keep our own index so we can delete exactly what we need. Ok fair enough, but I still think we should remove them, so this TODO should be handled shortly after landing this CL. https://codereview.chromium.org/487693002/diff/320001/chrome/browser/file_ass... File chrome/browser/file_associations_win_unittest.cc (right): https://codereview.chromium.org/487693002/diff/320001/chrome/browser/file_ass... chrome/browser/file_associations_win_unittest.cc:73: key.Open(HKEY_CURRENT_USER, L"Software\\Classes\\TestApp", KEY_READ)); On 2014/09/30 10:21:12, Matt Giuca wrote: > Now it's my turn to quote TOTT to you :) > > Episode 338: "Don't Put Logic in Tests" essentially gives this exact case as an > example. We don't want bugs in the test such as missing or doubling up slashes. > If I build the strings from the constants, my test would have the exact same > logic as the code it's testing, and if that logic builds the wrong string, the > test will still pass. > > It's best to fully write out the paths in the test even if it means duplicating > them a lot. Fair enough ;-)! https://codereview.chromium.org/487693002/diff/320001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/320001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:232: bool set_delegate_execute; On 2014/09/30 10:21:12, Matt Giuca wrote: > I'm not sure... I prefer to keep all this logic together in one place. I'm happy > to move it back if you feel strongly about it. > > Actually, if not, I need to make sure I generalize it a bit more, since it still > has "Browser.Launch" in there. Added a TODO about that. Yea, I don't see the point of having Chrome specific registrations in a generic registration method which contains an option to turn those off (which all callers but Chrome use...). https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:809: class ShellUtilRegistryTest : public testing::Test { I didn't re-review the expectation below in details, I assume the code below was moved as-is from the unittest file in patch set 16. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:921: // We don't need to delete the associations with the particular extensions It's not true (as discussed) that we should not delete them. Add TODO to delete them (i.e. add an expectation that they're not for now with a TODO to flip that expectation when the TODO in the code is fixed).
https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:202: // The unique internal name Windows will use for file associations with this instead of "unique internal name," which doesn't say much, just call it a ProgID or programmatic identifier, which has meaning to all Windows developers and can be found via search. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:214: // should contain "%1" to pass the filename as an argument. should or must? what happens if it doesn't? https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:217: // The unique internal name used by Windows 8 for this application. Distinct where possible, please use the same terminology as MSDN so that it's easy to search to find out what these things really mean. in this case, this value is the AppUserModelID (http://msdn.microsoft.com/library/windows/desktop/dd378459.aspx). https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:233: // A GUID for this application. // The CLSID for the application's DelegateExecute handler. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:234: base::string16 delegate_guid; delegate_clsid https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:307: base::string16 model_id_shell(ShellUtil::kRegClasses); DCHECK_NE(std::string16(), app_info.app_id); and other fields that need to be set to register the DEx handler https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:344: // code. DO NOT SUBMIT. DO NOT SUBMIT? https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:574: RegistryEntry::LOOK_IN_HKCU)) { is HKCU correct here? the comment above says HKCR. maybe the comment should say "HKEY_CURRENT_USER\Software\Classes\EXT" if that's what is really meant. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2438: ScopedVector<RegistryEntry> entries; DCHECK that command_line contains %1? https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.h:613: // association. It should contain "%1" to pass the filename as an argument. should or must? https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.h:618: // Returns true on success; otherwise logs to stderr and returns false. if it uses LOG macros, it isn't going to stderr on Windows. since installer util functions don't generally document their logging behavior, i suggest you remove this from the comment. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.h:624: const std::set<base::string16>& file_extensions); #include <set> https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.h:629: // with this application, as given to AddWindowsFileAssociations. All AddWindowsFileAssociations -> AddFileAssociations
please refresh the CL description as well, thanks.
Patchset #19 (id:460001) has been deleted
Hopefully that addresses all the comments. Since I have LGTMs (and didn't change the code too much), I'm going to commit now. I updated the CL description. https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... File chrome/browser/file_associations_win.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/browser/file_ass... chrome/browser/file_associations_win.cc:33: SHCNE_ASSOCCHANGED, SHCNF_IDLIST | SHCNF_FLUSHNOWAIT, NULL, NULL); Thanks for the summary. Makes sense. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:557: static void GetAppDefaultRegistrationEntries( I have completed my investigation on Windows 8, and updated the documentation accordingly. So you are partly right: Windows 8 *does* ignore the default setting in the registry (so this function has no effect). However, there is no fly-in upon SHChangeNotify. Instead, if there is only one handler, it automatically becomes the default. If there are 2 or more, it prompts the user to choose a default the first time it is opened. So for my use case, that is equivalent to calling GetAppDefaultRegistrationEntries with overwrite_existing=false. https://codereview.chromium.org/487693002/diff/220001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2469: // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144148.aspx On 2014/10/01 14:31:11, gab wrote: > On 2014/09/30 10:21:12, Matt Giuca wrote: > > But remember I will be calling this code far more frequently than when > > uninstalling Chrome. It will be whenever an app is removed, which could be > > triggered by sync and could be done in bulk. I don't think that iterating over > > every single key in HKEY_CLASSES_ROOT is a good idea in this situation. Far > > better to keep our own index so we can delete exactly what we need. > > Ok fair enough, but I still think we should remove them, so this TODO should be > handled shortly after landing this CL. Acknowledged. https://codereview.chromium.org/487693002/diff/320001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/320001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:232: bool set_delegate_execute; OK, all this code has moved back to where it originally was, in GetChromeProgIdEntries. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:202: // The unique internal name Windows will use for file associations with this On 2014/10/01 16:28:38, grt wrote: > instead of "unique internal name," which doesn't say much, just call it a ProgID > or programmatic identifier, which has meaning to all Windows developers and can > be found via search. Done. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:214: // should contain "%1" to pass the filename as an argument. On 2014/10/01 16:28:38, grt wrote: > should or must? what happens if it doesn't? Then the filename won't get passed as an argument. This still qualifies as "should" in my mind, because it still works if you don't do it, it just might not do what you want. Reworded slightly. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:217: // The unique internal name used by Windows 8 for this application. Distinct On 2014/10/01 16:28:38, grt wrote: > where possible, please use the same terminology as MSDN so that it's easy to > search to find out what these things really mean. in this case, this value is > the AppUserModelID > (http://msdn.microsoft.com/library/windows/desktop/dd378459.aspx). Done. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:233: // A GUID for this application. On 2014/10/01 16:28:38, grt wrote: > // The CLSID for the application's DelegateExecute handler. Done. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:234: base::string16 delegate_guid; On 2014/10/01 16:28:38, grt wrote: > delegate_clsid Done. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:307: base::string16 model_id_shell(ShellUtil::kRegClasses); On 2014/10/01 16:28:38, grt wrote: > DCHECK_NE(std::string16(), app_info.app_id); > and other fields that need to be set to register the DEx handler No longer applies (reverted all this code). https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:344: // code. DO NOT SUBMIT. It just means I'm planning to do this before submitting. (There's a presubmit error that prevents the CQ from submitting it if it contains the string "DO NOT SUBMIT" to make sure I remember to do it.) https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:574: RegistryEntry::LOOK_IN_HKCU)) { On 2014/10/01 16:28:38, grt wrote: > is HKCU correct here? the comment above says HKCR. maybe the comment should say > "HKEY_CURRENT_USER\Software\Classes\EXT" if that's what is really meant. Done. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2438: ScopedVector<RegistryEntry> entries; I don't know if we want that. It's still a valid file association, it just probably won't do what you want. It doesn't feel right for this code to enforce that your command line conforms to a particular pattern. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.h:613: // association. It should contain "%1" to pass the filename as an argument. See discussion in .cc file. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.h:618: // Returns true on success; otherwise logs to stderr and returns false. On 2014/10/01 16:28:39, grt wrote: > if it uses LOG macros, it isn't going to stderr on Windows. since installer util > functions don't generally document their logging behavior, i suggest you remove > this from the comment. Done. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.h:624: const std::set<base::string16>& file_extensions); On 2014/10/01 16:28:38, grt wrote: > #include <set> Done. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.h:629: // with this application, as given to AddWindowsFileAssociations. All On 2014/10/01 16:28:39, grt wrote: > AddWindowsFileAssociations -> AddFileAssociations Done. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:809: class ShellUtilRegistryTest : public testing::Test { Correct. https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util_unittest.cc:921: // We don't need to delete the associations with the particular extensions On 2014/10/01 14:31:11, gab wrote: > It's not true (as discussed) that we should not delete them. > > Add TODO to delete them (i.e. add an expectation that they're not for now with a > TODO to flip that expectation when the TODO in the code is fixed). Done.
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/487693002/520001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/487693002/520001
Message was sent while issue was closed.
Committed patchset #21 (id:520001) as 9b831d06baa5c0b0b4f20840c92fdbda41fe3126
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/33dfd20c12513837f23d41452c7f8cfdec9a9e6a Cr-Commit-Position: refs/heads/master@{#297814}
Message was sent while issue was closed.
Awesome, thanks. The result of your Win8 investigation makes sense to me. I guess the fly-in is only for a few types then (e.g., HTTP handlers). Please address the two post-commit nits below in a follow-up CL. Thanks! Gab https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/440001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:344: // code. DO NOT SUBMIT. On 2014/10/02 09:36:34, Matt Giuca wrote: > It just means I'm planning to do this before submitting. (There's a presubmit > error that prevents the CQ from submitting it if it contains the string "DO NOT > SUBMIT" to make sure I remember to do it.) Nice, didn't know about this trick, I was using a more obscure one which prevents // not followed by a space, e.g. //FIXME: blah blah is (or at least was -- I've noticed it not work sometimes) caught by presubmit. I'll switch to using this, thanks! PS: Any idea why the code for it [1] uses 'DO NOT ''SUBMIT' rather than simply 'DO NOT SUBMIT'? This would explain why I missed it when doing a quick lookup for it yesterday. [1] https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/pres... https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:311: // user readable/localized strings. See relevant MSDN article: s/user/use https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:353: DCHECK_NE(L'.', app_info.prog_id[0]); Another restriction worth enforcing here and in the API comments is that ProgIds cannot be longer than 39 characters: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u... This was previously enforced by GetBrowserProgId() but now only Chrome uses that and other users of this API need to be told about this restriction.
Message was sent while issue was closed.
https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:311: // user readable/localized strings. See relevant MSDN article: I believe it's trying to say "user-readable", not "use readable". (A hyphen would be useful, but it's really not worth a CL to fix this.) (Note I didn't write this or even touch it in this patch so I'm not sure why it came up in this review.) https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:353: DCHECK_NE(L'.', app_info.prog_id[0]); Actually, I looked into this yesterday (since I found your comment about it in GetBrowserProgId) and considered adding a check here. But I investigated your link: http://msdn.microsoft.com/en-us/library/aa911706.aspx this documentation is for Windows Mobile 6.5. The "real" documentation for ProgID on desktop Windows platforms is here: http://msdn.microsoft.com/en-us/library/windows/desktop/cc144152.aspx and it doesn't mention the 39-character restriction at all. I tried on Windows 7 with a 78-character ProgID and everything worked as expected. So I think this was a Windows Mobile only restriction. We probably should remove this check from GetBrowserProgId unless you can find supporting evidence that desktop Windows has a problem with it. (Coincidentally, the ProgIDs I am using for Chrome apps are *exactly* 39 characters long: "chrome-" + 32-char AppID!)
Message was sent while issue was closed.
More details below; I do think this check is worthwhile (and in fact you'll need to find a way to make your progids 38 characters, 39 with the NULL char...); sorry looks like I failed to update all our code regarding this weird requirement when I fixed the issue mentioned below 2 years ago.. PS: I'll be OOO for a while after today, grt will be your man in the mean time (but please CC me on all CLs so that I can easily catch up when I'm back). Cheers! Gab https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:311: // user readable/localized strings. See relevant MSDN article: On 2014/10/03 00:26:40, Matt Giuca wrote: > I believe it's trying to say "user-readable", not "use readable". (A hyphen > would be useful, but it's really not worth a CL to fix this.) > > (Note I didn't write this or even touch it in this patch so I'm not sure why it > came up in this review.) Ah it's /"user readable"/"localized strings/; I was reading it as /use "readable"/"localized" strings/. All good :-)! https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:353: DCHECK_NE(L'.', app_info.prog_id[0]); On 2014/10/03 00:26:40, Matt Giuca wrote: > Actually, I looked into this yesterday (since I found your comment about it in > GetBrowserProgId) and considered adding a check here. > > But I investigated your link: > http://msdn.microsoft.com/en-us/library/aa911706.aspx > this documentation is for Windows Mobile 6.5. > > The "real" documentation for ProgID on desktop Windows platforms is here: > http://msdn.microsoft.com/en-us/library/windows/desktop/cc144152.aspx > and it doesn't mention the 39-character restriction at all. > > I tried on Windows 7 with a 78-character ProgID and everything worked as > expected. So I think this was a Windows Mobile only restriction. We probably > should remove this check from GetBrowserProgId unless you can find supporting > evidence that desktop Windows has a problem with it. > > (Coincidentally, the ProgIDs I am using for Chrome apps are *exactly* 39 > characters long: "chrome-" + 32-char AppID!) So I just dug further into this since I remembered it being an issue: here's the relevant issue crbug.com/153349. Turns out the 39 characters restriction INCLUDES the NULL character (so your progids won't work)... They *will* work in most cases, but see issue 153349 for a Win8 use case were a weird Chromium-only issue was caused by this. The documentation on this is unclear... but as is typical with these APIs, experimentation often trumps documentation.. Looks like I forgot to fix GetBrowserProgId in that CL in fact so it's wrong, it should only allow progids of 38 characters (39 with the NULL char); can you please update that method/documentation in your follow up CL as well? Thanks!
Message was sent while issue was closed.
https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/487693002/diff/520001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:353: DCHECK_NE(L'.', app_info.prog_id[0]); On 2014/10/03 15:26:25, gab wrote: > On 2014/10/03 00:26:40, Matt Giuca wrote: > > Actually, I looked into this yesterday (since I found your comment about it in > > GetBrowserProgId) and considered adding a check here. > > > > But I investigated your link: > > http://msdn.microsoft.com/en-us/library/aa911706.aspx > > this documentation is for Windows Mobile 6.5. > > > > The "real" documentation for ProgID on desktop Windows platforms is here: > > http://msdn.microsoft.com/en-us/library/windows/desktop/cc144152.aspx > > and it doesn't mention the 39-character restriction at all. > > > > I tried on Windows 7 with a 78-character ProgID and everything worked as > > expected. So I think this was a Windows Mobile only restriction. We probably > > should remove this check from GetBrowserProgId unless you can find supporting > > evidence that desktop Windows has a problem with it. > > > > (Coincidentally, the ProgIDs I am using for Chrome apps are *exactly* 39 > > characters long: "chrome-" + 32-char AppID!) > > So I just dug further into this since I remembered it being an issue: here's the > relevant issue crbug.com/153349. Turns out the 39 characters restriction > INCLUDES the NULL character (so your progids won't work)... > > They *will* work in most cases, but see issue 153349 for a Win8 use case were a > weird Chromium-only issue was caused by this. The documentation on this is > unclear... but as is typical with these APIs, experimentation often trumps > documentation.. > > Looks like I forgot to fix GetBrowserProgId in that CL in fact so it's wrong, it > should only allow progids of 38 characters (39 with the NULL char); can you > please update that method/documentation in your follow up CL as well? > > Thanks! ping; just back from a month leave, has anything been done to fix this?
Message was sent while issue was closed.
I was planning to leave these follow-ups till after branch point. I have a lot of things that need attention before then. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
