Last Comment Bug 756308 - Implement MacWebAppUtils to allow callers to locate and manipulate native webapps on Mac
: Implement MacWebAppUtils to allow callers to locate and manipulate native web...
Status: RESOLVED FIXED
[blocking-webrtdesktop1+], [qa-]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal
: Firefox 16
Assigned To: Dan Walkowski
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks: 749033 762698 763740
  Show dependency treegraph
 
Reported: 2012-05-17 16:37 PDT by Tim Abraldes [:TimAbraldes] [:tabraldes]
Modified: 2016-02-04 15:00 PST (History)
9 users (show)
jsmith: in‑moztrap-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
hypothetical patch (7.18 KB, patch)
2012-05-31 17:10 PDT, Dan Walkowski
jaas: feedback+
Details | Diff | Review
proposed patch (2.92 KB, patch)
2012-06-01 11:59 PDT, Dan Walkowski
no flags Details | Diff | Review
complete working patch with tests (10.23 KB, text/plain)
2012-06-07 14:16 PDT, Dan Walkowski
no flags Details
/widget/cocoa patches only (9.09 KB, patch)
2012-06-07 15:17 PDT, Dan Walkowski
no flags Details | Diff | Review
/widget/cocoa patches only (8.98 KB, patch)
2012-06-08 10:33 PDT, Dan Walkowski
no flags Details | Diff | Review
/widget/cocoa patch only (8.34 KB, patch)
2012-06-08 10:58 PDT, Dan Walkowski
no flags Details | Diff | Review
/widget/cocoa patch only (8.35 KB, patch)
2012-06-08 11:26 PDT, Dan Walkowski
no flags Details | Diff | Review
/widget/cocoa patch only, removed comment (8.24 KB, patch)
2012-06-08 13:27 PDT, Dan Walkowski
jaas: review+
Details | Diff | Review

Description Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-17 16:37:26 PDT
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] 2012-05-23 17:11:53 PDT
Dan, can you please give us some feasibility analysis?
Comment 2 Jason Smith [:jsmith] 2012-05-23 23:35:05 PDT
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 2012-05-24 14:43:59 PDT
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 2012-05-31 17:10:24 PDT
Created attachment 628983 [details] [diff] [review]
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] 2012-05-31 17:25:20 PDT
Comment on attachment 628983 [details] [diff] [review]
hypothetical patch

Josh: can you provide feedback on this approach?
Comment 6 Josh Aas 2012-05-31 18:07:55 PDT
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 2012-06-01 10:21:11 PDT
(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 2012-06-01 11:59:35 PDT
Created attachment 629284 [details] [diff] [review]
proposed patch

Made changes suggested by Josh.
This one might be close to correct, please advise.
Comment 9 Josh Aas 2012-06-01 13:16:48 PDT
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 2012-06-07 14:16:23 PDT
Created attachment 631134 [details]
complete working patch with tests

Please review
Comment 11 Dan Walkowski 2012-06-07 15:08:48 PDT
above patch is just for /widget/cocoa
Comment 12 Dan Walkowski 2012-06-07 15:17:06 PDT
Created attachment 631171 [details] [diff] [review]
/widget/cocoa patches only

Please review
Comment 13 Josh Aas 2012-06-08 07:19:22 PDT
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 2012-06-08 10:33:30 PDT
Created attachment 631450 [details] [diff] [review]
/widget/cocoa patches only

Comment removed.
Comment 15 Josh Aas 2012-06-08 10:50:30 PDT
Daniel - are you only posting part of a patch, relative to an old version? Can you post the full patch?
Comment 16 Dan Walkowski 2012-06-08 10:58:21 PDT
Created attachment 631465 [details] [diff] [review]
/widget/cocoa patch only

removed comment, corrected diff
Comment 17 Dan Walkowski 2012-06-08 11:26:45 PDT
Created attachment 631473 [details] [diff] [review]
/widget/cocoa patch only

previous diff, while technically correct, had strange formatting
Comment 18 Ed Lee :Mardak 2012-06-08 11:46:44 PDT
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 2012-06-08 13:27:38 PDT
Created attachment 631515 [details] [diff] [review]
/widget/cocoa patch only, removed comment

Now with proper formatting -and- the comment removed.  Apologies.
Comment 20 Josh Aas 2012-06-08 14:09:09 PDT
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 me!) 2012-06-20 18:41:40 PDT
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] 2012-06-21 04:03:29 PDT
https://hg.mozilla.org/mozilla-central/rev/50cd7af72891
Comment 23 Jason Smith [:jsmith] 2012-06-21 07:31:41 PDT
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 me!) 2012-06-21 15:14:32 PDT
Yes, nothing to verify here. Let me update the bug title to be a bit more descriptive of what we implemented.

Note You need to log in before you can comment on or make changes to this bug.