756308 - Implement MacWebAppUtils to allow callers to locate and manipulate native webapps on Mac

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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.

Comment 1

[Bill Walker [:bwalker] [@wfwalker]]

Dan, can you please give us some feasibility analysis?

Comment 2

[Jason Smith [:jsmith]]

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.

Comment 3

[Dan Walkowski]

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.

Comment 4

[Dan Walkowski]

Attachment: hypothetical patch

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

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 628983 [details] [diff] [review]
hypothetical patch

Josh: can you provide feedback on this approach?

Comment 6

[Josh Aas]

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++).

Comment 7

[Dan Walkowski]

(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.

Comment 8

[Dan Walkowski]

Attachment: proposed patch

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

Comment 9

[Josh Aas]

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?

Comment 10

[Dan Walkowski]

Attachment: complete working patch with tests

Please review

Comment 11

[Dan Walkowski]

above patch is just for /widget/cocoa

Comment 12

[Dan Walkowski]

Attachment: /widget/cocoa patches only

Please review

Comment 13

[Josh Aas]

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 14

[Dan Walkowski]

Attachment: /widget/cocoa patches only

Comment removed.

Comment 15

[Josh Aas]

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

Comment 16

[Dan Walkowski]

Attachment: /widget/cocoa patch only

removed comment, corrected diff

Comment 17

[Dan Walkowski]

Attachment: /widget/cocoa patch only

previous diff, while technically correct, had strange formatting

Comment 18

[Ed Lee :Mardak]

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?

Comment 19

[Dan Walkowski]

Attachment: /widget/cocoa patch only, removed comment

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

Comment 20

[Josh Aas]

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.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

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

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/50cd7af72891

Comment 23

[Jason Smith [:jsmith]]

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?

Comment 24

[:Felipe Gomes (needinfo for replies!)]

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