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

RESOLVED FIXED in Firefox 16

Status

Firefox Graveyard
Web Apps
P1
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: TimAbraldes, Assigned: Dan Walkowski)

Tracking

Trunk
Firefox 16
x86
Mac OS X
Dependency tree / graph
Bug Flags:
in-moztrap -

Details

(Whiteboard: [blocking-webrtdesktop1+], [qa-])

Attachments

(1 attachment, 7 obsolete attachments)

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.

Updated

5 years ago
blocking-kilimanjaro: --- → ?

Updated

5 years ago
blocking-kilimanjaro: ? → +
Assignee: nobody → dwalkowski
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.
(Assignee)

Comment 3

5 years ago
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.
Priority: -- → P1
Whiteboard: [blocking-webrtdesktop1+]
Target Milestone: --- → Firefox 15
(Assignee)

Comment 4

5 years ago
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 on attachment 628983 [details] [diff] [review]
hypothetical patch

Josh: can you provide feedback on this approach?
Attachment #628983 - Attachment is patch: true
Attachment #628983 - Flags: feedback?(joshmoz)

Comment 6

5 years ago
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++).
Attachment #628983 - Flags: feedback?(joshmoz) → feedback+
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 629284 [details] [diff] [review]
proposed patch

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

Updated

5 years ago
Attachment #629284 - Attachment is patch: true
Attachment #629284 - Flags: review?(joshmoz)

Comment 9

5 years ago
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?
Attachment #629284 - Flags: review?(joshmoz)
(Assignee)

Comment 10

5 years ago
Created attachment 631134 [details]
complete working patch with tests

Please review
(Assignee)

Comment 11

5 years ago
above patch is just for /widget/cocoa
(Assignee)

Comment 12

5 years ago
Created attachment 631171 [details] [diff] [review]
/widget/cocoa patches only

Please review
Attachment #628983 - Attachment is obsolete: true
Attachment #629284 - Attachment is obsolete: true
Attachment #631134 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 762698
Attachment #631171 - Attachment is patch: true
(Assignee)

Updated

5 years ago
Attachment #631171 - Flags: review?(joshmoz)

Comment 13

5 years ago
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.
Attachment #631171 - Flags: review?(joshmoz)
(Assignee)

Comment 14

5 years ago
Created attachment 631450 [details] [diff] [review]
/widget/cocoa patches only

Comment removed.
Attachment #631171 - Attachment is obsolete: true
Attachment #631450 - Flags: review?(joshmoz)
(Assignee)

Updated

5 years ago
Attachment #631450 - Flags: review?(joshmoz)

Updated

5 years ago
Attachment #631450 - Attachment is patch: true

Comment 15

5 years ago
Daniel - are you only posting part of a patch, relative to an old version? Can you post the full patch?
(Assignee)

Comment 16

5 years ago
Created attachment 631465 [details] [diff] [review]
/widget/cocoa patch only

removed comment, corrected diff
Attachment #631450 - Attachment is obsolete: true
Attachment #631465 - Flags: review?(joshmoz)
(Assignee)

Comment 17

5 years ago
Created attachment 631473 [details] [diff] [review]
/widget/cocoa patch only

previous diff, while technically correct, had strange formatting
Attachment #631465 - Attachment is obsolete: true
Attachment #631465 - Flags: review?(joshmoz)
Attachment #631473 - Flags: review?(joshmoz)

Comment 18

5 years ago
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?
(Assignee)

Comment 19

5 years ago
Created attachment 631515 [details] [diff] [review]
/widget/cocoa patch only, removed comment

Now with proper formatting -and- the comment removed.  Apologies.
Attachment #631473 - Attachment is obsolete: true
Attachment #631473 - Flags: review?(joshmoz)
Attachment #631515 - Flags: review?(joshmoz)

Comment 20

5 years ago
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.
Attachment #631515 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

5 years ago
Blocks: 763740
Target Milestone: Firefox 15 → Firefox 16
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
https://hg.mozilla.org/mozilla-central/rev/50cd7af72891
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Summary: `mozApps.getInstalled()` should return only the apps that are natively installed - Mac → Build prereq code libraries to allow for getting apps that are only natively installed on Mac
Whiteboard: [blocking-webrtdesktop1+] → [blocking-webrtdesktop1+], [qa-]
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.
Summary: Build prereq code libraries to allow for getting apps that are only natively installed on Mac → Implement MacWebAppUtils to allow callers to locate and manipulate native webapps on Mac

Updated

5 years ago
Flags: in-moztrap-

Updated

5 years ago
QA Contact: jsmith
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.