See discussion in bug 749033. Marketplaces need to be able to distinguish between 3 states: App has not been purchased/acquired by this user App has been purchased/acquired by this user but it is not installed on this machine App is natively installed on the device the user is currently using Marketplaces are aware of whether an app has been purchased/acquired by a particular user because they can look at the user's purchase history. The only missing piece is the ability to tell whether an app is currently installed on the user's machine.

Dan, can you please give us some feasibility analysis?

Opinion on Priority and Milestone: Priority: P1, blocker Milestone: Firefox 15 Reasoning: This part of the blocker bug attached - which is already a blocker for the release. Windows is a primary operating system to be supported. Therefore, this blocks.

The cheap and easy way is to check to see if the app we are looking for is in the Applications folder. This can and will generate false negatives, since apps can be moved to any location by users. The OS, while I don't believe there is a getAllApps() call, there is a haveYouGotAnAppWithThisIdentifier() call. So we could walk the list of purchased apps, and check to see which are actually installed. This would give fully correct results. If necessary for updates, the location of the app with a specific signature can also be found.

This is not committed, or even likely to compile. It's the approach and suggestions and obvious errors I'd like comments about.

Comment on attachment 628983 [details] [diff] [review] hypothetical patch Josh: can you provide feedback on this approach?

Comment on attachment 628983 [details] [diff] [review] hypothetical patch Review of attachment 628983 [details] [diff] [review]: ----------------------------------------------------------------- Generally this looks good, most of my comments are style and nits. My other suggestion would be to think about coming up with a more general utility name for the interface so we can put more stuff in it. I don't really want to build up a collection of one-method interfaces eventually. ::: widget/cocoa/nsMacWebAppUtils.h @@ +1,3 @@ > +/* -*- Mode: c++; tab-width: 2; indent-tabs-mode: nil; -*- */ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 I'm pretty sure we're suppose to use MPL 2 license headers in new files. This happens in all your new files. ::: widget/cocoa/nsMacWebAppUtils.mm @@ +41,5 @@ > + > + > +//find the path to the app with the given signature, if any. > +// note that the OS will return the path to the newest binary, if there is only one. > +// the determination of 'newest' is complex and beyond the scope of this comment Please use capital letters to start a sentence and ending punctuation as you would in other writing. Nit-picky, I know! @@ +48,5 @@ > + NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT; > + NS_ENSURE_ARG_POINTER(path); > + > + NSAutoreleasePool* ap = [[NSAutoreleasePool alloc] init]; > + nsresult rez = NS_OK; It's standard in Gecko code to call this variable "rv". Doing so will make the pattern here more recognizable quickly. Also given how you use this variable later you might want to init to a failure code and assign on success to avoid an "else" statement later. @@ +54,5 @@ > + //note that the result of this expression might be nil, meaning no matching app was found. > + NSString* temp = [[NSWorkspace sharedWorkspace] absolutePathForAppBundleWithIdentifier:(NSString*)signature]; > + if (temp != nil) > + { > + //copy out the result into pathif non-nil Missing a space in "pathif". @@ +57,5 @@ > + { > + //copy out the result into pathif non-nil > + GetStringForNSString(temp, path); > + } > + else rez = NS_FAILED; Need to follow Cocoa widget style here: if (temp) { ... } else { ... } Always brackets, no explicit nil comparison. ::: widget/nsIMacWebAppUtils.idl @@ +46,5 @@ > +{ > + /** > + * Find the path for an app with the given signature. > + */ > + nsresult pathForAppWithSignature(CFStringRef signature, nsAString& path); You can't use a native string type here - how would JS pass that in? You need to pass in an XPCOM string and convert to CFStringRef (or whatever native type is most convenient) in the method. And what exactly is a "signature" in this context? To be clear - you could use a native type here but you'd have to make this not scriptable (restricted to C++).

(In reply to Josh Aas (Mozilla Corporation) from comment #6) Thanks Josh. I will make those mods and post another patch. As for the use of a separate object, there is one more webapp method that definitely needs to go in here, and probably more. This one is just the most important at the moment.

Made changes suggested by Josh. This one might be close to correct, please advise.

Comment on attachment 629284 [details] [diff] [review] proposed patch Review of attachment 629284 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsMacWebAppUtils.h @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsIMacWebAppUtils.h" > +#include "nsCOMPtr.h" > +#include "nsString.h" These includes shouldn't be necessary except for nsIMacWebAppUtils.h. If you need them in the impl file put them there, if you need them in the interface (idl file) put them there. Don't want unnecessary includes in header files. ::: widget/cocoa/nsMacWebAppUtils.mm @@ +9,5 @@ > + > + > +//Find the path to the app with the given signature, if any. > +//Note that the OS will return the path to the newest binary, if there is more than one. > +//The determination of 'newest' is complex and beyond the scope of this comment. I prefer spaces after the comment start. Happens other places too. @@ +14,5 @@ > + > +NS_IMETHODIMP nsMacWebAppUtils::PathForAppWithSignature(nsAString& signature, nsAString& outPath) > +{ > + NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT; > + NS_ENSURE_ARG_POINTER(outPath); 'outPath' isn't really a pointer, is it? Any point to checking this? @@ +24,5 @@ > + NSString* temp = [[NSWorkspace sharedWorkspace] absolutePathForAppBundleWithIdentifier: > + [NSString stringWithCharacters:signature.get() length:signature.Length()]]; > + > + if (temp) > + { Bracket on the same line as the "if" statement, see example from my last review. ::: widget/nsIMacWebAppUtils.idl @@ +13,5 @@ > +[scriptable, uuid(398373c8-0467-4ffa-b386-dba3930da193)] > +interface nsIMacWebAppUtils : nsISupports > +{ > + /** > + * Find the path for an app with the given signature. This would be a good place to explain what a signature is. How are you squaring this term "signature" with the fact that I think what you're passing is called a "bundle identifier" in Mac OS X terms? Perhaps you should use the same terms?

Please review

above patch is just for /widget/cocoa

Please review

Comment on attachment 631171 [details] [diff] [review] /widget/cocoa patches only Review of attachment 631171 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsMacWebAppUtils.h @@ +8,4 @@ > > +#define NS_MACWEBAPPUTILS_CONTRACTID "@mozilla.org/widget/mac-web-app-utils;1" > +#define NS_MACWEBAPPUTILS_COMPONENT_CLASSNAME "Mac Web Application Utils" > +// define NS_MACWEBAPPUTILS_CID { 0xe9096367, 0xddd9, 0x45e4, { 0xb7, 0x62, 0x49, 0xc0, 0xc1, 0x8b, 0x71, 0x19 }} Why is this commented-out line here? Isn't this a new file? Can you please post the full patch? I don't want to review an interdiff.

Comment removed.

Daniel - are you only posting part of a patch, relative to an old version? Can you post the full patch?

removed comment, corrected diff

previous diff, while technically correct, had strange formatting

Dan, how are you generating these patches? The latest one seems to correctly treat nsMacWebAppUtils.h as a new file as opposed to a diff/patch, but it has the commented out NS_MACWEBAPPUTILS_CID again. Are you using mq?

Now with proper formatting -and- the comment removed. Apologies.

Comment on attachment 631515 [details] [diff] [review] /widget/cocoa patch only, removed comment Review of attachment 631515 [details] [diff] [review]: ----------------------------------------------------------------- If there was some way to avoid repeating that new CID a second time that would be great, but I don't know what that is. ::: widget/cocoa/nsMacWebAppUtils.mm @@ +24,5 @@ > + NSAutoreleasePool* ap = [[NSAutoreleasePool alloc] init]; > + > + //note that the result of this expression might be nil, meaning no matching app was found. > + NSString* temp = [[NSWorkspace sharedWorkspace] absolutePathForAppBundleWithIdentifier: > + [NSString stringWithCharacters:((nsString)bundleIdentifier).get() length:((nsString)bundleIdentifier).Length()]]; This could be a little more clear - if you must cast then you could do it once, in its own stack variable. And/Or put the result of stringWithCharacters into its own stack variable. If you don't do something like that this is a pretty ugly long line.

I changed the test to look for TextEdit instead of Firefox because it's not present in the build machines (only Nightly), and set the test to only run on Mac: https://hg.mozilla.org/integration/mozilla-inbound/rev/50cd7af72891

Changing the name to reflect what I understand has landed. I think there's no end-user impact in this patch (it's the follow-up patch that has it), so no verification is needed. Felipe - Can you confirm?

Yes, nothing to verify here. Let me update the bug title to be a bit more descriptive of what we implemented.