Last Comment Bug 739636 - Install web app on host OS - Mac
: Install web app on host OS - Mac
Status: VERIFIED FIXED
[marketplace-beta]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal
: Firefox 14
Assigned To: :Felipe Gomes (needinfo me!)
: Jason Smith [:jsmith]
Mentors:
Depends on: 731541
Blocks: 745924 746771
  Show dependency treegraph
 
Reported: 2012-03-27 08:56 PDT by :Felipe Gomes (needinfo me!)
Modified: 2016-02-04 15:00 PST (History)
10 users (show)
jsmith: in‑moztrap+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (6.99 KB, patch)
2012-03-27 08:56 PDT, :Felipe Gomes (needinfo me!)
mstange: review+
Details | Diff | Review
Diff with changes (2.46 KB, patch)
2012-04-04 15:13 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Review
Patch v2 (7.54 KB, patch)
2012-04-10 02:42 PDT, :Felipe Gomes (needinfo me!)
mstange: review+
Details | Diff | Review
Patch v3 (7.59 KB, patch)
2012-04-16 15:23 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Review
Standalone mac patch (16.34 KB, patch)
2012-04-16 16:57 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Review
Mac standalone patch w/fixes (16.34 KB, patch)
2012-04-17 07:29 PDT, Myk Melez [:myk] [@mykmelez]
myk: review+
myk: checkin+
Details | Diff | Review

Description :Felipe Gomes (needinfo me!) 2012-03-27 08:56:40 PDT
Created attachment 609738 [details] [diff] [review]
Patch

Mac version of webapp install process. Equivalent to the Windows version at bug 731541 and is built on top of that patch
Comment 1 :Felipe Gomes (needinfo me!) 2012-03-27 13:46:36 PDT
I should note that trying this current patch with a pull from the github tree will fail at the moment because the webapprt binary is not being built or included in the firefox dist directory. Creating a blanking file with that name will make it to work
Comment 2 Myk Melez [:myk] [@mykmelez] 2012-03-27 13:50:10 PDT
Felipe: if you change to your objdir and do |make -C webapprt| it should get built.  I'm not sure why it isn't being built by default, as it's supposed to be.
Comment 3 :Felipe Gomes (needinfo me!) 2012-03-28 02:26:41 PDT
Markus, could you take a look at this patch? It is part of the native webapp integration, and it's meant to create a .app bundle that will launch a registered web app using the webapprt (bug 725408).

The patch is similar (and built on top of) the Windows version from bug 731541

The installation flow is described here: https://etherpad.mozilla.org/webapprt-install-flow  (jump to the Mac Native section)
Comment 4 Markus Stange [:mstange] 2012-03-28 03:22:33 PDT
Comment on attachment 609738 [details] [diff] [review]
Patch

Looks good. I'm a bit sad about the blocking nature of the IO and sips process invocation here, but I guess that's because we don't have a nice async file API yet, right?

>diff --git a/browser/modules/WebappsInstaller.jsm b/browser/modules/WebappsInstaller.jsm

>+  _createDirectoryStructure: function() {
>+    this.appProfileDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0777);
>+    this.installDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0777);
>+    this.contentsDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0777);
>+    this.macOSDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0777);
>+    this.resourcesDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0777);
>+  },

Is 0777 a good choice here? Most apps in my /Applications/ folder only have the writable bit set for the owner, but I don't know enough in this area to tell whether using 0777 has any negative effects.

>+    let configJson = this.appProfileDir.clone();
>+    configJson.append("webapp.json");
>+    writeToFile(configJson, JSON.stringify(json), function() {});

Will there be any kind of progress / throbber UI during the installation? I think it's a bit strange not to care about the finish callback here.

>+      process.run(true, ["-s",
>+                  "format", "icns",
>+                  aIcon.path,
>+                  "--out", this.iconFile.path,
>+                  "-z", "128", "128"],
>+                  9);

I think this should work without the 9. The IDL definition has a size_is(count) annotation on the args parameter, so the count should be filled in automatically. But please test anyway.
Comment 5 :Felipe Gomes (needinfo me!) 2012-04-04 15:13:18 PDT
Created attachment 612357 [details] [diff] [review]
Diff with changes

This is a diff to be applied on top of the previous patch that implements the recent config changes.

I still want to do some clean-up like moving the config.json creation out of the platform-specific part. But this works.

Note that on this patch I'm also including the FirefoxBinary key in the .plist file that I was using to test.
Comment 6 :Felipe Gomes (needinfo me!) 2012-04-10 02:42:58 PDT
Created attachment 613528 [details] [diff] [review]
Patch v2

Hi Markus, this is a new version of the patch with just some minimal changes from the previous one, mostly due to changes in the format of the json and .ini file that need to be created.
I also changed the folder permissions as you said to match the existing folder permissions which is 0755

Attached is the full patch but here's an interdiff to make it simple to review these latest changes: http://pastebin.mozilla.org/1561984

The other extra change is checking if the profile folder was already created and, if so, don't fail trying to create it again. This can happen if an app was uninstalled (by throwing it at the Trash) and then reinstalled.
Comment 7 :Felipe Gomes (needinfo me!) 2012-04-10 02:53:25 PDT
Also replying to your previous questions:

(In reply to Markus Stange from comment #4)
> Looks good. I'm a bit sad about the blocking nature of the IO and sips
> process invocation here, but I guess that's because we don't have a nice
> async file API yet, right?

Yeah, and also because most of these operations are simple operations. There is NetUtil.asyncCopy but that is more focused towards streams and for files it would read the entire file to copy, as opposed to calling .copyTo which should just call the OS file copy. So I believe this is a net win. The only real sync prob here is the .ini writer but that can be improved later.

> 
> Is 0777 a good choice here? Most apps in my /Applications/ folder only have
> the writable bit set for the owner, but I don't know enough in this area to
> tell whether using 0777 has any negative effects.

changed to 0755 which matches the other apps in my /Applications/ folder

> 
> >+    let configJson = this.appProfileDir.clone();
> >+    configJson.append("webapp.json");
> >+    writeToFile(configJson, JSON.stringify(json), function() {});
> 
> Will there be any kind of progress / throbber UI during the installation? I
> think it's a bit strange not to care about the finish callback here.

Not in our UI, no, but the mozApps.install is an async operation that calls a callback at the end of this process, and the Marketplace webpage can choose to display progress UI if wanted.
I don't care about this callback here because there are other operations, like the icon generation, that presumably take longer and the .json file will most likely be ready by then. I can make it 100% accurate later but there is no great impact not doing it.

> 
> >+      process.run(true, ["-s",
> >+                  "format", "icns",
> >+                  aIcon.path,
> >+                  "--out", this.iconFile.path,
> >+                  "-z", "128", "128"],
> >+                  9);
> 
> I think this should work without the 9. The IDL definition has a
> size_is(count) annotation on the args parameter, so the count should be
> filled in automatically. But please test anyway.

I tried without that parameter and it fails, nsIProcess.run says there were not enoug args
Comment 8 Markus Stange [:mstange] 2012-04-10 06:49:12 PDT
Comment on attachment 613528 [details] [diff] [review]
Patch v2

>+  install: function() {
>+    this._removeInstallation(true);
>+    try {
>+      this._createDirectoryStructure();
>+      this._copyPrebuiltFiles();
>+      this._createConfigFiles();
>+    } catch (ex) {
>+      this._removeInstallation();

Pass false explicitly here.

> > > 
> > >+      process.run(true, ["-s",
> > >+                  "format", "icns",
> > >+                  aIcon.path,
> > >+                  "--out", this.iconFile.path,
> > >+                  "-z", "128", "128"],
> > >+                  9);
> > 
> > I think this should work without the 9. The IDL definition has a
> > size_is(count) annotation on the args parameter, so the count should be
> > filled in automatically. But please test anyway.
> 
> I tried without that parameter and it fails, nsIProcess.run says there were
> not enoug args

okay
Comment 9 :Felipe Gomes (needinfo me!) 2012-04-16 15:23:35 PDT
Created attachment 615497 [details] [diff] [review]
Patch v3

Just changed the binary name from webapprt to webapprt-stub
Comment 10 :Felipe Gomes (needinfo me!) 2012-04-16 16:57:28 PDT
Created attachment 615541 [details] [diff] [review]
Standalone mac patch

This patch version contains all the pieces necessary for the Mac installer and doesn't depend on applying the Windows installer patch first
Comment 11 Markus Stange [:mstange] 2012-04-17 01:24:01 PDT
By the way, these updated patches don't address comment 8. (In case it was unclear, I'd like this._removeInstallation() to be replaced by this._removeInstallation(false).)
Comment 12 :Felipe Gomes (needinfo me!) 2012-04-17 01:32:36 PDT
Sorry Markus, I missed that comment. I'll make this change very soon in a follow-up if necessary as I believe Myk is already preparing to land the patch together with the webapp runtime, as it got a green tryserver run.
Comment 13 Myk Melez [:myk] [@mykmelez] 2012-04-17 07:29:32 PDT
Created attachment 615705 [details] [diff] [review]
Mac standalone patch w/fixes

https://hg.mozilla.org/integration/mozilla-inbound/rev/762911344837


Markus, Felipe: I saw y'all's comments before landing, so I included Markus's requested change in the patch I landed on inbound.  Here's the final patch I landed.

I also made one more change for a bug I noticed while testing the tryserver builds: the code was copying "webapprt-stub" in the Firefox install dir to "webapprt-stub" in the webapp install dir, whereas it should have been copying it to simply "webapprt" in the webapp install dir.  Fixed.
Comment 14 Myk Melez [:myk] [@mykmelez] 2012-04-17 08:06:16 PDT
Note: I resubmitted the patch to tryserver last night after noticing the "webapprt-stub" issue:

https://tbpl.mozilla.org/?tree=Try&rev=7c9ffc51663f

And then, for the sake of completeness, I resubmitted it again this morning with the fix that Markus requested:

https://tbpl.mozilla.org/?tree=Try&rev=799ccded28a5
Comment 15 Marco Bonardo [::mak] 2012-04-18 05:28:40 PDT
https://hg.mozilla.org/mozilla-central/rev/762911344837
Comment 16 Jason Smith [:jsmith] 2012-05-04 14:31:48 PDT
Verified that this feature has landed. Followup bugs will track problems related to Mac install in separate bugs in the web apps component.

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