725408 - implement webapp runtime (launcher and shell) for desktop

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Anant Narayanan [:anant]]

We need a command-line flag to Firefox that starts it in a "webapp" mode. For example, invoking `firefox-bin -webapp nytimes.com' should launch the New York Times "app" (in a chromeless window) if the user has previously installed it.

There are more details on the requirements here: https://etherpad.mozilla.org/webapp-flag-req

This bug is intended to track the implementation of the webapp mode and to discuss the correct path to do so.

Comment 1

[(no longer active)]

Here's some questions on the proposal:

1. What is considered to be the main user profile?  Given the situation where a user has N Firefox profiles, which one do you want to pick as the main profile?

2. Is the apps database residing in the main user profile a sqlite database?

3. What does "native installation" mean?

In general, I think you can mostly use the existing -app flag, with some things to keep in mind:

1. I don't understand why the symlinking of the database is necessary.  If you have the path to the main profile (which I assume is the profile in which the app was installed), then you can just store that path in the application.ini file for the app, and parse that value in XRE_ParseAppData and use it later on.  You should also figure out whether our storage code allows access to the same database from multiple applications.  Marco should know whether that's possible.

2. I see that the proposal is structured around command lines such as -webapp http://foo.com, but I don't see why you can't just use -app /path/to/foo.com.ini.

3. Looking at the code here <https://github.com/michaelrhanson/mozilla-central/blob/master/browser/app/nsBrowserApp.cpp#L182>, I can't quite tell why you're manually setting xreDirectory and directory on the app data.  By default, directory is set to the directory which contains application.ini.  Is that not what you want?  Also, xreDirectory should point to the directory containing the XUL runtime.  I can't tell whether this is what happens with the code there.

Comment 2

[Benjamin Smedberg]

Why is this <origin> instead of <URL>? Won't multiple apps be able to be hosted on a single domain?

What is the install sequence? When I discussed this with tabraldes, and the install sequence is that we copy firefox.exe or a similar stub into a per-user app "install directory" (which probably can also just contain the profile data). Is this flag here the flag to *install* those files, or the flag to launch the app after it is installed?

I wouldn't think we'd need a command line flag to trigger installation, and once it is installed the app information is stored in the install and doesn't need to be specified on the command line.

Comment 3

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Why is this <origin> instead of <URL>? Won't multiple apps be able to be
> hosted on a single domain?
> 

sub1.domain.com and sub2.domain.com can have their own apps, but sub.domain.com/app1 and sub.domain.com/app2 will not be able to.

> What is the install sequence? When I discussed this with tabraldes, and the
> install sequence is that we copy firefox.exe or a similar stub into a
> per-user app "install directory" (which probably can also just contain the
> profile data). 

Yes, this is the install sequence.  Copy firefox.exe or a similar stub, along with app-specific metadata (icons, INIs, etc), into the app's "install directory" and do the OS-specific appropriate thing for user discovery of the newly-installed app (desktop shortcuts, dock icon, etc)

> Is this flag here the flag to *install* those files, or the
> flag to launch the app after it is installed?
> 

This flag is to launch the app.  However, I don't think this will need to be implemented as a command line flag.

> I wouldn't think we'd need a command line flag to trigger installation, and
> once it is installed the app information is stored in the install and
> doesn't need to be specified on the command line.

I agree; since app-specific info is available in the install dir and since we control the functionality of the executable (whether it's a stub or firefox.exe), we won't need to pass this info along on the command line.

Comment 4

[Benjamin Smedberg]

Is there a document explaining why apps have to be segregated to subdomains? That surprises me from my previous understanding of how webapp manifests were supposed to work.

Comment 5

[Anant Narayanan [:anant]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Is there a document explaining why apps have to be segregated to subdomains?
> That surprises me from my previous understanding of how webapp manifests
> were supposed to work.

There have been several discussions in the past on the apps mailing list about this, but the basic reason is that a domain is the smallest unit we have in the current browser code that enforces a security boundary. If we relax the 1-app-per-domain restriction, it means multiple apps from the same domain will be able to read each other's cookies and share permissions (eg: you grant camera access on a domain basis, not an app basis).

Supporting multiple apps per domain will require an extensive rewrite of several security primitives in Gecko, which is a large project and we thought it would be best not to rely on it for Apps; especially given that the 1-app-per-domain restriction isn't terrbile ;-)

Comment 6

[Anant Narayanan [:anant]]

(In reply to Ehsan Akhgari [:ehsan] from comment #1)
> 1. What is considered to be the main user profile?  Given the situation
> where a user has N Firefox profiles, which one do you want to pick as the
> main profile?

Our suggestion was to use the profile ending with ".default" be considered the main user profile. Most users don't use our profile switching feature (because it's not invokable without a command line switch), so we are just optimizing for the mass.

> 2. Is the apps database residing in the main user profile a sqlite database?

Yes, created using mozStorage.

> 3. What does "native installation" mean?

It means the creation of a executable native to the platform (.exe on Windows, .app on Mac) that invokes the Firefox executable with the -webapp flag. Tim indicates that this is not possible on Windows, so the alternate there seems to be to embed a webapp.exe file alongside the "native" app.
 
> In general, I think you can mostly use the existing -app flag, with some
> things to keep in mind:

The big problem we had with the -app flag is that it loads Firefox anyway. On the Mac this meant that the system menu bar showed 'Firefox' as the app name, not 'My Awesome Webapp'. We don't need to load any of the Firefox UI at all, we just need to launch a special XUL app (the skeleton for the webapp), just like XULRunner did.

> 1. I don't understand why the symlinking of the database is necessary.  If
> you have the path to the main profile (which I assume is the profile in
> which the app was installed), then you can just store that path in the
> application.ini file for the app, and parse that value in XRE_ParseAppData
> and use it later on.  You should also figure out whether our storage code
> allows access to the same database from multiple applications.  Marco should
> know whether that's possible.

The simplistic reason was that the existing JS code that talks to the sqlite file expects it to be in 'applications.sqlite' in the *current* profile directory. We could certainly change that code to load the sqlite file from a different path that is stored in the .ini, no problem.

> 2. I see that the proposal is structured around command lines such as
> -webapp http://foo.com, but I don't see why you can't just use -app
> /path/to/foo.com.ini.

We cannot use absolute paths in the launcher executable, because that will break when the user switches between OS accounts. On the Mac, for example, we install apps into /Applications, if I switch my OS-level user account and launch this app from /Applications, it will point to an ini file that lives on another user's home directory (/Users/OtherUser/Library/path/to/ini) which will cause breakage.

The other option is to have another copy of application.ini alongside the native executable and use a relative path to launch. I believe Dan is currently implementing this.

> 3. Looking at the code here
> <https://github.com/michaelrhanson/mozilla-central/blob/master/browser/app/
> nsBrowserApp.cpp#L182>, I can't quite tell why you're manually setting
> xreDirectory and directory on the app data.  By default, directory is set to
> the directory which contains application.ini.  Is that not what you want? 
> Also, xreDirectory should point to the directory containing the XUL runtime.
> I can't tell whether this is what happens with the code there.

We don't need to create one XUL app stub for every installed webapp. The intent with this is to ship Firefox with only one default XUL app that will serve as the skeleton for all installed webapps. Thus, installation of a webapp will only generate a .ini and a native executale, and not an entire XUL directory.

Comment 7

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

(In reply to Anant Narayanan [:anant] from comment #6)
> We don't need to create one XUL app stub for every installed webapp. The
> intent with this is to ship Firefox with only one default XUL app that will
> serve as the skeleton for all installed webapps. Thus, installation of a
> webapp will only generate a .ini and a native executale, and not an entire
> XUL directory.

Besides working around the problem of -app loading Firefox, this is the other key advantage of -webapp: it enables all installed webapps to use the same instance of the appshell (i.e. the XUL app that loads the webapp in a chromeless window and provides it with a set of native integration APIs, like the ability to create native menus), which resides in Firefox's application package and gets updated automatically when Firefox is updated.

So the webapp's application package contains just the stub executable (plus icons, etc.), and the only file in the package that needs to be updated when Firefox is updated is the stub executable.

So the idea is something like this: /Applications/Example.app contains a stub executable that invokes `firefox -webapp http://example.com/`, whereupon Firefox loads the appshell from /Applications/Firefox.app/Contents/MacOS/path/to/appshell/ with the application.ini from /Users/someone/Library/Application Support/Firefox/Profiles/http:\/\/example.com\/.

Comment 8

[Benjamin Smedberg]

The install location (which has to be a per-user) contains the location of the Firefox install and probably the location of the default profile (although I still don't understand how you plan to synchronize access to the Firefox profile data, that can be a different discussion). The stub, which is basically just like the XULRunner stub with slightly different logic, loads libxul from the Firefox install, sets up the application paths/data and then calls XRE_main to launch.

Comment 9

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

Benjamin: Why does the install location have to be per-user?  We're planning to do that on Windows, to avoid UAC prompts, but on Mac we plan to install apps into the system-wide /Applications directory, which is where users expect to find them.

Comment 10

[Benjamin Smedberg]

This was discussed on the email thread about whether apps should have their own gecko profile: one of the requirements was that when a user chooses to uninstall an app via the app management UI, all of the data associated with that app should also be removed. Since users cannot access or remove another user's profile, all app installs should be considered per-user.

Comment 11

[(no longer active)]

(In reply to Anant Narayanan [:anant] from comment #6)
> (In reply to Ehsan Akhgari [:ehsan] from comment #1)
> > 1. What is considered to be the main user profile?  Given the situation
> > where a user has N Firefox profiles, which one do you want to pick as the
> > main profile?
> 
> Our suggestion was to use the profile ending with ".default" be considered
> the main user profile. Most users don't use our profile switching feature
> (because it's not invokable without a command line switch), so we are just
> optimizing for the mass.

This will prevent any user who is not on the ".default" profile to install and use any apps, which seems really bad to me.  Note that  there might be people with an old .default profile and a friend might have migrated them to a new profile which has been set to be the new default in the profile manager.

What's wrong with just using the current profile when the installation happens?

> > In general, I think you can mostly use the existing -app flag, with some
> > things to keep in mind:
> 
> The big problem we had with the -app flag is that it loads Firefox anyway.
> On the Mac this meant that the system menu bar showed 'Firefox' as the app
> name, not 'My Awesome Webapp'. We don't need to load any of the Firefox UI
> at all, we just need to launch a special XUL app (the skeleton for the
> webapp), just like XULRunner did.

Hmm, I believe that information is read from the application's bundle on Mac.  And that needs to change as part of packaging the stub executable, right?  That doesn't require a change in the -app logic, I don't think.

Or do you mean that just launching firefox -app /path/to/application.ini opens one Firefox window and one XUL application window?

Benjamin, is -no-remote the default for -app?  If not, I think it should be.

> > 1. I don't understand why the symlinking of the database is necessary.  If
> > you have the path to the main profile (which I assume is the profile in
> > which the app was installed), then you can just store that path in the
> > application.ini file for the app, and parse that value in XRE_ParseAppData
> > and use it later on.  You should also figure out whether our storage code
> > allows access to the same database from multiple applications.  Marco should
> > know whether that's possible.
> 
> The simplistic reason was that the existing JS code that talks to the sqlite
> file expects it to be in 'applications.sqlite' in the *current* profile
> directory. We could certainly change that code to load the sqlite file from
> a different path that is stored in the .ini, no problem.

Mak needs to confirm that accessing the database from multiple processes is OK.  If yes, then this change seems fine to me.

> > 2. I see that the proposal is structured around command lines such as
> > -webapp http://foo.com, but I don't see why you can't just use -app
> > /path/to/foo.com.ini.
> 
> We cannot use absolute paths in the launcher executable, because that will
> break when the user switches between OS accounts. On the Mac, for example,
> we install apps into /Applications, if I switch my OS-level user account and
> launch this app from /Applications, it will point to an ini file that lives
> on another user's home directory (/Users/OtherUser/Library/path/to/ini)
> which will cause breakage.
> 
> The other option is to have another copy of application.ini alongside the
> native executable and use a relative path to launch. I believe Dan is
> currently implementing this.

Like Benjamin said, I think we should keep the app installations per-user.

> > 3. Looking at the code here
> > <https://github.com/michaelrhanson/mozilla-central/blob/master/browser/app/
> > nsBrowserApp.cpp#L182>, I can't quite tell why you're manually setting
> > xreDirectory and directory on the app data.  By default, directory is set to
> > the directory which contains application.ini.  Is that not what you want? 
> > Also, xreDirectory should point to the directory containing the XUL runtime.
> > I can't tell whether this is what happens with the code there.
> 
> We don't need to create one XUL app stub for every installed webapp. The
> intent with this is to ship Firefox with only one default XUL app that will
> serve as the skeleton for all installed webapps. Thus, installation of a
> webapp will only generate a .ini and a native executale, and not an entire
> XUL directory.

Hmm, AFAIK on Windows if you want to get a native icon associated with your app you do need to have a separate stub executable with the icon built into it.  Same goes with app bundles on Mac I think.  So how do you plan to have a single XUL app stub executable?

Comment 12

[Robert Strong (they/them - no direct email)]

On Windows, the shortcut's icon is used when the stub is the shortcut target and it doesn't exit.

Comment 13

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

Attachment: work in progress #1: substantial portions implemented

It's time for some feedback to make sure we're on a reasonable path.

Benjamin: you know a ton about XULRunner, the build system, and the other stuff we're touching; and you've participated in previous implementation discussions.  Can you take a look at this and let us know what you think?


Our strategy for implementation of the "webapp runtime for desktop" is the combination of a "launcher" executable for each webapp along with a single "shell" xulapp.

A launcher locates the Firefox bin dir and invokes xulrunner to load the shell, providing it with an XREAppData object whose contents are a mashup of webapp (Name, Profile) and shell (ID, Version, Gecko Min/MaxVersion) configuration data.  The shell then loads the webapp in a window and provides it with OS integration APIs and capabilities.

On startup, if the launcher notices a mismatch between its version and the Firefox version, it replaces itself with a copy of the launcher in Firefox's bin dir and restarts.

A webapp stores its data in a webapp-specific dir in the standard OS location (Windows: %APPDATA%; Mac: ~/Library/Application Data/).  On Mac, its launcher lives in the standard OS location for executables (an application bundle inside the /Applications folder). On Windows, however, its launcher lives in its app data dir.  The launcher is exposed to users via standard OS discovery/launch affordances.

The launcher and shell share the unified GRE/app dir (and omnijar) with Firefox in a way that doesn't affect Firefox execution (and vice-versa; modulo lurking issues).


The bulk of this patch is a new top-level webapprt/ dir that contains code for both the launcher and the shell:

webapprt/
  mac/        - Mac launcher
  win/        - Windows launcher

  base/       - chrome for shell
  components/ - component manifest for shell
  modules/    - JavaScript module for shell

  tools/      - redit, a small util to embed icons into Windows executables


There are also a few platform changes.  In particular, nsXREDirProvider.cpp and Preferences.cpp have been modified to load default prefs from an additional location that is specific to the xulapp being loaded by xulrunner (to override browser.chromeURL and .hiddenWindowChromeURL for the shell).  And nsXREDirProvider.cpp has been modified to load app icons from the webapp's data dir.

Comment 14

[Benjamin Smedberg]

Comment on attachment 602243 [details] [diff] [review]
work in progress #1: substantial portions implemented

I may be reviewing this in the wrong order, and these notes are just preliminary:

1) I don't think we want to add a generic defaults/pref/{appid} directory. You can specify arbitrary directories to use for default preferences by giving an application directory provider to nsXREDirProvider. Currently this is used by XRE_InitEmbedding but not XRE_main, but we can just fix that (the provider would respond to NS_APP_PREFS_DEFAULTS_DIR_LIST). Or even do something simpler like hardcode "we're running in webrt mode".

2) On mac, it used to be that launch services figured the application bundle location by looking at argv[0]. So when we were using the XULRunner stub execv, we would exec /Library/Frameworks/XUL.framework/Versions/Current/xulrunner-bin but argv[0] would be /Applications/MyXULApp.app/Contents/MacOS/xulrunner-stub. It's not clear to me whether you're using this technique in "execNewBinary", but it looks like argv[0] is just "webapprt", which surprises me because that isn't a full path. Does this work no matter what the CWD is? Also the sprintf there looks like it's unnecessary, the compiler can just make a string array for us.

In nsXREDirProvider::GetFilesInternal you have

+      static const char *const kAppendAppIDPrefDir[] =
+        { "defaults", "pref", gAppData->ID, nsnull };

If we're not getting rid of this (see above), it shouldn't be static, that's just extra work at runtime when it could be local.

More later.

Comment 15

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #14)
> 1) I don't think we want to add a generic defaults/pref/{appid} directory.
> You can specify arbitrary directories to use for default preferences by
> giving an application directory provider to nsXREDirProvider. Currently this
> is used by XRE_InitEmbedding but not XRE_main, but we can just fix that (the
> provider would respond to NS_APP_PREFS_DEFAULTS_DIR_LIST). Or even do
> something simpler like hardcode "we're running in webrt mode".

D'oh!  A provider, so simple, I wish I'd thought of that.  I'll try implementing it that way, falling back to hardcoding "we're running in webrt mode" (which actually seems a bit more complicated; or at least it isn't obvious where to expose this state to nsXREDirProvider).


> 2) On mac, it used to be that launch services figured the application bundle
> location by looking at argv[0]. So when we were using the XULRunner stub
> execv, we would exec
> /Library/Frameworks/XUL.framework/Versions/Current/xulrunner-bin but argv[0]
> would be /Applications/MyXULApp.app/Contents/MacOS/xulrunner-stub. It's not
> clear to me whether you're using this technique in "execNewBinary", but it
> looks like argv[0] is just "webapprt", which surprises me because that isn't
> a full path. Does this work no matter what the CWD is? Also the sprintf
> there looks like it's unnecessary, the compiler can just make a string array
> for us.

Hmm, not sure about this one.  Dan: any thoughts here?


> In nsXREDirProvider::GetFilesInternal you have
> 
> +      static const char *const kAppendAppIDPrefDir[] =
> +        { "defaults", "pref", gAppData->ID, nsnull };
> 
> If we're not getting rid of this (see above), it shouldn't be static, that's
> just extra work at runtime when it could be local.

Roger that!


In the meantime, an issue that has cropped up is that calling XPCOMGlueStartup from the webapp launcher on mac is failing with:

    XPCOMGlueLoad error 4:0 for file [path/to]/libxpcom.dylib:
    Library not loaded: @executable_path/XUL
      Referenced from: [path/to]/libxpcom.dylib
      Reason: image not found

Presumably that's because libxpcom is compiled to look for libxul at @executable_path/XUL, but the webapp launcher executable is in a different directory.  I can work around the problem on the command line by setting DYLD_FALLBACK_LIBRARY_PATH to [path/to], but setting that env var inside the launcher itself via setenv doesn't have the same effect.

bsmedberg: do you know of another way to get XPCOMGlueLoad to find libxul?

Comment 16

[Dan Walkowski]

> 2) On mac, it used to be that launch services figured the application bundle
> location by looking at argv[0]. 

What do you mean by launch services?  If you mean OSX, then no, it does not utilize argv[0] for anything, afaik.  Argv[0] is the pretty name of the process, for display purposes, and is usually just the binary name.  If you mean xul-app launching services inside Firefox, then I have no idea.  Perhaps Firefox is assuming that argv[0] is the full path to the binary.  This is convenient, but cannot be assumed.  I can (and am about to) test to see if having the full path in argv[0] improves anything.

Comment 17

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #15)
> D'oh!  A provider, so simple, I wish I'd thought of that.  I'll try
> implementing it that way, falling back to hardcoding "we're running in webrt
> mode" (which actually seems a bit more complicated; or at least it isn't
> obvious where to expose this state to nsXREDirProvider).

Erm, after saying that I realized that it shouldn't be hard to hardcode, since I can simply check the app ID via nsXREAppData or nsIXULAppInfo.

Comment 18

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

Attachment: work in progress #2: working mac launcher

Here's an updated work-in-progress patch with a working Mac launcher.

Dan worked around the libxul load path issue by setting DYLD_FALLBACK_LIBRARY_PATH and then restarting the launcher with the modified environment, since dyld apparently configures its search path for a process before the process has a chance to modify it via that variable.

Also, I hardcoded the locations for WebappRT's default prefs, per our earlier conversation, so there is no longer a general facility by which apps sharing an app dir can provide app-specific prefs.

And Tim made a bunch of improvements to Windows integration.

Comment 19

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #18)
> And Tim made a bunch of improvements to Windows integration.

Generate webapp-uninstaller at build time
  Added NSIS script and Makefiles to build webapp-uninstaller and place them in dist/bin.
  Updated configure.in to reflect that NSIS is required to build the Web App Runtime.
Created WindowsShortcutService.jsm
  This file exposes Windows' shortcut-making functionality through a function "setShortcut".

Now that the uninstaller is generated at build time and we can create Windows shortcuts from javascript, it is no longer necessary to have a separate webapp installer that we invoke during native app generation.

Fixed DLL loading issues in webapprt:
  Removed dependency on mozglue.
  Used SetDllDirectory so DLLs are loaded from Firefox dir.
Made webapprt.exe look in correct location for icon.
Updated README.md.
Other directory/code cleanup.

Comment 20

[Benjamin Smedberg]

> diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp

> @@ -449,23 +453,25 @@ LoadDirsIntoArray(nsCOMArray<nsIFile>& aSourceDirs,

>  {
> -  nsCOMPtr<nsIFile> appended;
> +  nsCOMPtr<nsIFile> curDir;

This would be easier to review if this weren't renamed, and I can't see why it was renamed, please revert this change.

> +    else {
> +      if (NS_SUCCEEDED(curDir->Exists(&exists)) && exists)
> +	aDirectories.AppendObject(curDir);
>      }

This is a behavior change. If it's intentional, can you please explain it? Admittedly this function was underdocumented to begin with, and JARred extension support was hacked into the middle, but this feels kinda strange and the correct behavior needs to be documented.

> @@ -681,6 +694,13 @@ nsXREDirProvider::GetFilesInternal(const char* aProperty,
>                        kAppendChromeDir,
>                        directories);
>  
> +    nsCOMPtr<nsILocalFile> profileDir;
> +    rv = GetUserDataDirectory(getter_AddRefs(profileDir), false);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    LoadDirIntoArray(profileDir,
> +                     kAppendChromeDir,
> +                     directories);

I don't understand this change at all. It appears to be loading a chrome manifest from ~/Library/Application Support/Firefox/chrome/chrome.manifest, but that is never a feature we have supported before and is not one I think we should start supporting now.

> diff --git a/webapprt/Makefile.in b/webapprt/Makefile.in

This makefile appears to have been copied from somewhere else, but I'm not sure why all the bits are here:

> +MODULE = webapprt
> +XPIDL_MODULE = webapprt
> +GRE_MODULE = 1

None of this is necessary or correct, MODULE is a dead variable, XPIDL_MODULE is only used in makefiles that set XPIDLSRCS, and AFAICT this is not a GRE_MODULE (it's not compiled into libxul).

> +ifneq (,$(filter WINNT,$(OS_ARCH)))
> +DIRS = win
> +PARALLEL_DIRS += tools/redit

For sanity's sake and future editors, please use DIRS += here and in the mac case below.

> +# This switches $(INSTALL) to copy mode, like $(SYSINSTALL), so things that
> +# shouldn't get 755 perms need $(IFLAGS1) for either way of calling nsinstall.
> +NSDISTMODE = copy

Why?

> +include $(topsrcdir)/config/rules.mk
> +
> +libs:: prefs.js
> +	$(MKDIR) -p $(DIST)/bin/defaults/pref/[e-mail]

mkdir -p is not portable, please use $(NSINSTALL) -D

> +	$(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/defaults/pref/[e-mail]
> +# XXX need install:: for prefs.js?

No.

> +install:: webapprt.ini
> +	$(SYSINSTALL) $(IFLAGS1) webapprt.ini $(DESTDIR)$(mozappdir)

install rules are unused, please remove

> +
> +GARBAGE += webapprt.ini

Don't use need GARBAGE_DIRS for $(DIST)/bin/defaults/pref/[e-mail] ?

> diff --git a/webapprt/README.md b/webapprt/README.md

This file is odd: it seems like a combination of build instructions and some Windows-specific documentation about the install flow. I doesn't look like this belongs here.

> +Windows Native App Installation Flow
> +====================================
> +**PORT** Create an app-specific directory according
> +to the origin of the app.
> +
> +  - Location: `%APPDATA%`
> +  - Name: [subdomain.]server.domain;{http|https};port (or -1 if default)
> +  - e.g. `%APPDATA%\www.phoboslab.org;http;-1`
> +  - Impl note: The pieces of the directory name can be retrieved from an
> +  `nsIURI` whose spec is the origin of the app.
> +
> +The app-specific directory will be referred to as `${InstallDir}`.

Does the name of the directory convey any meaning later, or serve as a way to find if an app is already installed? Or to put it another way, why name them like this, instead of using pretty names or something else?

Will you be using the ascii version of the domain name to avoid IDN issues?

> +**TODO** Create the directory `${InstallDir}\bin`.

Why do you need a bin/ subdirectory? That seems like a very odd convention on Windows.

> +    # ==== Windows application.ini template ====
> +    [App]
> +    # NOTE: Slashes in the display name will cause the app to fail to launch
> +    Name=${AppDisplayName}

Why are you using the displayname here instead of "www.phoboslab.org;http;-1"? I think we should use a value we can control, for the most part. Otherwise we're going to have to sanitize it pretty heavily.

> +    # Just the dir name, not the full path
> +    # e.g. www.phoboslab.org;http;-1
> +    Profile=${InstallDir}

Does this preserve the local and roaming profile separation?

> +**TODO** Create `${AppDir}\${AppFilename}.lnk`, a shortcut to the app.
> +To accomplish this, call the `setShortcut` function available in
> +`webapprt\modules\WindowsShortcutService.jsm`.

Why do we need this shortcut in the installdir, instead of just on the desktop/start menu?

> +Step 2:  note: all folder and file names are case-sensitive!
> +
> +  - Create folder: /Applications/${appname}, where ${appname} is the friendly, user-facing name of
> +    the application, which is in the install record.
> +
> +    - Create folder: /Applications/${appname}/Contents
> +
> +      - Create folder: /Applications/${appname}/Contents/MacOS
> +        
> +        - Copy file: Firefox.app/Contents/MacOS/webapprt to 
> +            /Applications/${appname}/Contents/MacOS/webapprt
> +
> +        - Create file: application.ini
> +        
> +          - The template is similar to the Windows example above, but doesn't need the firefox path:  
> +              [App]  
> +              Name=${appname}  
> +              Profile=${profile}  

How will this work mean when multiple versions of Firefox installed, which is very common for QA/testing scenarios?

> +      - Create file: /Applications/${appname}/Contents/Info.plist

> +    	            <key>CFBundleIdentifier</key>  
> +    	            <string>${origin}</string>  

What are the set of legal characters in a CFBundleIdentifier, and how does MacOS use the identifier? The web says that underscores are illegal characters, apparently. And if we really want to avoid collisions shouldn't we instead do something like:

org.mozilla.webapprt.${escapedorigin} }

> +    	            <key>CFBundleSignature</key>  
> +    	            <string>MOZB</string>  

Is it correct to use the same CFBundleSignature for all of these bundles? Bug 323288 indicates that using the same signature could cause functional problems in Firefox (Sunbird had to switch to MOZC).

> diff --git a/webapprt/base/Makefile.in b/webapprt/base/Makefile.in

It appears that this makefile exists solely to package chrome (using jar.mn). Can we just move jar.mn up one directory and get rid of this Makefile entirely? That will make the build faster and probably easier to understand.

> diff --git a/webapprt/base/content/hiddenWindow.xul b/webapprt/base/content/hiddenWindow.xul

> +<?xml version="1.0"?>
> +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> +<window id="hiddenWindow" windowtype="app" title="App Helper Window" width="1024" height="800" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" sizemode="normal" resizable="true">
> +</window>

What is the correct behavior of a webapp when the last window is closed on Mac? If it stays active, doesn't the hidden window need to provide a more complete menu, such as the ability to open the webapp window again?

> diff --git a/webapprt/base/content/injector.js b/webapprt/base/content/injector.js

I do not understand the purpose of this file, so I have not reviewed it.

> diff --git a/webapprt/base/content/main.js b/webapprt/base/content/main.js

> +//only make the tiny window that handles window closing events
> +//(and quits the app) for Mac OS
> +if("Darwin" === os) {
> +    var observer = {
> +        observe: function(contentWindow, aTopic, aData) {
> +            if (aTopic == 'xul-window-destroyed') {
> +                // If there is nothing left but the main (invisible) window,
> +                // quit
> +                var wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                         .getService(Ci.nsIWindowMediator);
> +                var enumerator = wm.getEnumerator("app");
> +                if(enumerator.hasMoreElements()) {
> +                  return;
> +                }
> +
> +                var appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]
> +                                 .getService(Ci.nsIAppStartup);
> +                appStartup.quit(appStartup.eAttemptQuit);
> +            }
> +        }
> +    }

I see that you've answered my question above ;-) I don't think you need this code. We currently keep the app live with this code: http://hg.mozilla.org/mozilla-central/annotate/c6f26a8dcd08/toolkit/components/startup/nsAppStartup.cpp#l290

It should be straightforward to either make this configurable in application.ini or to hack in an ExitLastWindowClosingSurvivalArea as the application starts to get the correct behavior.

> diff --git a/webapprt/base/content/main.xul b/webapprt/base/content/main.xul

When does this window get used? I can't seem to find any external references to main.xul in this patch. In addition, it appears that this window exists solely to run main.js, which seems odd in itself, because main.js doesn't appear to require a window at all and could just be a JS component (or not exist at all).

> diff --git a/webapprt/base/content/window.js b/webapprt/base/content/window.js

> +function base64urldecode(arg) {
> +    var s = arg;
> +    s = s.replace(/-/g, '+'); // 62nd char of encoding
> +    s = s.replace(/_/g, '/'); // 63rd char of encoding
> +    switch (s.length % 4) // Pad with trailing '='s
> +    {
> +        case 0: break; // No pad chars in this case
> +        case 2: s += "=="; break; // Two pad chars
> +        case 3: s += "="; break; // One pad char
> +        default:
> +          throw new InputException("Illegal base64url string");
> +    }
> +    return window.atob(s); // Standard base64 decoder
> +}

This seems like a function which could be moved to shared library code somewhere.

I have not reviewed most of this code (I hope I'm not the right person to do so!). If there are issues in this file you'd like me to understand or comment on specifically, please let me know. It's not clear to me yet how the browserid data gets shared. If it gets shared through this file, it seems that should perhaps be in its own JS module for easier understanding and factoring.

> diff --git a/webapprt/components/Makefile.in b/webapprt/components/Makefile.in

It seems to me that this makefile exists solely to process WebappShellComponents.manifest, which could be done from the parent makefile and we don't need this subdirectory at all.

> diff --git a/webapprt/components/WebappShellComponents.manifest b/webapprt/components/WebappShellComponents.manifest

> +component {5d0ce354-df01-421a-83fb-7ead0990c24e} nsBrowserContentHandler.js application=[e-mail]
> +contract @mozilla.org/browser/clh;1 {5d0ce354-df01-421a-83fb-7ead0990c24e} application=[e-mail]

This surprises me greatly. Why do you want the Firefox command-line handlers in webapprt?

> diff --git a/webapprt/mac/Makefile.in b/webapprt/mac/Makefile.in

> +# No need for libmozglue.  Yet.  As far as we know.  Stay tuned.
> +MOZ_GLUE_LDFLAGS =
> +MOZ_GLUE_PROGRAM_LDFLAGS =

I don't understand what this is doing. You mean that you don't need to *statically* link mozglue? It's a bug if the standalone XPCOM glue requires mozglue, but in that case XPCOM_STANDALONE_GLUE_LDOPTS should be fixed.

> diff --git a/webapprt/mac/webapprt.mm b/webapprt/mac/webapprt.mm

> +const char *WEBAPPRT_EXECUTABLE = "webapprt";
> +const char *APPINI_NAME = "application.ini";
> +const char *WEBRTINI_NAME = "webapprt.ini";

These should all be

const char kFOO[] = "...";

The way they are currently written, the string buffer is const, but the actual pointer is not.

> +  nsresult AttemptGRELoad(char *greDir) {

Style nit, according to the style guide this should be:

nsresult
AttemptGRELoard(char* greDir)
{
  ...
}

> +  @try {
> +
> +  if (!firefoxPath) {
> +    firefoxPath = pathToNewestFirefox(bundleID); // XXX developer feature to specify other firefox here
> +    if (!firefoxPath) {
> +      // Launch a dialog to explain to the user that there's no compatible web runtime
> +      @throw makeException(@"Missing Web Runtime", @"Web Applications require Firefox to be installed");

TODO/followup?

> +  //Get the version number from my Info.plist.  This is more complicated that you might think; the information isn't meant for us,
> +  // it's meant for the OS.  We're supposed to know our version number.  I am reading the file and parsing into a mutable
> +  // dictionary object so that we can update it if necessary with the current version number and write it out again.
> +  NSString* myBundlePath = [[NSBundle mainBundle] bundlePath];

Is there a particular reason we can't just compile-in the version number? This seems like a rather complicated and perhaps I/O-expensive way of doing it. In that case, it also shouldn't be necessary to rewrite or change the Info.plist version number.

Munging DYLD_FALLBACK_LIBRARY_PATH should not be necessary. The way we have the XPCOM glue set up, it explicitly loads all the dependent libs (using dependentlibs.list) and should have already loaded XUL before libxpcom.dylib. I'll play around with this a little bit, but I don't think we want to start munging DYLD_ environment variables unless we have no possible other choice.

> +      // NOTE: The GRE has successfully loaded, so we can use XPCOM now
> +
> +      { // Scope for any XPCOM stuff we create

Note: in order for XPCOM leak checking to work, you should put matching calls to NS_LogInit and NS_LogTerm at the beginning and end of this block.

> +        // Cleanup
> +        // TODO: The app is about to exit;
> +        //       do we care about cleaning this stuff up?

Yes, in debug builds. Release builds will soon not even get this far (see bug 662444).

> +/* Find the currently installed Firefox, if any, and return
> + * an absolute path to it. */
> +NSString *pathToNewestFirefox(NSString* identifier)
> +{
> +  //default is firefox
> +  NSString* appIdent = @"org.mozilla.nightlydebug";
> +  if (identifier != NULL) appIdent = identifier;

We're going to need some buildconfig to get the right value here, no?

> +void execNewBinary(NSString* launchPath)
> +{
> +
> +  NSLog(@" launching webrt at path: %@\n", launchPath);
> +  // char binfile[500];
> +  // sprintf(binfile, "%s/%s", WEBAPPRT_EXECUTABLE);
> +
> +  const char *newargv[2];
> +  newargv[0] = [launchPath UTF8String];
> +  newargv[1] = NULL;

This can be more cleanly: const char *const newargv[] = {[launchPath UTF8String], NULL};

> diff --git a/webapprt/modules/Makefile.in b/webapprt/modules/Makefile.in

May I suggest that you don't need any of the components/base/modules subdirectories, and that all these files can just go directly in webapprt? The subdirectories were a mistake in Firefox, and they'll just slow down the build.

> diff --git a/webapprt/modules/WebAppService.jsm b/webapprt/modules/WebAppService.jsm

> +      db = Services.storage.openUnsharedDatabase(dbFile);

I'm no storage expert, but if we have any hope of this successfully using cross-process locking, doesn't it need to use openDatabase instead of openUnsahredDatabase?

> diff --git a/webapprt/modules/WindowsShortcutService.jsm b/webapprt/modules/WindowsShortcutService.jsm

I have not reviewed this file. I think that bholley or somebody else who can spot potential ctypes rooting errors should go through this with a fine-tooth comb.

> diff --git a/webapprt/win/app/Makefile.in b/webapprt/win/app/Makefile.in

> +WEBAPPRT_APP_NAME = webapprt

You don't appear to use this variable again. I don't see why you need it at all. If you do decide to keep it, however, our convention for makefile variables is that common variables used by config/rules.mk should be all uppercase. Variables specific to one makefile should be lowercase.

> +# Don't create a dependency on mozglue
> +MOZ_GLUE_LDFLAGS =

This again seems problematic. The standalone XPCOM glue should never create this dependency, and if it does we should fix that directly instead doing weird makefile variable tricks.

> +#
> +# Control the default heap size.
> +# This is the heap returned by GetProcessHeap().
> +# As we use the CRT heap, the default size is too large and wastes VM.
> +#
> +# The default heap size is 1MB on Win32.
> +# The heap will grow if need be.
> +#
> +# Set it to 256k.  See bug 127069.
> +#
> +ifndef GNU_CC
> +LDFLAGS += /HEAP:0x40000
> +ifeq ($(OS_TEST),x86_64)
> +# set stack to 2MB on x64 build.  See bug 582910
> +LDFLAGS += -STACK:2097152
> +endif
> +endif

I realize that you copied this snippet from browser/app/Makefile, and that's ok for now, but since we now have at least 4 different places in the tree we're sharing this snippet, please file a followup bug to unify that in a common makefile snippet.

> diff --git a/webapprt/win/app/webapprt.cpp b/webapprt/win/app/webapprt.cpp

> +namespace {
> +  const char * APP_INI = "application.ini";

ditto comments on the mac version, this should be "const char kAPP_INI[] = "...";

> +  bool AppendLeaf(char *base, const char *toAppend, int baseSize) {
> +    if (strlen(base)
> +      + sizeof(XPCOM_FILE_PATH_SEPARATOR[0])
> +      + strlen(toAppend)
> +      + 1
> +      > baseSize) {
> +      return false;
> +    }
> +
> +    if(base[strlen(base)-1] != XPCOM_FILE_PATH_SEPARATOR[0]) {
> +        base[strlen(base)+1] = '\0';
> +        base[strlen(base)] = XPCOM_FILE_PATH_SEPARATOR[0];
> +    }
> +
> +    strcpy(base + strlen(base), toAppend);
> +    return true;
> +  }

This kind of string manipulation seems wrong to me. Is there a reason we can't use std::string or something like that? It appears to be correct. Also, is there a particular reason you need this to be fallible? I assume that if this code fails, we exit pretty quickly anyway, right? If so, can we just _exit here and avoid propagating return codes?

> +  nsresult AttemptGRELoad(char *greDir) {
> +    nsresult rv;
> +
> +    AppendLeaf(greDir, XPCOM_DLL, MAXPATHLEN);
> +    rv = XPCOMGlueStartup(greDir);
> +    StripLeaf(greDir);
> +    if(NS_FAILED(rv)) {
> +      return rv;
> +    }
> +
> +    rv = XPCOMGlueLoadXULFunctions(kXULFuncs);
> +    if(NS_FAILED(rv)) {
> +      return rv;
> +    }
> +
> +    SetDllDirectoryA(greDir);

Can you explain why this is necessary? We explicitly SetDllDirectoryW(L"") in various places, and it shouldn't be necessary if the XPCOM glue load is working correctly.

> +  int CopyAndRun(const char *src, const char *dest) {

This code is not unicode-safe. You should assume that some of our users (especially Thai users) will be running in native Windows codepages which cannot express their own username and perhaps not the Firefox install path. This means that these paths either need to be wchar or use UTF8. "main" is getting its paths in UTF8, not the native charset, because of the automatic conversion from wmain. But in this case wchar is probably much easier, and you should be doing the UTF8 conversion much later, right before XRE_main is called.

> +int main(int argc, char* argv[])
> +{
> +  int result = 0;
> +  int realArgc = 1;
> +  char **realArgv = argv;
> +  char appINIPath[MAXPATHLEN];
> +  char rtINIPath[MAXPATHLEN];
> +  char greDir[MAXPATHLEN];
> +
> +  // Get the path and version of our running executable
> +  if (NS_FAILED(mozilla::BinaryPath::Get(argv[0], curEXE))) {
> +    Output("Couldn't calculate the application directory.\n");
> +    return 255;
> +  }
> +  Version runningVersion;
> +  GetVersionFromPath(curEXE, runningVersion);

Again, I think this would be much better compiled-in rather than reading it off the disk.

> +  { // Scope for any XPCOM stuff we create

Again use NS_LogInit/NS_LogTerm to scope this for leak-checking in debug builds.

I did not review any of the uninstaller work.

In general, this looks really close. We should talk about the XPCOM glue loading issues with paths (both DYLD_ setting on Mac and SetDllDirectory on Windows shouldn't be necessary).

Comment 21

[Dan Walkowski]

> +    	            <key>CFBundleSignature</key>  
> +    	            <string>MOZB</string>  

the Bundle Signature is obsolete, and no longer used.  It can be removed from the file.

Comment 22

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

Attachment: patch v1: initial implementation for Windows and Mac

Here's an updated patch that seems ready for review, although there are still a few known issues.

bsmedberg: TimA, DanW, and I wrote review comment responses via an Etherpad that nicely shows your comments and our responses in person-specific colors:

https://etherpad.mozilla.org/bug-725408

But I can copy them to a Bugzilla comment if you'd prefer to read them here.

Regarding the scope of review, I am hoping you'll review the build system changes, native launchers, and platform changes.  I'll find a front-end hacker to review the shell.

Comment 23

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

Comment on attachment 608664 [details] [diff] [review]
patch v1: initial implementation for Windows and Mac

Dietrich: are you a good reviewer for the shell portion of this patch?  The "shell" is the XUL/JS application that loads a webapp in a (mostly) chromeless window, so it seems appropriate to have a Firefox reviewer look at it, even though most of its code is in a separate top-level directory (and might become its own module).

If you aren't the right guy, can you recommend someone from the Firefox review crew?

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 608664 [details] [diff] [review]
patch v1: initial implementation for Windows and Mac

>+++ b/webapprt/content/doctype.inc
>+<!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd" >

>+++ b/webapprt/content/hiddenWindow.xul
>+<?xul-overlay href="chrome://browser/content/baseMenuOverlay.xul"?>

>+++ b/webapprt/content/webappWindow.xul
>+<?xul-overlay href="chrome://browser/content/baseMenuOverlay.xul"?>

These look like invalid dependencies on browser/.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 608664 [details] [diff] [review]
patch v1: initial implementation for Windows and Mac

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in

>+#ifndef XP_UNIX
>+@BINPATH@/webapprt.exe

Use @BIN_SUFFIX@?

>diff --git a/browser/locales/en-US/webapprt/window.properties b/browser/locales/en-US/webapprt/window.properties

>+# LOCALIZATION NOTE (quitApplicationCmdMac.label): %S will be replaced with
>+# the name of the webapp.
>+quitApplicationCmdMac.label=Quit %S

Some more context about what "the web app" means here would probably be useful. In particular, explaining that this string doesn't appear in Firefox proper, but rather in the "app" window when it is run separately, etc.

>diff --git a/webapprt/WebappRT.jsm b/webapprt/WebappRT.jsm

>+let WebappRT = {
>+  get config() {

I imagine it isn't possible for this to use non-main-thread-blocking I/O?

>diff --git a/webapprt/WebappRTCommandLineHandler.js b/webapprt/WebappRTCommandLineHandler.js

>+CommandLineHandler.prototype = {

>+  handle: function handle(cmdLine) {
>+    Services.ww.openWindow(null,
>+                           Services.prefs.getCharPref("browser.chromeURL"),

Given that this seems to be used completely separately, can you use a pref name other than browser.chromeURL, to avoid confusion/potential conflict? webapprt.chromeWindowURL? Actually, does this need to be pref-customizable at all? Why not just hard code the URL in the command line handler?

>diff --git a/webapprt/content/doctype.inc b/webapprt/content/doctype.inc

Seems like it would be simpler to just include these couple of lines directly in both files (I find the indirection more confusing than helpful for so little boilerplate).

>diff --git a/webapprt/content/hiddenWindow.xul b/webapprt/content/hiddenWindow.xul

>+        xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#">

unused

>diff --git a/webapprt/content/menubar.inc b/webapprt/content/menubar.inc

>+<menubar id="main-menubar"
>+         onpopupshowing="if (event.target.parentNode.parentNode == this &amp;&amp;
>+                            !('@mozilla.org/widget/nativemenuservice;1' in Cc))
>+                           this.setAttribute('openedwithkey',
>+                                             event.target.parentNode.openedWithKey);"
>+         style="border:0px;padding:0px;margin:0px;-moz-appearance:none">

A helper function in window.js for the script would be nice (avoid the gross XML-escaping of &&). Are these style rules really needed?

>diff --git a/webapprt/content/scripts.inc b/webapprt/content/scripts.inc

>+<script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>+<script type="application/javascript" src="chrome://global/content/viewZoomOverlay.js"/>
>+<script type="application/javascript" src="chrome://global/content/inlineSpellCheckUI.js"/>
>+<script type="application/javascript" src="chrome://global/content/viewSourceUtils.js"/>

Are these really all needed/useful?

>diff --git a/webapprt/content/sets.inc b/webapprt/content/sets.inc

>+  <command id="cmd_close" oncommand="BrowserCloseTabOrWindow()"/>
>+  <command id="View:FullScreen" oncommand="BrowserFullScreen();"/>

These seem to refer to undefined functions.

>diff --git a/webapprt/content/webappWindow.xul b/webapprt/content/webappWindow.xul

>+<?xul-overlay href="chrome://browser/content/baseMenuOverlay.xul"?>

I guess this is only for macWindowMenu.inc?

>+<window windowtype="webapprt:webapp"

>+        xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>+        xmlns:svg="http://www.w3.org/2000/svg"

These don't appear to be used here.

>diff --git a/webapprt/content/window.js b/webapprt/content/window.js

>+function updateEditUIVisibility() {

>+  let contextMenuPopupState = document.getElementById("contentAreaContextMenu").state;
>+  let placesContextMenuPopupState = document.getElementById("placesContext").state;
>+#ifdef MENUBAR_CAN_AUTOHIDE
>+  let appMenuPopupState = document.getElementById("appmenu-popup").state;
>+#endif

These elements don't seem to exist in this code... Doesn't this throw?

>diff --git a/webapprt/win/webapprt.cpp b/webapprt/win/webapprt.cpp

>+  /*
>+  const Version runningVersion = { MOZILLA_MAJOR_VERSION,
>+                                   MOZILLA_MINOR_VERSION,
>+                                   MOZILLA_MINI_VERSION,
>+                                   MOZILLA_MICRO_VERSION };
>+  */

This should have an XXX comment/bug reference for the followup to use the compile-time values.

Comment 26

[Robert Strong (they/them - no direct email)]

Comment on attachment 608664 [details] [diff] [review]
patch v1: initial implementation for Windows and Mac

The NSIS portion of this patch is from bug 735518 and already has r-.

Comment 27

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

For the benefit of folks who read this bug report in the future, here are danwalkowski's, timA's, and my responses to bsmedberg's feedback on our work-in-progress patch.

Note: although patch v1 was attached to the bug last week, and I'm posting this comment today, the commentary here pre-dates the patch.

--------------------------------------------------------------------------------

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #20)
> > diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
> 
> > @@ -449,23 +453,25 @@ LoadDirsIntoArray(nsCOMArray<nsIFile>& aSourceDirs,
> 
> >  {
> > -  nsCOMPtr<nsIFile> appended;
> > +  nsCOMPtr<nsIFile> curDir;
> 
> This would be easier to review if this weren't renamed, and I can't see why
> it was renamed, please revert this change.

Done: https://github.com/michaelrhanson/mozilla-central/commit/904f14119e2fd7365de88223663b9baf30867bfd


> > +    else {
> > +      if (NS_SUCCEEDED(curDir->Exists(&exists)) && exists)
> > +    aDirectories.AppendObject(curDir);
> >      }
> 
> This is a behavior change. If it's intentional, can you please explain it?
> Admittedly this function was underdocumented to begin with, and JARred
> extension support was hacked into the middle, but this feels kinda strange
> and the correct behavior needs to be documented.

I did this during a merge from upstream, and now I can't remember why, so I removed it, which doesn't seem to have caused any problems.


> > @@ -681,6 +694,13 @@ nsXREDirProvider::GetFilesInternal(const char* aProperty,
> >                        kAppendChromeDir,
> >                        directories);
> >  
> > +    nsCOMPtr<nsILocalFile> profileDir;
> > +    rv = GetUserDataDirectory(getter_AddRefs(profileDir), false);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    LoadDirIntoArray(profileDir,
> > +                     kAppendChromeDir,
> > +                     directories);
> 
> I don't understand this change at all. It appears to be loading a chrome
> manifest from ~/Library/Application Support/Firefox/chrome/chrome.manifest,
> but that is never a feature we have supported before and is not one I think
> we should start supporting now.

On Windows, we want Firefox-as-XULRunner to use the icon that lives with the app.  We accomplished that by putting the icon in ${AppDir}\chrome\icons\default\topwindow.ico and making this change.  Sounds like we'll need to do some more fiddling...

(Benjamin later replied in the etherpad: "I still don't understand this, since chrome manifests aren't used for .ico searching.")


> > diff --git a/webapprt/Makefile.in b/webapprt/Makefile.in
> 
> This makefile appears to have been copied from somewhere else, but I'm not
> sure why all the bits are here:
> 
> > +MODULE = webapprt
> > +XPIDL_MODULE = webapprt
> > +GRE_MODULE = 1
> 
> None of this is necessary or correct, MODULE is a dead variable,
> XPIDL_MODULE is only used in makefiles that set XPIDLSRCS, and AFAICT this
> is not a GRE_MODULE (it's not compiled into libxul).

Ok, removed.

https://github.com/michaelrhanson/mozilla-central/commit/48690777595a5aa5cb86a424c0a1bc655793bfc6


> > +ifneq (,$(filter WINNT,$(OS_ARCH)))
> > +DIRS = win
> > +PARALLEL_DIRS += tools/redit
> 
> For sanity's sake and future editors, please use DIRS += here and in the mac
> case below.

Yup, makes sense, done.

https://github.com/michaelrhanson/mozilla-central/commit/9590ca95064a0432dcc3f62bcb0f573dca09b387


> > +# This switches $(INSTALL) to copy mode, like $(SYSINSTALL), so things that
> > +# shouldn't get 755 perms need $(IFLAGS1) for either way of calling nsinstall.
> > +NSDISTMODE = copy
> 
> Why?

I thought we needed it to ensure file permissions are set properly, but I  just tested that assumption, and it proved incorrect, so I have removed  this declaration.

https://github.com/michaelrhanson/mozilla-central/commit/d300df32523ff9716708fa9bd6851e754450ce8e


> > +include $(topsrcdir)/config/rules.mk
> > +
> > +libs:: prefs.js
> > +    $(MKDIR) -p $(DIST)/bin/defaults/pref/[e-mail]
> 
> mkdir -p is not portable, please use $(NSINSTALL) -D

Done.

https://github.com/michaelrhanson/mozilla-central/commit/6b7a142710e33176732c20568d1fa57b89ba4c79


> > +    $(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/defaults/pref/[e-mail]
> > +# XXX need install:: for prefs.js?
> 
> No.
> 
> > +install:: webapprt.ini
> > +    $(SYSINSTALL) $(IFLAGS1) webapprt.ini $(DESTDIR)$(mozappdir)
> 
> install rules are unused, please remove

Removed comment and install rule.

https://github.com/michaelrhanson/mozilla-central/commit/0de4d74a0136568a4fc04d612eb95aaeee81c540


> > +GARBAGE += webapprt.ini
> 
> Don't use need GARBAGE_DIRS for
> $(DIST)/bin/defaults/pref/[e-mail] ?

`make  clean` deletes $(DIST) in its entirety, and I don't see any other such declarations in the tree, so doing this seems both unnecessary and unconventional.

However, I did notice an existing GARBAGE_DIRS declaration that was misspelled, and I fixed it.

https://github.com/michaelrhanson/mozilla-central/commit/1b9ae6953be0f7d41011fb87e3a7ac861e18ac55

(Perhaps we copied it from a Firefox one <http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/Makefile.in#136> that is also misspelled.)


> > diff --git a/webapprt/README.md b/webapprt/README.md
> 
> This file is odd: it seems like a combination of build instructions and some
> Windows-specific documentation about the install flow. I doesn't look like
> this belongs here.

This  was a temporary file we used to document the install flow to help us  set up a testing environment and help felipe implement the install flow  in Firefox. I have moved it to an Etherpad page <https://etherpad.mozilla.org/webapprt-install-flow>.

https://github.com/michaelrhanson/mozilla-central/commit/ef7d31b24c03edd86ca68705710b81270ad748d9


> > +Windows Native App Installation Flow
> > +====================================
> > +**PORT** Create an app-specific directory according
> > +to the origin of the app.
> > +
> > +  - Location: `%APPDATA%`
> > +  - Name: [subdomain.]server.domain;{http|https};port (or -1 if default)
> > +  - e.g. `%APPDATA%\www.phoboslab.org;http;-1`
> > +  - Impl note: The pieces of the directory name can be retrieved from an
> > +  `nsIURI` whose spec is the origin of the app.
> > +
> > +The app-specific directory will be referred to as `${InstallDir}`.
> 
> Does the name of the directory convey any meaning later, or serve as a way
> to find if an app is already installed? Or to put it another way, why name
> them like this, instead of using pretty names or something else?

Webapp data dirs should have unique names, and we figured it would be more complicated to ensure the uniqueness of pretty names (f.e. by appending unique suffixes  to duplicates) than to format names in this fashion to ensure uniqueness.

Note that we don't expect users to look in %APPDATA% for their apps. That's just where we store the files.


> Will you be using the ascii version of the domain name to avoid IDN issues?

That probably makes sense; these directory names aren't intended to look pretty.  We'll have to make sure that the patches in bug 731541 write the ascii version of the domain name.


> > +**TODO** Create the directory `${InstallDir}\bin`.
> 
> Why do you need a bin/ subdirectory? That seems like a very odd convention
> on Windows.

The app install dir has been reorganized and no longer uses a bin/ subdirectory.


> > +    # ==== Windows application.ini template ====
> > +    [App]
> > +    # NOTE: Slashes in the display name will cause the app to fail to launch
> > +    Name=${AppDisplayName}
> 
> Why are you using the displayname here instead of
> "www.phoboslab.org;http;-1"? I think we should use a value we can control,
> for the most part. Otherwise we're going to have to sanitize it pretty
> heavily.

This is the name that XULRunner shows in the title bar, so using the origin could be confusing for users.  I haven't seen this value used for anything filesystem-related; what kind of sanitization would be required here?  Would removing unprintable characters and slashes not be sufficient?


> > +    # Just the dir name, not the full path
> > +    # e.g. www.phoboslab.org;http;-1
> > +    Profile=${InstallDir}
> 
> Does this preserve the local and roaming profile separation?

I don't fully understand the question.  The app is being installed to %APPDATA%\dir_based_on_origin.  This is also the directory that houses the roaming profile data for the app.  Additionally, local profile data lives at %LOCALAPPDATA%\dir_based_on_origin.  As the user roams around, her/his roaming profile data and the app follow her/him around.  I do see potential for issues when the user logs into a machine that does not have Firefox installed (or has Firefox installed to a different directory), which would be alleviated by installing to %LOCALAPPDATA%\dir_based_on_origin.  Either way, profile data for the app lives in both (roaming and local) locations.


> > +**TODO** Create `${AppDir}\${AppFilename}.lnk`, a shortcut to the app.
> > +To accomplish this, call the `setShortcut` function available in
> > +`webapprt\modules\WindowsShortcutService.jsm`.
> 
> Why do we need this shortcut in the installdir, instead of just on the
> desktop/start menu?

This is a holdover from when we had a different install mechanism.  This is no longer necessary and has been removed.


·> > +Step 2:  note: all folder and file names are case-sensitive!
> > +
> > +  - Create folder: /Applications/${appname}, where ${appname} is the friendly, user-facing name of
> > +    the application, which is in the install record.
> > +
> > +    - Create folder: /Applications/${appname}/Contents
> > +
> > +      - Create folder: /Applications/${appname}/Contents/MacOS
> > +        
> > +        - Copy file: Firefox.app/Contents/MacOS/webapprt to 
> > +            /Applications/${appname}/Contents/MacOS/webapprt
> > +
> > +        - Create file: application.ini
> > +        
> > +          - The template is similar to the Windows example above, but doesn't need the firefox path:  
> > +              [App]  
> > +              Name=${appname}  
> > +              Profile=${profile}  
> 
> How will this work mean when multiple versions of Firefox installed, which
> is very common for QA/testing scenarios?

Hardcoding paths to apps is a bad idea on the Mac.
The Firefox to use is determined at launch time, by asking the OS where it is, in this order:
org.mozilla.nightly
org.mozilla.aurora
org.mozilla.firefox

It can also be overridden on an app by app basis, for testing, by editing the Info.plist file of the app.


> > +      - Create file: /Applications/${appname}/Contents/Info.plist
> 
> > +                    <key>CFBundleIdentifier</key>  
> > +                    <string>${origin}</string>  
> 
> What are the set of legal characters in a CFBundleIdentifier, and how does
> MacOS use the identifier? The web says that underscores are illegal
> characters, apparently. And if we really want to avoid collisions shouldn't
> we instead do something like:
> 
> org.mozilla.webapprt.${escapedorigin} }

The BundleIdentifier is the origin of the web application, reversed. It has the same legal characters as a domain name.


> > +                    <key>CFBundleSignature</key>  
> > +                    <string>MOZB</string>  
> 
> Is it correct to use the same CFBundleSignature for all of these bundles?
> Bug 323288 indicates that using the same signature could cause functional
> problems in Firefox (Sunbird had to switch to MOZC).

CFBundle signature is obsolete, and can be removed if you are using CDBundleIdentifier.


> > diff --git a/webapprt/base/Makefile.in b/webapprt/base/Makefile.in
> 
> It appears that this makefile exists solely to package chrome (using
> jar.mn). Can we just move jar.mn up one directory and get rid of this
> Makefile entirely? That will make the build faster and probably easier to
> understand.

I don't see why not. Done!

https://github.com/michaelrhanson/mozilla-central/commit/f17eb79576f86eff48fd46c20c56203c6b57e9ad


> > diff --git a/webapprt/base/content/hiddenWindow.xul b/webapprt/base/content/hiddenWindow.xul
> 
> > +<?xml version="1.0"?>
> > +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> > +<window id="hiddenWindow" windowtype="app" title="App Helper Window" width="1024" height="800" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" sizemode="normal" resizable="true">
> > +</window>
> 
> What is the correct behavior of a webapp when the last window is closed on
> Mac? If it stays active, doesn't the hidden window need to provide a more
> complete menu, such as the ability to open the webapp window again?

Our plan is to close the app when the last window closes.


> > diff --git a/webapprt/base/content/injector.js b/webapprt/base/content/injector.js
> 
> I do not understand the purpose of this file, so I have not reviewed it.

It is cruft and has been removed.


> > diff --git a/webapprt/base/content/main.js b/webapprt/base/content/main.js
> 
> > +//only make the tiny window that handles window closing events
> > +//(and quits the app) for Mac OS
> > +if("Darwin" === os) {
> > +    var observer = {
> > +        observe: function(contentWindow, aTopic, aData) {
> > +            if (aTopic == 'xul-window-destroyed') {
> > +                // If there is nothing left but the main (invisible) window,
> > +                // quit
> > +                var wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> > +                         .getService(Ci.nsIWindowMediator);
> > +                var enumerator = wm.getEnumerator("app");
> > +                if(enumerator.hasMoreElements()) {
> > +                  return;
> > +                }
> > +
> > +                var appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]
> > +                                 .getService(Ci.nsIAppStartup);
> > +                appStartup.quit(appStartup.eAttemptQuit);
> > +            }
> > +        }
> > +    }
> 
> I see that you've answered my question above ;-) I don't think you need this
> code. We currently keep the app live with this code:
> http://hg.mozilla.org/mozilla-central/annotate/c6f26a8dcd08/toolkit/
> components/startup/nsAppStartup.cpp#l290
> 
> It should be straightforward to either make this configurable in
> application.ini or to hack in an ExitLastWindowClosingSurvivalArea as the
> application starts to get the correct behavior.

I added an ExitLastWindowClosingSurvivalArea call.

https://github.com/michaelrhanson/mozilla-central/commit/c4d9df099e48992c0d5da62830458798b99f1dea

It seems to work just fine when I close the app by closing the window or by selecting Quit from the Application menu. But if I press the Command+Q keyboard shortcut, I get:

###!!! ASSERTION: consider quit stopper out of bounds: 'mConsiderQuitStopper > 0', file /Users/myk/Dropbox/central-git/toolkit/components/startup/nsAppStartup.cpp, line 506

And then the app stays running until I tell it to quit a second time.


> > diff --git a/webapprt/base/content/main.xul b/webapprt/base/content/main.xul
> 
> When does this window get used? I can't seem to find any external references
> to main.xul in this patch. In addition, it appears that this window exists
> solely to run main.js, which seems odd in itself, because main.js doesn't
> appear to require a window at all and could just be a JS component (or not
> exist at all).

I think this is cruft and have removed it.

https://github.com/michaelrhanson/mozilla-central/commit/00ad5b04d962cf003ce7a5b6e3c9ba06c668e36e


> > diff --git a/webapprt/base/content/window.js b/webapprt/base/content/window.js
> 
> > +function base64urldecode(arg) {
> > +    var s = arg;
> > +    s = s.replace(/-/g, '+'); // 62nd char of encoding
> > +    s = s.replace(/_/g, '/'); // 63rd char of encoding
> > +    switch (s.length % 4) // Pad with trailing '='s
> > +    {
> > +        case 0: break; // No pad chars in this case
> > +        case 2: s += "=="; break; // Two pad chars
> > +        case 3: s += "="; break; // One pad char
> > +        default:
> > +          throw new InputException("Illegal base64url string");
> > +    }
> > +    return window.atob(s); // Standard base64 decoder
> > +}
> 
> This seems like a function which could be moved to shared library code
> somewhere.

That's also cruft, and I have removed it.


> I have not reviewed most of this code (I hope I'm not the right person to do
> so!). If there are issues in this file you'd like me to understand or
> comment on specifically, please let me know. It's not clear to me yet how
> the browserid data gets shared. If it gets shared through this file, it
> seems that should perhaps be in its own JS module for easier understanding
> and factoring.

I am hoping you'll review the build system changes, native launchers, and platform changes.  I'll find a front-end hacker to review the shell.


> > diff --git a/webapprt/components/Makefile.in b/webapprt/components/Makefile.in
> 
> It seems to me that this makefile exists solely to process
> WebappShellComponents.manifest, which could be done from the parent makefile
> and we don't need this subdirectory at all.

Yup, makes sense.

https://github.com/michaelrhanson/mozilla-central/commit/7d14b361b43ea38c51e907a6a83e24fd9b793bc7


> > diff --git a/webapprt/components/WebappShellComponents.manifest b/webapprt/components/WebappShellComponents.manifest
> 
> > +component {5d0ce354-df01-421a-83fb-7ead0990c24e} nsBrowserContentHandler.js application=[e-mail]
> > +contract @mozilla.org/browser/clh;1 {5d0ce354-df01-421a-83fb-7ead0990c24e} application=[e-mail]
> 
> This surprises me greatly. Why do you want the Firefox command-line handlers
> in webapprt?

We needed some command-line handler, since the startup path runs through it (it is responsible for opening the first window). And Firefox's works. Nevertheless, we can use a much simpler WebappRT-specific handler, since we don't actually have to handle any command-line flags.

Fixed!

https://github.com/michaelrhanson/mozilla-central/commit/06d534917cbd224f6312090699b4a487552b2399


> > diff --git a/webapprt/mac/Makefile.in b/webapprt/mac/Makefile.in
> 
> > +# No need for libmozglue.  Yet.  As far as we know.  Stay tuned.
> > +MOZ_GLUE_LDFLAGS =
> > +MOZ_GLUE_PROGRAM_LDFLAGS =
> 
> I don't understand what this is doing. You mean that you don't need to
> *statically* link mozglue? It's a bug if the standalone XPCOM glue requires
> mozglue, but in that case XPCOM_STANDALONE_GLUE_LDOPTS should be fixed.

webapprt/mac/Makefile.in now has:

    # Don't create a dependency on mozglue, which is impossible (difficult?)
    # to dynamically link into our executable, as we copy it to arbitrary locations.
    MOZ_GLUE_LDFLAGS =
    MOZ_GLUE_PROGRAM_LDFLAGS =
    
    # mozglue uses mfbt's Assertions.cpp, which provides MOZ_ASSERT, which lots
    # of code in libxpcom uses, so we have to do the same.
    VPATH += $(topsrcdir)/mfbt
    CPPSRCS = Assertions.cpp

I'm far from the expert on the build system (and library linking), so I apologize in advance for my ignorance, but it seems like we would have to link mozglue statically, and I don't know how to do it.  mozglue's Makefile appears to force it to be built as a shared library <http://mxr.mozilla.org/mozilla-central/source/mozglue/build/Makefile.in#58>.


> > diff --git a/webapprt/mac/webapprt.mm b/webapprt/mac/webapprt.mm
> 
> > +const char *WEBAPPRT_EXECUTABLE = "webapprt";
> > +const char *APPINI_NAME = "application.ini";
> > +const char *WEBRTINI_NAME = "webapprt.ini";
> 
> These should all be
> 
> const char kFOO[] = "...";

Fixed


> The way they are currently written, the string buffer is const, but the
> actual pointer is not.
> 
> > +  nsresult AttemptGRELoad(char *greDir) {
> 
> Style nit, according to the style guide this should be:
> 
> nsresult
> AttemptGRELoard(char* greDir)
> {
>   ...
> }
> 
> > +  @try {
> > +
> > +  if (!firefoxPath) {
> > +    firefoxPath = pathToNewestFirefox(bundleID); // XXX developer feature to specify other firefox here
> > +    if (!firefoxPath) {
> > +      // Launch a dialog to explain to the user that there's no compatible web runtime
> > +      @throw makeException(@"Missing Web Runtime", @"Web Applications require Firefox to be installed");
> 
> TODO/followup?

All finished and committed.


> > +  //Get the version number from my Info.plist.  This is more complicated that you might think; the information isn't meant for us,
> > +  // it's meant for the OS.  We're supposed to know our version number.  I am reading the file and parsing into a mutable
> > +  // dictionary object so that we can update it if necessary with the current version number and write it out again.
> > +  NSString* myBundlePath = [[NSBundle mainBundle] bundlePath];
> 
> Is there a particular reason we can't just compile-in the version number?
> This seems like a rather complicated and perhaps I/O-expensive way of doing
> it. In that case, it also shouldn't be necessary to rewrite or change the
> Info.plist version number.
> 

We are now using a BuildID check, to be better compatible with Nightly et. al, since they update frequently without changing version strings.  And it is now compiled into a constant.


> Munging DYLD_FALLBACK_LIBRARY_PATH should not be necessary. The way we have
> the XPCOM glue set up, it explicitly loads all the dependent libs (using
> dependentlibs.list) and should have already loaded XUL before
> libxpcom.dylib. I'll play around with this a little bit, but I don't think
> we want to start munging DYLD_ environment variables unless we have no
> possible other choice.
> 

Definitely agreed on not wanting to munge library search paths (neither on Mac, with DYLD_*, nor on Windows, where we munge it with a system call). We haven't been able to find an alternative, but it's also possible we're doing something wrong.


> > +      // NOTE: The GRE has successfully loaded, so we can use XPCOM now
> > +
> > +      { // Scope for any XPCOM stuff we create
> 
> Note: in order for XPCOM leak checking to work, you should put matching
> calls to NS_LogInit and NS_LogTerm at the beginning and end of this block.

ok done


> > +        // Cleanup
> > +        // TODO: The app is about to exit;
> > +        //       do we care about cleaning this stuff up?
> 
> Yes, in debug builds. Release builds will soon not even get this far (see
> bug 662444).
> 
> > +/* Find the currently installed Firefox, if any, and return
> > + * an absolute path to it. */
> > +NSString *pathToNewestFirefox(NSString* identifier)
> > +{
> > +  //default is firefox
> > +  NSString* appIdent = @"org.mozilla.nightlydebug";
> > +  if (identifier != NULL) appIdent = identifier;
> 
> We're going to need some buildconfig to get the right value here, no?

The Firefox to use is now determined at launch time, by asking the OS where it is, in this order:
org.mozilla.nightly
org.mozilla.aurora
org.mozilla.firefox

It can also be overridden on an app by app basis, for testing, by editing the Info.plist file of the app.


> > +void execNewBinary(NSString* launchPath)
> > +{
> > +
> > +  NSLog(@" launching webrt at path: %@\n", launchPath);
> > +  // char binfile[500];
> > +  // sprintf(binfile, "%s/%s", WEBAPPRT_EXECUTABLE);
> > +
> > +  const char *newargv[2];
> > +  newargv[0] = [launchPath UTF8String];
> > +  newargv[1] = NULL;
> 
> This can be more cleanly: const char *const newargv[] = {[launchPath
> UTF8String], NULL};

changed


> > diff --git a/webapprt/modules/Makefile.in b/webapprt/modules/Makefile.in
> 
> May I suggest that you don't need any of the components/base/modules
> subdirectories, and that all these files can just go directly in webapprt?
> The subdirectories were a mistake in Firefox, and they'll just slow down the
> build.

Yup, makes sense.

https://github.com/michaelrhanson/mozilla-central/commit/f1e64a488df7b9a6e4b056c411ab4233ea08585c


> > diff --git a/webapprt/modules/WebAppService.jsm b/webapprt/modules/WebAppService.jsm
> 
> > +      db = Services.storage.openUnsharedDatabase(dbFile);
> 
> I'm no storage expert, but if we have any hope of this successfully using
> cross-process locking, doesn't it need to use openDatabase instead of
> openUnsahredDatabase?

openUnsharedDatabase  has an unfortunate name (my bad; I implemented it lo those many years  ago), as it is actually a cache that is unshared, and that cache is  (used to be?) non-threadsafe, so opening a database in this way makes  the connection (more?) threadsafe.

Nevertheless,  the implementation of the app registry datastore that ended up landing  in mozilla-central for b2g and then getting enabled for desktop (in bug 697006) uses a set of JSON files to store install records, so this code has been rewritten and no longer accesses an SQLite database.

It does access those JSON files, if the webapp makes navigator.mozApps calls. That makes me uneasy, especially since those calls could theoretically result in reads and even writes that contend with a Firefox process's manipulation of those JSON files. I suspect there's still some work to do here, although perhaps this is sufficient for a first take, especially if webapps are unlikely to install other webapps.

And note that the runtime itself doesn't need to access those JSON files to run the app, as I modified the installer code to write a copy of the app install record and manifest into the webapp profile dir.

This is data duplication, and it complicates synchronization of that information. But it isolates webapps against profile location changes (f.e. due to Firefox reinstalls that blow away data or users creating a new profile to fix problems with the old one, which we recommend in some support articles) that would otherwise break webapps. Which feels more important than making synchronization as simple as possible to implement. Webapps shouldn't break because a user's profile location changes (nor, for that matter, because they uninstall Firefox; but we can cross that bridge at a later date).


> > diff --git a/webapprt/modules/WindowsShortcutService.jsm b/webapprt/modules/WindowsShortcutService.jsm
> 
> I have not reviewed this file. I think that bholley or somebody else who can
> spot potential ctypes rooting errors should go through this with a
> fine-tooth comb.

Bug 738501 has been created for this.  Additionally, bug 738500 has been created to track the similar "WindowsIconEmbeddingService.jsm" which is a js-ctypes implementation of redit.


> > diff --git a/webapprt/win/app/Makefile.in b/webapprt/win/app/Makefile.in
> 
> > +WEBAPPRT_APP_NAME = webapprt
> 
> You don't appear to use this variable again. I don't see why you need it at
> all. If you do decide to keep it, however, our convention for makefile
> variables is that common variables used by config/rules.mk should be all
> uppercase. Variables specific to one makefile should be lowercase.

It looks like we've gotten rid of this.


> > +# Don't create a dependency on mozglue
> > +MOZ_GLUE_LDFLAGS =
> 
> This again seems problematic. The standalone XPCOM glue should never create
> this dependency, and if it does we should fix that directly instead doing
> weird makefile variable tricks.

I  don't think it's the standalone XPCOM glue that creates it; rather  mozglue appears to be getting linked against all programs by default.  Thus programs in the tree that don't want to link against it have to  explicitly specify that, for example:

Crash Reporter: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/Makefile.in#58
Updater: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/Makefile.in#54
Maintenance Service: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/maintenanceservice/Makefile.in#60
XULRunner stub: http://mxr.mozilla.org/mozilla-central/source/xulrunner/stub/Makefile.in#50
MAR tool: http://mxr.mozilla.org/mozilla-central/source/modules/libmar/tool/Makefile.in#60
SQLite (on OS X): http://mxr.mozilla.org/mozilla-central/source/db/sqlite3/src/Makefile.in#80

Ironically, even mozglue has to specify that it should not be linked against itself!

http://mxr.mozilla.org/mozilla-central/source/mozglue/build/Makefile.in#66

Probably it would be better to flip the logic across the whole build system and require programs to explicitly enable mozglue instead of explicitly disabling it; but that change seems a bit out-of-scope for this bug.

In any case, I imagine that we actually *do* want to link against mozglue, because it would provide us with valuable improvements to memory usage.  It just isn't clear how to do so, and perhaps it isn't an absolute requirement for this first iteration?


> > +# Control the default heap size.
> > +# This is the heap returned by GetProcessHeap().
> > +# As we use the CRT heap, the default size is too large and wastes VM.
> > +#
> > +# The default heap size is 1MB on Win32.
> > +# The heap will grow if need be.
> > +#
> > +# Set it to 256k.  See bug 127069.
> > +#
> > +ifndef GNU_CC
> > +LDFLAGS += /HEAP:0x40000
> > +ifeq ($(OS_TEST),x86_64)
> > +# set stack to 2MB on x64 build.  See bug 582910
> > +LDFLAGS += -STACK:2097152
> > +endif
> > +endif
> 
> I realize that you copied this snippet from browser/app/Makefile, and that's
> ok for now, but since we now have at least 4 different places in the tree
> we're sharing this snippet, please file a followup bug to unify that in a
> common makefile snippet.

Filed as bug 738571.


> > diff --git a/webapprt/win/app/webapprt.cpp b/webapprt/win/app/webapprt.cpp
> 
> > +namespace {
> > +  const char * APP_INI = "application.ini";
> 
> ditto comments on the mac version, this should be "const char kAPP_INI[] =
> "...";

Fixed: https://github.com/michaelrhanson/mozilla-central/commit/1a868b70321cd91793360195ad11362720ddf3d3


> > +  bool AppendLeaf(char *base, const char *toAppend, int baseSize) {
> > +    if (strlen(base)
> > +      + sizeof(XPCOM_FILE_PATH_SEPARATOR[0])
> > +      + strlen(toAppend)
> > +      + 1
> > +      > baseSize) {
> > +      return false;
> > +    }
> > +
> > +    if(base[strlen(base)-1] != XPCOM_FILE_PATH_SEPARATOR[0]) {
> > +        base[strlen(base)+1] = '\0';
> > +        base[strlen(base)] = XPCOM_FILE_PATH_SEPARATOR[0];
> > +    }
> > +
> > +    strcpy(base + strlen(base), toAppend);
> > +    return true;
> > +  }
> 
> This kind of string manipulation seems wrong to me. Is there a reason we
> can't use std::string or something like that? It appears to be correct.

I've rewritten webapprt.cpp using a slightly different style of string manipulation; it should be  easier to read and maintain, and generally looks less wrong :).

New approach implemented in this commit - https://github.com/michaelrhanson/mozilla-central/commit/1a868b70321cd91793360195ad11362720ddf3d3

Both approaches were based on  the type of string manipulation found in nsBrowserApp.cpp ( https://mxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp#207 ).  I'm not sure why we don't use std::string there, but I thought that, in general, Firefox code avoids using the STL to avoid having exceptions thrown at us?

Even if we use a string class, we'll have to use character buffers for some of the functions we call (GetModuleFilename, WideCharToMultibyte, MultibyteToWideChar).  We'll also have to check that path separators are in the right places and that paths don't exceed MAXPATHLEN.


> Also, is there a particular reason you need this to be fallible? I assume
> that if this code fails, we exit pretty quickly anyway, right? If so, can we
> just _exit here and avoid propagating return codes?

It is possible that a string manipulation fails, causing us to give up on the InstallDir value in webapp.ini, but that we succeed in using the registry to look up the location of Firefox.


> > +  nsresult AttemptGRELoad(char *greDir) {
> > +    nsresult rv;
> > +
> > +    AppendLeaf(greDir, XPCOM_DLL, MAXPATHLEN);
> > +    rv = XPCOMGlueStartup(greDir);
> > +    StripLeaf(greDir);
> > +    if(NS_FAILED(rv)) {
> > +      return rv;
> > +    }
> > +
> > +    rv = XPCOMGlueLoadXULFunctions(kXULFuncs);
> > +    if(NS_FAILED(rv)) {
> > +      return rv;
> > +    }
> > +
> > +    SetDllDirectoryA(greDir);
> 
> Can you explain why this is necessary? We explicitly SetDllDirectoryW(L"")
> in various places, and it shouldn't be necessary if the XPCOM glue load is
> working correctly.

The cubeb_init function called from nsAudioStream::InitLibrary ( https://mxr.mozilla.org/mozilla-central/source/content/media/nsAudioStream.cpp#360 ) exists in gkmedias.dll.  If we don't call SetDllDirectory on the Firefox directory, gkmedias.dll is not found and the app crashes.


> > +  int CopyAndRun(const char *src, const char *dest) {
> 
> This code is not unicode-safe. You should assume that some of our users
> (especially Thai users) will be running in native Windows codepages which
> cannot express their own username and perhaps not the Firefox install path.
> This means that these paths either need to be wchar or use UTF8. "main" is
> getting its paths in UTF8, not the native charset, because of the automatic
> conversion from wmain. But in this case wchar is probably much easier, and
> you should be doing the UTF8 conversion much later, right before XRE_main is
> called.

Unicode concerns addressed in this commit - https://github.com/michaelrhanson/mozilla-central/commit/1a868b70321cd91793360195ad11362720ddf3d3


> > +int main(int argc, char* argv[])
> > +{
> > +  int result = 0;
> > +  int realArgc = 1;
> > +  char **realArgv = argv;
> > +  char appINIPath[MAXPATHLEN];
> > +  char rtINIPath[MAXPATHLEN];
> > +  char greDir[MAXPATHLEN];
> > +
> > +  // Get the path and version of our running executable
> > +  if (NS_FAILED(mozilla::BinaryPath::Get(argv[0], curEXE))) {
> > +    Output("Couldn't calculate the application directory.\n");
> > +    return 255;
> > +  }
> > +  Version runningVersion;
> > +  GetVersionFromPath(curEXE, runningVersion);
> 
> Again, I think this would be much better compiled-in rather than reading it
> off the disk.

We now compare a compiled-in buildid string against the buildid found in Firefox's application.ini file.

Mac commit: https://github.com/michaelrhanson/mozilla-central/commit/86d08a7a006e0f48053ed2701e48f33af4d507b4

Windows commit: https://github.com/michaelrhanson/mozilla-central/commit/c681630a3c85d26577cfbfcf6f6058845551190c


> > +  { // Scope for any XPCOM stuff we create
> 
> Again use NS_LogInit/NS_LogTerm to scope this for leak-checking in debug
> builds.

Fixed: https://github.com/michaelrhanson/mozilla-central/commit/1a868b70321cd91793360195ad11362720ddf3d3


> I did not review any of the uninstaller work.

Robert strong is doing the review of the uninstaller work in bug 735518: https://bugzilla.mozilla.org/show_bug.cgi?id=735518


> In general, this looks really close. We should talk about the XPCOM glue
> loading issues with paths (both DYLD_ setting on Mac and SetDllDirectory on
> Windows shouldn't be necessary).

Comment 28

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

Attachment: patch v2: address review comments, additional fixes

Here's a followup patch that addresses the review comments from the first patch and sports some additional fixes, in particular:

While investigating problems with the way we quit the app when the user closes the last window on Mac, I realized that we don't actually need a hidden window, since the purpose of the hidden window is to keep the app running (and its menus working) when all windows are closed, and we plan to quit in that case.

But it wasn't sufficient to simply not specify browser.hiddenWindowChromeURL, because WebappRT lives in the same directory as Firefox and thus inherits Firefox's preferences.

After some investigation, I made a simple modification to nsAppShellService::CreateHiddenWindow that enables the behavior I want: when a xulapp sets browser.hiddenWindowChromeURL to the default hidden window URL (DEFAULT_HIDDENWINDOW_URL, which is defined as resource://gre-resources/hiddenWindow.html), treat it as if it was not set by the xulapp.

With this change, not only do we not need a hidden window (which simplifies the XUL/JS code drastically, since we no longer have to share a bunch of code between two XUL files), but the xulapp quits when the user closes the last window without us having to fiddle with *LastWindowClosingSurvivalArea or call quit ourselves in a hacky fashion.

Benjamin: I know the nsAppShellService change itself is something of a hack, but it seems to work, it doesn't look like it'll have negative side-effects, and I don't see a simpler way to accomplish the goal.

In the long run, as you noted, WebappRT, Firefox, and the GRE should live in their own directories/omnijars, at which point there would be no overlapping preferences to contend with.  But that seems like a much more significant and regression-prone change at this point.


Note: this patch does not include the following new files in our development repository, as timA is getting review on them in other bugs:

toolkit/mozapps/installer/windows/nsis/makensis.mk
webapprt/locales/en-US/webapp-uninstaller/ <--exclude all files in this dir
webapprt/win/webapp-uninstaller.nsi
webapprt/WindowsIconEmbeddingService.jsm
webapprt/WindowsShortcutService.jsm


(In reply to Dão Gottwald [:dao] from comment #24)
> Comment on attachment 608664 [details] [diff] [review]
> patch v1: initial implementation for Windows and Mac
> 
> >+++ b/webapprt/content/doctype.inc
> >+<!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd" >
> 
> >+++ b/webapprt/content/hiddenWindow.xul
> >+<?xul-overlay href="chrome://browser/content/baseMenuOverlay.xul"?>
> 
> >+++ b/webapprt/content/webappWindow.xul
> >+<?xul-overlay href="chrome://browser/content/baseMenuOverlay.xul"?>
> 
> These look like invalid dependencies on browser/.

Our thinking was to reuse browser code where possible, but on second thought, the dependencies seem too tenuous and prone to bustage, and it would be better to avoid them. It might make sense to move some of the shared code into toolkit, like the localizations for common Edit menuitems; but we can and probably should do that refactoring in a separate bug.

So I've copied the strings to a WebappRT-specific file. <https://github.com/michaelrhanson/mozilla-central/commit/fd1821e13a8dc0bd2a76f2011f29991778b154f6>. And removed the dependency on baseMenuOverlay.xul <https://github.com/michaelrhanson/mozilla-central/commit/70a699c094e65ef1a750925fa1e381885dbb937c>.


(In reply to Gavin Sharp (use [e-mail] for email) from comment #25)
> Comment on attachment 608664 [details] [diff] [review]
> patch v1: initial implementation for Windows and Mac
> 
> >diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
> 
> >+#ifndef XP_UNIX
> >+@BINPATH@/webapprt.exe
> 
> Use @BIN_SUFFIX@?

Usage is inconsistent, but @BIN_SUFFIX@ seems to be preferable. Fixed <https://github.com/michaelrhanson/mozilla-central/commit/8f102458a0f7336549947cd1275cc90bf544a100>.


> >diff --git a/browser/locales/en-US/webapprt/window.properties b/browser/locales/en-US/webapprt/window.properties
> 
> >+# LOCALIZATION NOTE (quitApplicationCmdMac.label): %S will be replaced with
> >+# the name of the webapp.
> >+quitApplicationCmdMac.label=Quit %S
> 
> Some more context about what "the web app" means here would probably be
> useful. In particular, explaining that this string doesn't appear in Firefox
> proper, but rather in the "app" window when it is run separately, etc.

Wouldn't it be better to provide this context for the browser/locales/en-US/webapprt/ directory as a whole rather than this one string? I've already added a second string to this properties file, plus there's now a DTD file in the directory. It seems like it would be better to explain WebappRT to localizers for the directory as a whole rather than for each individual string.

However, I don't see prior art on this. There are several subdirectories in browser/locales/en-US/ for ride-along executables: crashreporter/, installer/, and update/. And there's feedback/, which is a ride-along feature implemented as a loosely-coupled extension. But none appear to have explanations of the nature of the component.

So I added a README file to the directory that explains its purpose <https://github.com/michaelrhanson/mozilla-central/commit/471d345cb95ea77096aa2a5f8b87582e95b9b9fb>. But alternative suggestions welcome!


> >diff --git a/webapprt/WebappRT.jsm b/webapprt/WebappRT.jsm
> 
> >+let WebappRT = {
> >+  get config() {
> 
> I imagine it isn't possible for this to use non-main-thread-blocking I/O?

The shell needs some information from the configuration file to load the webapp. It could obtain it asynchronously, but that wouldn't make loading the webapp any faster.

Also, the directory provider needs a value from the configuration file to provide a directory, and that XPCOM API requires synchronicity.

We could go to extremes to isolate the stuff that needed to be loaded synchronously from the rest of the stuff in that file, but that seems like overkill.


> >diff --git a/webapprt/WebappRTCommandLineHandler.js b/webapprt/WebappRTCommandLineHandler.js
> 
> >+CommandLineHandler.prototype = {
> 
> >+  handle: function handle(cmdLine) {
> >+    Services.ww.openWindow(null,
> >+                           Services.prefs.getCharPref("browser.chromeURL"),
> 
> Given that this seems to be used completely separately, can you use a pref
> name other than browser.chromeURL, to avoid confusion/potential conflict?
> webapprt.chromeWindowURL? Actually, does this need to be pref-customizable
> at all? Why not just hard code the URL in the command line handler?

It is unclear whether or not we can get away with hardcoding the URL in the command line handler. It would certainly work for the startup case, but there is (at least) one other code path that references that preference, in nsXULWindow::CreateNewContentWindow <http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#1759>; and its existence may be assumed by other platform code, test harnesses, etc., especially given that it seems to be used by the other xulapps in the tree.

So while the preference might not be needed, and it would certainly simplify the implementation to hardcode the value, it seems safer to provide it.


> >diff --git a/webapprt/content/doctype.inc b/webapprt/content/doctype.inc
> 
> Seems like it would be simpler to just include these couple of lines
> directly in both files (I find the indirection more confusing than helpful
> for so little boilerplate).

Good point! Done <https://github.com/michaelrhanson/mozilla-central/commit/16cc831051239463393b484b300eaaac5857e0f1>.


> >diff --git a/webapprt/content/hiddenWindow.xul b/webapprt/content/hiddenWindow.xul
> 
> >+        xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#">
> 
> unused

Removed! <https://github.com/michaelrhanson/mozilla-central/commit/ce6c13a780cf22ce08228eef1e1c1a9f2a33ff32>


> >diff --git a/webapprt/content/menubar.inc b/webapprt/content/menubar.inc
> 
> >+<menubar id="main-menubar"
> >+         onpopupshowing="if (event.target.parentNode.parentNode == this &amp;&amp;
> >+                            !('@mozilla.org/widget/nativemenuservice;1' in Cc))
> >+                           this.setAttribute('openedwithkey',
> >+                                             event.target.parentNode.openedWithKey);"
> >+         style="border:0px;padding:0px;margin:0px;-moz-appearance:none">
> 
> A helper function in window.js for the script would be nice (avoid the gross
> XML-escaping of &&).

It  looks like that onpopupshowing attribute (which we copied from  browser's equivalent) was added to support a change to Bookmark All Tabs  <http://hg.mozilla.org/mozilla-central/rev/e2f9d9c8b18c>, which isn't relevant in our case, so removed! <https://github.com/michaelrhanson/mozilla-central/commit/07c998c722864c32fff827df8d7472e298661a02>


> Are these style rules really needed?

They exist in browser's version of the menubar from the initial commit of browser-menubar.inc back on 2003-08-11 (no good link to this, but here's the list of revisions <http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/browser/base/content/browser-menubar.inc&root=/cvsroot> and the diff between 1.1 and 1.2 <http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=browser-menubar.inc&branch=&root=/cvsroot&subdir=mozilla/browser/base/content&command=DIFF_FRAMESET&rev1=1.1&rev2=1.2>).

However, I removed them, and the menubar seems to still work, so perhaps they are no longer needed. <https://github.com/michaelrhanson/mozilla-central/commit/af6281fba54fb0e8bd8bf575d979a4f4e856044f>


> >diff --git a/webapprt/content/scripts.inc b/webapprt/content/scripts.inc
> 
> >+<script type="application/javascript" src="chrome://global/content/printUtils.js"/>
> >+<script type="application/javascript" src="chrome://global/content/viewZoomOverlay.js"/>
> >+<script type="application/javascript" src="chrome://global/content/inlineSpellCheckUI.js"/>
> >+<script type="application/javascript" src="chrome://global/content/viewSourceUtils.js"/>
> 
> Are these really all needed/useful?

Hmm, I don't think so, at least not initially. And better to add them as we need them to enhance webapp functionality than to leave them in until then. Removed. <https://github.com/michaelrhanson/mozilla-central/commit/5345b78a258cb7fdaa55815757b161c937d0681e>


> >diff --git a/webapprt/content/sets.inc b/webapprt/content/sets.inc
> 
> >+  <command id="cmd_close" oncommand="BrowserCloseTabOrWindow()"/>
> >+  <command id="View:FullScreen" oncommand="BrowserFullScreen();"/>
> 
> These seem to refer to undefined functions.

We removed these <https://github.com/michaelrhanson/mozilla-central/commit/017520475b8103993159a2e31dd52d8056108921>, since we don't provide this functionality (yet).


> >diff --git a/webapprt/content/webappWindow.xul b/webapprt/content/webappWindow.xul
> 
> >+<?xul-overlay href="chrome://browser/content/baseMenuOverlay.xul"?>
> 
> I guess this is only for macWindowMenu.inc?

We don't actually have a Window menu; this is for the Application menuitems that the overlay provides; although those weren't showing up because of a bug. I fixed the bug while removing the dependency on baseMenuOverlay.xul <https://github.com/michaelrhanson/mozilla-central/commit/70a699c094e65ef1a750925fa1e381885dbb937c>.


> >+<window windowtype="webapprt:webapp"
> 
> >+        xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
> >+        xmlns:svg="http://www.w3.org/2000/svg"
> 
> These don't appear to be used here.

Indeed. Removed! <https://github.com/michaelrhanson/mozilla-central/commit/afdd6c26730b5087358a84f303d16b5953d3d6f6>


> >diff --git a/webapprt/content/window.js b/webapprt/content/window.js
> 
> >+function updateEditUIVisibility() {
> 
> >+  let contextMenuPopupState = document.getElementById("contentAreaContextMenu").state;
> >+  let placesContextMenuPopupState = document.getElementById("placesContext").state;
> >+#ifdef MENUBAR_CAN_AUTOHIDE
> >+  let appMenuPopupState = document.getElementById("appmenu-popup").state;
> >+#endif
> 
> These elements don't seem to exist in this code... Doesn't this throw?

Erm, yes, it does. Fixed. <https://github.com/michaelrhanson/mozilla-central/commit/b2ede0833854a78d8f40fbe72ac6c869eab83abd>


> >diff --git a/webapprt/win/webapprt.cpp b/webapprt/win/webapprt.cpp
> 
> >+  /*
> >+  const Version runningVersion = { MOZILLA_MAJOR_VERSION,
> >+                                   MOZILLA_MINOR_VERSION,
> >+                                   MOZILLA_MINI_VERSION,
> >+                                   MOZILLA_MICRO_VERSION };
> >+  */
> 
> This should have an XXX comment/bug reference for the followup to use the
> compile-time values.

We transitioned to comparing buildid rather than file version: https://github.com/michaelrhanson/mozilla-central/commit/c681630a3c85d26577cfbfcf6f6058845551190c#L7L632

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #28)
> So I added a README file to the directory that explains its purpose
> <https://github.com/michaelrhanson/mozilla-central/commit/
> 471d345cb95ea77096aa2a5f8b87582e95b9b9fb>. But alternative suggestions
> welcome!

This is good, but it's unlikely that most localizers will notice the README. Better would be to put the comment in the properties file itself, as a LOCALIZATION NOTE (see https://developer.mozilla.org/en/Localization_notes).
 
> We could go to extremes to isolate the stuff that needed to be loaded
> synchronously from the rest of the stuff in that file, but that seems like
> overkill.

I agree.

> It is unclear whether or not we can get away with hardcoding the URL in the
> command line handler. It would certainly work for the startup case, but
> there is (at least) one other code path that references that preference, in
> nsXULWindow::CreateNewContentWindow

This code is used to create a new top level window. Since I don't think you want that to happen in webapps, it may actually be better for this code to fail. But it doesn't look possible to have it fail cleanly at this level, so you're right that it's probably best to keep the preference.

Comment 30

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #27)
>
> [...]
>
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #20)
> >
> > [...]
> >
> > diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
> > @@ -681,6 +694,13 @@ nsXREDirProvider::GetFilesInternal(const char* aProperty,
> >                        kAppendChromeDir,
> >                        directories);
> >  
> > +    nsCOMPtr<nsILocalFile> profileDir;
> > +    rv = GetUserDataDirectory(getter_AddRefs(profileDir), false);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    LoadDirIntoArray(profileDir,
> > +                     kAppendChromeDir,
> > +                     directories);
> > 
> > I don't understand this change at all. It appears to be loading a chrome
> > manifest from ~/Library/Application Support/Firefox/chrome/chrome.manifest,
> > but that is never a feature we have supported before and is not one I think
> > we should start supporting now.
> 
> On Windows, we want Firefox-as-XULRunner to use the icon that lives with the
> app.  We accomplished that by putting the icon in
> ${AppDir}\chrome\icons\default\topwindow.ico and making this change.  Sounds
> like we'll need to do some more fiddling...
> 
> (Benjamin later replied in the etherpad: "I still don't understand this,
> since chrome manifests aren't used for .ico searching.")
>
> [...]
>

This is the code that does the icon searching:
  https://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseWidget.cpp?rev=db7260efc9a7#1255
It checks for .ico files in a "icons/default" subdirectory of every dir in NS_APP_CHROME_DIR_LIST.

According to a comment in nsXREDirProvider, NS_APP_CHROME_DIR_LIST is only used for icon searching:
  https://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp?rev=d228248c1b5a#669

Doing a quick MXR search, the comment seems accurate:
  https://mxr.mozilla.org/mozilla-central/ident?i=NS_APP_CHROME_DIR_LIST

It seems like the name NS_APP_CHROME_DIR_LIST is pretty misleading, which led to the confusion here.

If we want to avoid this modification to nsXREDirProvider, we could modify nsWindow::SetIcon to use the embedded icon of the running EXE if it does not find a suitable .ico file.  This would obviously have side effects for other apps, but it seems like it is reasonable behavior for all apps.

Comment 31

[Dietrich Ayala (:dietrich)]

Comment on attachment 610800 [details] [diff] [review]
patch v2: address review comments, additional fixes

Review of attachment 610800 [details] [diff] [review]:
-----------------------------------------------------------------

Gavin and Dao covered everything (and more) that I saw.

I'm still concerned that the config file is read synchronously and fairly late in the loading process. But we can address that if/when it shows itself to be a problem. I'd feel better with some telemetry for where in the startup timeline that happens and how long it takes, but don't need to block landing on that.

Please overcommunicate the README to l10n, preferably by posting to dev.l10n about it before landing.

::: webapprt/content/webapp.js
@@ +20,5 @@
> +  document.documentElement.setAttribute("title", manifest.name);
> +
> +  // Load the webapp's launch path.
> +  let url = installRecord.origin;
> +  // XXX Resolve launch path relative to origin instead of merely concating 'em?

file a bug and note the # here?

::: webapprt/content/webapp.xul
@@ +66,5 @@
> +       - (from which nsMenuBarX still moves it properly), and we don't create
> +       - the File menu in the first place on that OS.
> +       -
> +       - XXX Move the Mac Quit item back up here after adding the first item
> +       - that always shows up in the File menu on Mac. -->

file a bug and note the # here? XXX has a tendency to foreverize.

Comment 32

[Jason Smith [:jsmith]]

> I'm still concerned that the config file is read synchronously and fairly
> late in the loading process. But we can address that if/when it shows itself
> to be a problem. I'd feel better with some telemetry for where in the
> startup timeline that happens and how long it takes, but don't need to block
> landing on that.

From the QA perspective, how could an end user be affected by this? Just wondering what I should watch out for when I start doing initial test runs for this feature.

Comment 33

[Dietrich Ayala (:dietrich)]

(In reply to Jason Smith from comment #32)
> > I'm still concerned that the config file is read synchronously and fairly
> > late in the loading process. But we can address that if/when it shows itself
> > to be a problem. I'd feel better with some telemetry for where in the
> > startup timeline that happens and how long it takes, but don't need to block
> > landing on that.
> 
> From the QA perspective, how could an end user be affected by this? Just
> wondering what I should watch out for when I start doing initial test runs
> for this feature.

You'd click to open an app and would see varying amounts of time taken for it to become ready to use.

It's not immediately clear to me whether this would manifest itself as:

* nothing happening and then the whole app becoming visible
* empty window frame becoming visible, and then app contents loading in it
* some bit of both of the above

Comment 34

[Jason Smith [:jsmith]]

> You'd click to open an app and would see varying amounts of time taken for
> it to become ready to use.
> 
> It's not immediately clear to me whether this would manifest itself as:
> 
> * nothing happening and then the whole app becoming visible
> * empty window frame becoming visible, and then app contents loading in it
> * some bit of both of the above

I wonder if this situation you are describing is similar to a past extension bug (bug 707291). I saw cases on Mac OS X where launching a native application would non-deterministically take up to 30 seconds to launch. Although that bug is more about downloading contents, I've seen this with other apps as well.

Comment 35

[Benjamin Smedberg]

Attachment: Patch v2 review

Comment 36

[Benjamin Smedberg]

Comment on attachment 610800 [details] [diff] [review]
patch v2: address review comments, additional fixes

r=me with lots of nits and a few hunk removals as noted in the review doc

Comment 37

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

Attachment: patch v3: addresses review nits

(In reply to Gavin Sharp (use [e-mail] for email) from comment #29)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #28)
> > So I added a README file to the directory that explains its purpose
> > <https://github.com/michaelrhanson/mozilla-central/commit/
> > 471d345cb95ea77096aa2a5f8b87582e95b9b9fb>. But alternative suggestions
> > welcome!
> 
> This is good, but it's unlikely that most localizers will notice the README.
> Better would be to put the comment in the properties file itself, as a
> LOCALIZATION NOTE (see https://developer.mozilla.org/en/Localization_notes).

Ah, indeed.
https://github.com/michaelrhanson/mozilla-central/commit/8a41b93d56b927fbea41d9537285d7be8d3627e6


(In reply to Dietrich Ayala (:dietrich) from comment #31)
> I'm still concerned that the config file is read synchronously and fairly
> late in the loading process. But we can address that if/when it shows itself
> to be a problem. I'd feel better with some telemetry for where in the
> startup timeline that happens and how long it takes, but don't need to block
> landing on that.

We could read it earlier, but we'd still need to read it synchronously, so that wouldn't buy us any webapp startup time. Reading it lazily late in the loading process is intended to be a feature, not a bug, as it blocks as little of the loading process as possible.

And the measures we could take to optimize read performance (f.e. separating the data essential to loading a webapp into its own JSON file) seem premature, given the small size of the file.

But I'll keep it in mind in case it proves to be an issue in the future.


> Please overcommunicate the README to l10n, preferably by posting to dev.l10n
> about it before landing.

Good idea, I'll make sure to do that. I'm already subscribed to the mailing list.


> ::: webapprt/content/webapp.js
> @@ +20,5 @@
> > +  document.documentElement.setAttribute("title", manifest.name);
> > +
> > +  // Load the webapp's launch path.
> > +  let url = installRecord.origin;
> > +  // XXX Resolve launch path relative to origin instead of merely concating 'em?
> 
> file a bug and note the # here?

It turned out to be quicker to just fix it.
https://github.com/michaelrhanson/mozilla-central/commit/b62cceaceba9d1a1f57b319397cbd79a0a98fe5e

However, I did file bug 744491 for the XXX above it (dynamically updating the page's title), and then I removed that XXX, since the fix may land elsewhere in the codebase, and thus the bug fixer might not see that note.
https://github.com/michaelrhanson/mozilla-central/commit/04efe80d38fbbb8f3e4ee50d5f46cd8f32e8e192

And in general, I hear you loud and clear: bugs are better than XXXs for identifying work that needs to be done.


> ::: webapprt/content/webapp.xul
> @@ +66,5 @@
> > +       - (from which nsMenuBarX still moves it properly), and we don't create
> > +       - the File menu in the first place on that OS.
> > +       -
> > +       - XXX Move the Mac Quit item back up here after adding the first item
> > +       - that always shows up in the File menu on Mac. -->
> 
> file a bug and note the # here? XXX has a tendency to foreverize.

In this case, a bug report would be premature, since we should only do this if we add a persistent menu item to the File menu on Mac, which we don't yet plan to do. But perhaps the triple X is misleading, so I have removed it and clarified the comment it prefaces.
https://github.com/michaelrhanson/mozilla-central/commit/6bd30753fd7f709d759bcc80834a54dfb9a8b3cc


(In reply to Benjamin Smedberg from comment #36)
> > +#ifdef MOZ_WEBAPP_RUNTIME
> > +; [Webapp Runtime]
> > +#ifndef XP_UNIX
> > +@BINPATH@/webapprt@BIN_SUFFIX@
> > +#else
> > +@BINPATH@/webapprt
> > +#endif
>
> I don't think this ifdef is necessary, since BIN_SUFFIX is empty on unix

Indeed!
https://github.com/michaelrhanson/mozilla-central/commit/42b4fefc8754831a074fee5c62c58e7ce4d8ac29


> > diff --git a/configure.in b/configure.in
> > index 42b1bd5..1bca316 100644
> > --- a/configure.in
> > +++ b/configure.in
> > @@ -2151,6 +2151,10 @@ fi
> >  dnl Get mozilla version from central milestone file
> >  MOZILLA_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir`
> >  MOZILLA_UAVERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -uaversion`
> > +MOZILLA_MAJOR_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -majorversion`
> > +MOZILLA_MINOR_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -minorversion`
> > +MOZILLA_MINI_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -miniversion`
> > +MOZILLA_MICRO_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -microversion`
> >  
> >  dnl Get version of various core apps from the version files.
> >  FIREFOX_VERSION=`cat $_topsrcdir/browser/config/version.txt`
> > @@ -2162,6 +2166,10 @@ fi
> >  AC_DEFINE_UNQUOTED(MOZILLA_VERSION,"$MOZILLA_VERSION")
> >  AC_DEFINE_UNQUOTED(MOZILLA_VERSION_U,$MOZILLA_VERSION)
> >  AC_DEFINE_UNQUOTED(MOZILLA_UAVERSION,"$MOZILLA_UAVERSION")
> > +AC_DEFINE_UNQUOTED(MOZILLA_MAJOR_VERSION,$MOZILLA_MAJOR_VERSION)
> > +AC_DEFINE_UNQUOTED(MOZILLA_MINOR_VERSION,$MOZILLA_MINOR_VERSION)
> > +AC_DEFINE_UNQUOTED(MOZILLA_MINI_VERSION,$MOZILLA_MINI_VERSION)
> > +AC_DEFINE_UNQUOTED(MOZILLA_MICRO_VERSION,$MOZILLA_MICRO_VERSION)
>
> I don't see anywhere in this patch where these values are used. Can you explain why this was necessary?

This is no longer necessary. It is a remnant of a different approach to testing whether two webapprt.exe files have different versions. Removed.
https://github.com/michaelrhanson/mozilla-central/commit/e7ed9bc7e039ba1561d70946b11128449a59da18
https://github.com/michaelrhanson/mozilla-central/commit/02be813b69cc78ef09b0ff76a19d4012bd217469
https://github.com/michaelrhanson/mozilla-central/commit/dcc5bf7e7d39490ed5ede2a13d2438c3dd7d894a


> > diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>
> > @@ -682,6 +691,13 @@ nsXREDirProvider::GetFilesInternal(const char* aProperty,
> >                        kAppendChromeDir,
> >                        directories);
> >  
> > +    nsCOMPtr<nsILocalFile> profileDir;
> > +    rv = GetUserDataDirectory(getter_AddRefs(profileDir), false);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    LoadDirIntoArray(profileDir,
> > +                     kAppendChromeDir,
> > +                     directories);
>
> Now that I understand you're trying to find the icon directory, I still don't believe that this is necessary or desirable: instead of the user directory, can we put these icons in the app location and load them from there? r- on this section only, for the purposes of getting everything else landed.

Removed so it doesn't block landing the rest of this patch.
https://github.com/michaelrhanson/mozilla-central/commit/709fda20543ac97a19931426f58ce316b033a643


> > diff --git a/webapprt/WebappRTCommandLineHandler.js b/webapprt/WebappRTCommandLineHandler.js
>
> > +CommandLineHandler.prototype = {
> > +  classID: Components.ID("{6d69c782-40a3-469b-8bfd-3ee366105a4a}"),
> > +
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsICommandLineHandler]),
> > +
> > +  handle: function handle(cmdLine) {
> > +    Services.ww.openWindow(null,
> > +                           Services.prefs.getCharPref("browser.chromeURL"),
> > +                           "_blank",
> > +                           "chrome,dialog=no,all",
> > +                           null);
>
> Is there any particular reason we need to use a pref for this? Don't we know that for webapprt the initial launch URL will always be chrome://webapprt/content/webapp.xul ? If so, please just put it directly in this file and skip the pref call.

It is unclear whether or not we can get away with hardcoding the URL in the command line handler. It would certainly work for the startup case, but there is (at least) one other code path that references that preference, in nsXULWindow::CreateNewContentWindow <http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#1759>; and its existence may be assumed by other platform code, test harnesses, etc., especially given that it seems to be used by the other xulapps in the tree.

So while the preference might not be needed, and it would certainly simplify the implementation to hardcode the value, it seems safer to provide it.


> > +++ b/webapprt/mac/webapprt.mm
>
> > +//we look for these flavors of Firefox, in this order
> > +NSArray* launchBinarySearchList = [NSArray arrayWithObjects: @"org.mozilla.nightly", 
> > +                                                              @"org.mozilla.aurora", 
> > +                                                              @"org.mozilla.beta", 
> > +                                                              @"org.mozilla.firefox", nil];
>
> For testing/QA, can this be controlled from the environment, either by specifying the bundle IDs or specifying a specific path to explicitly use as the runtime? I suspect that the path-based usage will be important for automated tests.

It can be controlled from the Info.plist file for testing.  Any binary can be used to launch the app, as described at the bottom here: https://etherpad.mozilla.org/webapprt-install-flow

As we stand up automated testing, if this facility proves insufficient, we'll add a way to control it.


> > +    //CHECK DYLD_FALLBACK_LIBRARY_PATH
> > +    char libEnv[MAXPATHLEN];
> > +    snprintf(libEnv, MAXPATHLEN, "%s%s", [firefoxPath UTF8String], APP_CONTENTS_PATH);
> > +
> > +    char* curVal = getenv("DYLD_FALLBACK_LIBRARY_PATH");
> > +
> > +    if ((curVal == NULL) || strncmp(libEnv, curVal, MAXPATHLEN)) 
> > +    {
> > +      //NOT SET! SET AND RELAUNCH
> > +      NSLog(@"DYLD_FALLBACK_LIBRARY_PATH NOT SET!!");
> > +      //they differ, so set it and relaunch
> > +      setenv("DYLD_FALLBACK_LIBRARY_PATH", libEnv, 1);
> > +      execNewBinary(myWebRTPath);
> > +      exit(0);
> > +    }
> > +    //HAVE DYLD_FALLBACK_LIBRARY_PATH
>
> This should no longer be necessary now that bug 738790 is being fixed properly. Please remove.

Removed!
https://github.com/michaelrhanson/mozilla-central/commit/eeb108adeb0e646f3d88c62ba076a894622c2d5a


> > +      //force myself to the foreground.  exec-ing a non .app bundle normally leaves you in the background
> > +      [NSApp activateIgnoringOtherApps:YES];
>
> I don't understand what this is doing here: we've already finished XRE_main and are in the shutdown process? Perhaps this was meant to go before XRE_main?

Due to the way we launch another process from within our Mac pseudo-app, the webapp ends up launching in the background.  This fixes it.


> > diff --git a/webapprt/win/webapprt.cpp b/webapprt/win/webapprt.cpp
>
> > +  #pragma pack(push, 2)
> > +  typedef struct
> > +  {
> > +    WORD Reserved;
> > +    WORD ResourceType;
> > +    WORD ImageCount;
> > +  } IconHeader;
> > +
> > +  typedef struct
> > +  {
> > +    BYTE Width;
> > +    BYTE Height;
> > +    BYTE Colors;
> > +    BYTE Reserved;
> > +    WORD Planes;
> > +    WORD BitsPerPixel;
> > +    DWORD ImageSize;
> > +    DWORD ImageOffset;
> > +  } IconDirEntry;
> > +
> > +  typedef struct
> > +  {
> > +    BYTE Width;
> > +    BYTE Height;
> > +    BYTE Colors;
> > +    BYTE Reserved;
> > +    WORD Planes;
> > +    WORD BitsPerPixel;
> > +    DWORD ImageSize;
> > +    WORD ResourceID;    // This field is the one difference to above
> > +  } IconResEntry;
> > +  #pragma pack(pop)
>
> Where do these structure declarations come from? Are there potential licensing issues that we need to be aware of?

timA: These structure declarations were copied from redit.cpp in xulrunner/tools/redit.  Dave Townsend wrote that file and it is MPL, but I'll follow up with him to make sure that there are no licensing issues here.


> > +  // Copied from toolkit/xre/nsAppData.cpp.
> > +  void
> > +  SetAllocatedString(const char *&str, const char *newvalue) {
>
> nit, brace in column 0 of the next line, here and below

Fixed!
https://github.com/michaelrhanson/mozilla-central/commit/fb7b9823088e12abef9e5398fe04cf4d6bce7e29


> > +  /**
> > +   * A helper class which calls XPCOMGlueStartup/XPCOMGlueShutdown in its scope.
> > +   */
> > +  class ScopedXPCOMGlue
> > +  {
> > +    public:
> > +      ScopedXPCOMGlue()
> > +        : mRv(NS_ERROR_FAILURE) { }
> > +      ~ScopedXPCOMGlue() {
> > +        if(NS_SUCCEEDED(mRv)) {
> > +          XPCOMGlueShutdown();
> > +        }
> > +      }
> > +      nsresult startup(const char * xpcomFile) {
> > +        return (mRv = XPCOMGlueStartup(xpcomFile));
> > +      }
> > +    private:
> > +      nsresult mRv;
> > +  };
>
> I really don't think this is necessary: XPCOMGlueShutdown is not a useful function in general, and we shouldn't be calling it.

Excellent, that makes webapprt.cpp cleaner. Removed.
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +  /**
> > +   * A helper class for scope-guarding a file handle.
> > +   */
> > +  class ScopedFile
>
> why do we need this instead of just using ScopedClose from FileUtils.h?

timA: I was unaware of ScopedClose :).  I'll switch to that.
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +  nsresult
> > +  embedIcon(wchar_t const * const src,
> > +            wchar_t const * const dst) {
>
> nits: name this EmbedIcon. The only caller doesn't appear to care about the return code, so just make it return void, and fix the opening brace.

https://github.com/michaelrhanson/mozilla-central/commit/3f29f030ddf838a573967d58582c23f7e9cbec74
https://github.com/michaelrhanson/mozilla-central/commit/940f033dc0162c361615dfe4e4ea2c3cd64fdfd5
https://github.com/michaelrhanson/mozilla-central/commit/fb7b9823088e12abef9e5398fe04cf4d6bce7e29


> > +    ScopedResourceUpdateHandle updateRes;
> > +
> > +    { // Scope for group
> > +      nsAutoArrayPtr<BYTE> group;
> > +      long groupSize;
> > +
> > +      { // Scope for data
> > +        nsAutoArrayPtr<BYTE> data;
> > +
> > +        { // Scope for file
> > +          ScopedFile file;
>
> Do all of these need to go out of scope separately? If so, please document why it's necessary to close the file before the end of the function. Otherwise, I wouldn't bother with the extra scope braces, they make the function harder to read.

timA: They do not need to go out of scope separately.  I've removed the extra scope braces.
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +  nsresult
> > +  AttemptCopyAndLaunch(wchar_t* src,
>
> This is the beginning of a bunch of functions which use nsresult failure codes for no obvious reason: this doesn't appear to be XPCOM code, and nobody is distinguishing between various failure codes. Why don't we just make this set of functions return booleans?

Done!
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +                       int* result) {
> > +    // Rename the old app executable
> > +    if(FALSE == ::MoveFileExW(curExePath,
>
> nit, unsnuggle the paren here and several more times below

Done!
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +    if(!CreateProcessW(curExePath, // No module name (use command line)
> > +                       NULL,             // Command line
> > +                       NULL,             // Process handle not inheritable
> > +                       NULL,             // Thread handle not inheritable
> > +                       FALSE,            // Set handle inheritance to FALSE
> > +                       0,                // No creation flags
> > +                       NULL,             // Use parent's environment block
> > +                       NULL,             // Use parent's starting directory 
> > +                       &si,
> > +                       &pi)) {
> > +      return NS_ERROR_FAILURE;
> > +    }
> > +
> > +    // Wait until child process exits.
> > +    WaitForSingleObject(pi.hProcess, INFINITE);
>
> Why are we waiting for the app to exit? All of the other cases where we relaunch ourself (for updates or extension installation) we exit the first process immediately after launching the new one.

The idea was to have a meaningful exit code, but it seems that the exit code will not always be meaningful anyway.  Removed.
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d
(In reply to Gavin Sharp (use [e-mail] for email) from comment #29)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #28)
> > So I added a README file to the directory that explains its purpose
> > <https://github.com/michaelrhanson/mozilla-central/commit/
> > 471d345cb95ea77096aa2a5f8b87582e95b9b9fb>. But alternative suggestions
> > welcome!
> 
> This is good, but it's unlikely that most localizers will notice the README.
> Better would be to put the comment in the properties file itself, as a
> LOCALIZATION NOTE (see https://developer.mozilla.org/en/Localization_notes).

Ah, indeed.
https://github.com/michaelrhanson/mozilla-central/commit/8a41b93d56b927fbea41d9537285d7be8d3627e6


(In reply to Dietrich Ayala (:dietrich) from comment #31)
> I'm still concerned that the config file is read synchronously and fairly
> late in the loading process. But we can address that if/when it shows itself
> to be a problem. I'd feel better with some telemetry for where in the
> startup timeline that happens and how long it takes, but don't need to block
> landing on that.

We could read it earlier, but we'd still need to read it synchronously, so that wouldn't buy us any webapp startup time. Reading it lazily late in the loading process is intended to be a feature, not a bug, as it blocks as little of the loading process as possible.

And the measures we could take to optimize read performance (f.e. separating the data essential to loading a webapp into its own JSON file) seem premature, given the small size of the file.


> Please overcommunicate the README to l10n, preferably by posting to dev.l10n
> about it before landing.

Good idea, I'll make sure to do that. I'm already subscribed to the mailing list.


> ::: webapprt/content/webapp.js
> @@ +20,5 @@
> > +  document.documentElement.setAttribute("title", manifest.name);
> > +
> > +  // Load the webapp's launch path.
> > +  let url = installRecord.origin;
> > +  // XXX Resolve launch path relative to origin instead of merely concating 'em?
> 
> file a bug and note the # here?

It turned out to be quicker to just fix it.
https://github.com/michaelrhanson/mozilla-central/commit/b62cceaceba9d1a1f57b319397cbd79a0a98fe5e

However, I did file bug 744491 for the XXX above it (dynamically updating the page's title), and then I removed that XXX, since the fix may land elsewhere in the codebase, and thus the bug fixer might not see that note.
https://github.com/michaelrhanson/mozilla-central/commit/04efe80d38fbbb8f3e4ee50d5f46cd8f32e8e192

And in general, I hear you loud and clear: bugs are better than XXXs for identifying work that needs to be done.


> ::: webapprt/content/webapp.xul
> @@ +66,5 @@
> > +       - (from which nsMenuBarX still moves it properly), and we don't create
> > +       - the File menu in the first place on that OS.
> > +       -
> > +       - XXX Move the Mac Quit item back up here after adding the first item
> > +       - that always shows up in the File menu on Mac. -->
> 
> file a bug and note the # here? XXX has a tendency to foreverize.

In this case, a bug report would be premature, since we should only do this if we add a persistent menu item to the File menu on Mac, which we don't yet plan to do. But perhaps the triple X is misleading, so I have removed it and clarified the comment it prefaces.
https://github.com/michaelrhanson/mozilla-central/commit/6bd30753fd7f709d759bcc80834a54dfb9a8b3cc


(In reply to Benjamin Smedberg from comment #36)
> > +#ifdef MOZ_WEBAPP_RUNTIME
> > +; [Webapp Runtime]
> > +#ifndef XP_UNIX
> > +@BINPATH@/webapprt@BIN_SUFFIX@
> > +#else
> > +@BINPATH@/webapprt
> > +#endif
>
> I don't think this ifdef is necessary, since BIN_SUFFIX is empty on unix

Indeed!
https://github.com/michaelrhanson/mozilla-central/commit/42b4fefc8754831a074fee5c62c58e7ce4d8ac29


> > diff --git a/configure.in b/configure.in
> > index 42b1bd5..1bca316 100644
> > --- a/configure.in
> > +++ b/configure.in
> > @@ -2151,6 +2151,10 @@ fi
> >  dnl Get mozilla version from central milestone file
> >  MOZILLA_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir`
> >  MOZILLA_UAVERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -uaversion`
> > +MOZILLA_MAJOR_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -majorversion`
> > +MOZILLA_MINOR_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -minorversion`
> > +MOZILLA_MINI_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -miniversion`
> > +MOZILLA_MICRO_VERSION=`$PERL $srcdir/config/milestone.pl -topsrcdir $srcdir -microversion`
> >  
> >  dnl Get version of various core apps from the version files.
> >  FIREFOX_VERSION=`cat $_topsrcdir/browser/config/version.txt`
> > @@ -2162,6 +2166,10 @@ fi
> >  AC_DEFINE_UNQUOTED(MOZILLA_VERSION,"$MOZILLA_VERSION")
> >  AC_DEFINE_UNQUOTED(MOZILLA_VERSION_U,$MOZILLA_VERSION)
> >  AC_DEFINE_UNQUOTED(MOZILLA_UAVERSION,"$MOZILLA_UAVERSION")
> > +AC_DEFINE_UNQUOTED(MOZILLA_MAJOR_VERSION,$MOZILLA_MAJOR_VERSION)
> > +AC_DEFINE_UNQUOTED(MOZILLA_MINOR_VERSION,$MOZILLA_MINOR_VERSION)
> > +AC_DEFINE_UNQUOTED(MOZILLA_MINI_VERSION,$MOZILLA_MINI_VERSION)
> > +AC_DEFINE_UNQUOTED(MOZILLA_MICRO_VERSION,$MOZILLA_MICRO_VERSION)
>
> I don't see anywhere in this patch where these values are used. Can you explain why this was necessary?

This is no longer necessary. It is a remnant of a different approach to testing whether two webapprt.exe files have different versions. Removed.
https://github.com/michaelrhanson/mozilla-central/commit/e7ed9bc7e039ba1561d70946b11128449a59da18
https://github.com/michaelrhanson/mozilla-central/commit/02be813b69cc78ef09b0ff76a19d4012bd217469
https://github.com/michaelrhanson/mozilla-central/commit/dcc5bf7e7d39490ed5ede2a13d2438c3dd7d894a


> > diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>
> > @@ -682,6 +691,13 @@ nsXREDirProvider::GetFilesInternal(const char* aProperty,
> >                        kAppendChromeDir,
> >                        directories);
> >  
> > +    nsCOMPtr<nsILocalFile> profileDir;
> > +    rv = GetUserDataDirectory(getter_AddRefs(profileDir), false);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    LoadDirIntoArray(profileDir,
> > +                     kAppendChromeDir,
> > +                     directories);
>
> Now that I understand you're trying to find the icon directory, I still don't believe that this is necessary or desirable: instead of the user directory, can we put these icons in the app location and load them from there? r- on this section only, for the purposes of getting everything else landed.

Removed so it doesn't block landing the rest of this patch.
https://github.com/michaelrhanson/mozilla-central/commit/709fda20543ac97a19931426f58ce316b033a643


> > diff --git a/webapprt/WebappRTCommandLineHandler.js b/webapprt/WebappRTCommandLineHandler.js
>
> > +CommandLineHandler.prototype = {
> > +  classID: Components.ID("{6d69c782-40a3-469b-8bfd-3ee366105a4a}"),
> > +
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsICommandLineHandler]),
> > +
> > +  handle: function handle(cmdLine) {
> > +    Services.ww.openWindow(null,
> > +                           Services.prefs.getCharPref("browser.chromeURL"),
> > +                           "_blank",
> > +                           "chrome,dialog=no,all",
> > +                           null);
>
> Is there any particular reason we need to use a pref for this? Don't we know that for webapprt the initial launch URL will always be chrome://webapprt/content/webapp.xul ? If so, please just put it directly in this file and skip the pref call.

It is unclear whether or not we can get away with hardcoding the URL in the command line handler. It would certainly work for the startup case, but there is (at least) one other code path that references that preference, in nsXULWindow::CreateNewContentWindow <http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#1759>; and its existence may be assumed by other platform code, test harnesses, etc., especially given that it seems to be used by the other xulapps in the tree.

So while the preference might not be needed, and it would certainly simplify the implementation to hardcode the value, it seems safer to provide it.


> > +++ b/webapprt/mac/webapprt.mm
>
> > +//we look for these flavors of Firefox, in this order
> > +NSArray* launchBinarySearchList = [NSArray arrayWithObjects: @"org.mozilla.nightly", 
> > +                                                              @"org.mozilla.aurora", 
> > +                                                              @"org.mozilla.beta", 
> > +                                                              @"org.mozilla.firefox", nil];
>
> For testing/QA, can this be controlled from the environment, either by specifying the bundle IDs or specifying a specific path to explicitly use as the runtime? I suspect that the path-based usage will be important for automated tests.

It can be controlled from the Info.plist file for testing.  Any binary can be used to launch the app, as described at the bottom here: https://etherpad.mozilla.org/webapprt-install-flow

As we stand up automated testing, if this facility proves insufficient, we'll add a way to control it.


> > +    //CHECK DYLD_FALLBACK_LIBRARY_PATH
> > +    char libEnv[MAXPATHLEN];
> > +    snprintf(libEnv, MAXPATHLEN, "%s%s", [firefoxPath UTF8String], APP_CONTENTS_PATH);
> > +
> > +    char* curVal = getenv("DYLD_FALLBACK_LIBRARY_PATH");
> > +
> > +    if ((curVal == NULL) || strncmp(libEnv, curVal, MAXPATHLEN)) 
> > +    {
> > +      //NOT SET! SET AND RELAUNCH
> > +      NSLog(@"DYLD_FALLBACK_LIBRARY_PATH NOT SET!!");
> > +      //they differ, so set it and relaunch
> > +      setenv("DYLD_FALLBACK_LIBRARY_PATH", libEnv, 1);
> > +      execNewBinary(myWebRTPath);
> > +      exit(0);
> > +    }
> > +    //HAVE DYLD_FALLBACK_LIBRARY_PATH
>
> This should no longer be necessary now that bug 738790 is being fixed properly. Please remove.

Removed!
https://github.com/michaelrhanson/mozilla-central/commit/eeb108adeb0e646f3d88c62ba076a894622c2d5a


> > +      //force myself to the foreground.  exec-ing a non .app bundle normally leaves you in the background
> > +      [NSApp activateIgnoringOtherApps:YES];
>
> I don't understand what this is doing here: we've already finished XRE_main and are in the shutdown process? Perhaps this was meant to go before XRE_main?

Due to the way we launch another process from within our Mac pseudo-app, the webapp ends up launching in the background.  This fixes it.


> > diff --git a/webapprt/win/webapprt.cpp b/webapprt/win/webapprt.cpp
>
> > +  #pragma pack(push, 2)
> > +  typedef struct
> > +  {
> > +    WORD Reserved;
> > +    WORD ResourceType;
> > +    WORD ImageCount;
> > +  } IconHeader;
> > +
> > +  typedef struct
> > +  {
> > +    BYTE Width;
> > +    BYTE Height;
> > +    BYTE Colors;
> > +    BYTE Reserved;
> > +    WORD Planes;
> > +    WORD BitsPerPixel;
> > +    DWORD ImageSize;
> > +    DWORD ImageOffset;
> > +  } IconDirEntry;
> > +
> > +  typedef struct
> > +  {
> > +    BYTE Width;
> > +    BYTE Height;
> > +    BYTE Colors;
> > +    BYTE Reserved;
> > +    WORD Planes;
> > +    WORD BitsPerPixel;
> > +    DWORD ImageSize;
> > +    WORD ResourceID;    // This field is the one difference to above
> > +  } IconResEntry;
> > +  #pragma pack(pop)
>
> Where do these structure declarations come from? Are there potential licensing issues that we need to be aware of?

These structure declarations were copied from redit.cpp in xulrunner/tools/redit.  Dave Townsend wrote that file and it is MPL, but we'll follow up with him to make sure that there are no licensing issues here.


> > +  // Copied from toolkit/xre/nsAppData.cpp.
> > +  void
> > +  SetAllocatedString(const char *&str, const char *newvalue) {
>
> nit, brace in column 0 of the next line, here and below

Fixed!
https://github.com/michaelrhanson/mozilla-central/commit/fb7b9823088e12abef9e5398fe04cf4d6bce7e29


> > +  /**
> > +   * A helper class which calls XPCOMGlueStartup/XPCOMGlueShutdown in its scope.
> > +   */
> > +  class ScopedXPCOMGlue
> > +  {
> > +    public:
> > +      ScopedXPCOMGlue()
> > +        : mRv(NS_ERROR_FAILURE) { }
> > +      ~ScopedXPCOMGlue() {
> > +        if(NS_SUCCEEDED(mRv)) {
> > +          XPCOMGlueShutdown();
> > +        }
> > +      }
> > +      nsresult startup(const char * xpcomFile) {
> > +        return (mRv = XPCOMGlueStartup(xpcomFile));
> > +      }
> > +    private:
> > +      nsresult mRv;
> > +  };
>
> I really don't think this is necessary: XPCOMGlueShutdown is not a useful function in general, and we shouldn't be calling it.

Excellent, that makes webapprt.cpp cleaner. Removed.
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +  /**
> > +   * A helper class for scope-guarding a file handle.
> > +   */
> > +  class ScopedFile
>
> why do we need this instead of just using ScopedClose from FileUtils.h?

timA: I was unaware of ScopedClose :).  I'll switch to that.
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +  nsresult
> > +  embedIcon(wchar_t const * const src,
> > +            wchar_t const * const dst) {
>
> nits: name this EmbedIcon. The only caller doesn't appear to care about the return code, so just make it return void, and fix the opening brace.

https://github.com/michaelrhanson/mozilla-central/commit/3f29f030ddf838a573967d58582c23f7e9cbec74
https://github.com/michaelrhanson/mozilla-central/commit/940f033dc0162c361615dfe4e4ea2c3cd64fdfd5
https://github.com/michaelrhanson/mozilla-central/commit/fb7b9823088e12abef9e5398fe04cf4d6bce7e29


> > +    ScopedResourceUpdateHandle updateRes;
> > +
> > +    { // Scope for group
> > +      nsAutoArrayPtr<BYTE> group;
> > +      long groupSize;
> > +
> > +      { // Scope for data
> > +        nsAutoArrayPtr<BYTE> data;
> > +
> > +        { // Scope for file
> > +          ScopedFile file;
>
> Do all of these need to go out of scope separately? If so, please document why it's necessary to close the file before the end of the function. Otherwise, I wouldn't bother with the extra scope braces, they make the function harder to read.

timA: They do not need to go out of scope separately.  I've removed the extra scope braces.
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +  nsresult
> > +  AttemptCopyAndLaunch(wchar_t* src,
>
> This is the beginning of a bunch of functions which use nsresult failure codes for no obvious reason: this doesn't appear to be XPCOM code, and nobody is distinguishing between various failure codes. Why don't we just make this set of functions return booleans?

Done!
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +                       int* result) {
> > +    // Rename the old app executable
> > +    if(FALSE == ::MoveFileExW(curExePath,
>
> nit, unsnuggle the paren here and several more times below

Done!
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +    if(!CreateProcessW(curExePath, // No module name (use command line)
> > +                       NULL,             // Command line
> > +                       NULL,             // Process handle not inheritable
> > +                       NULL,             // Thread handle not inheritable
> > +                       FALSE,            // Set handle inheritance to FALSE
> > +                       0,                // No creation flags
> > +                       NULL,             // Use parent's environment block
> > +                       NULL,             // Use parent's starting directory 
> > +                       &si,
> > +                       &pi)) {
> > +      return NS_ERROR_FAILURE;
> > +    }
> > +
> > +    // Wait until child process exits.
> > +    WaitForSingleObject(pi.hProcess, INFINITE);
>
> Why are we waiting for the app to exit? All of the other cases where we relaunch ourself (for updates or extension installation) we exit the first process immediately after launching the new one.

The idea was to have a meaningful exit code, but it seems that the exit code will not always be meaningful anyway.  Removed.
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +  nsresult
> > +  AttemptGRELoadAndLaunch(char* greDir,
> > +                          int* result) {
> > +    nsresult rv;
> ...
> > +    SetDllDirectoryW(wideGreDir);
>
> This is no longer necessary now that bug 740919 is fixed, please remove it.

Removed!
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +  { // Scope for first attempt at loading Firefox binaries
> > +
> > +    // Get the location of Firefox from our webapp.ini
> > +    // XXX: This string better be UTF-8...
> > +    rv = parser.GetString("WebappRT",
> > +                          "InstallDir",
> > +                          firefoxDir,
> > +                          MAXPATHLEN);
> > +    if(NS_SUCCEEDED(rv)) {
> > +      rv = AttemptLoadFromDir(firefoxDir, &result);
> > +      if(NS_SUCCEEDED(rv)) {
> > +        return result;
> > +      }
> > +    }
> > +  }
> > +
> > +  { // Scope for second attempt at loading Firefox binaries
> > +
> > +    rv = GetFirefoxDirFromRegistry(firefoxDir);
> > +    if(NS_SUCCEEDED(rv)) {
> > +      rv = AttemptLoadFromDir(firefoxDir, &result);
> > +      if(NS_SUCCEEDED(rv)) {
> > +        // XXX: Write gre dir location to webapp.ini
> > +        return result;
> > +      }
> > +    }
> > +  }
>
> What is the point of these extra scopes? They don't appear to hold any interesting locals, can they be removed?

I think they were at one point useful but can now be removed.  Removed!
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > --- a/xpfe/appshell/src/nsAppShellService.cpp
> > +++ b/xpfe/appshell/src/nsAppShellService.cpp
> > @@ -121,8 +121,8 @@ nsAppShellService::CreateHiddenWindow()
> >    PRUint32    chromeMask = 0;
> >    nsAdoptingCString prefVal =
> >        Preferences::GetCString("browser.hiddenWindowChromeURL");
> > -  const char* hiddenWindowURL = prefVal.get() ? prefVal.get() : DEFAULT_HIDDENWINDOW_URL;
> > -  mApplicationProvidedHiddenWindow = prefVal.get() ? true : false;
> > +  mApplicationProvidedHiddenWindow = prefVal.get() && strcmp(prefVal.get(), DEFAULT_HIDDENWINDOW_URL) ? true : false;
> > +  const char* hiddenWindowURL = mApplicationProvidedHiddenWindow ? prefVal.get() : DEFAULT_HIDDENWINDOW_URL;
>
> This deserves an explanatory comment.

Good point, I added a comment describing the enhancement and explaining its purpose.
https://github.com/michaelrhanson/mozilla-central/commit/cb83470ac381f98f83d20a381058563296de8db8


> r=me with nits fixed

We also fixed the last remaining string leak.
https://github.com/michaelrhanson/mozilla-central/commit/a48863a09f475cebe3b36515302bebba0e9071c2

And we disabled the unnecessary registration of browser and services components to the app-startup category, to prevent them from loading needlessly (or harmfully).
https://github.com/michaelrhanson/mozilla-central/commit/80d930af9e107f7bd7636fffd6df429be69e30a9

And made webapp windows resizeable.
https://github.com/michaelrhanson/mozilla-central/commit/06341ed918cae2f3e898a203ed462cdf4870d076

And removed the js-ctypes implementation of the icon embedding and shortcut creation services, which we're tackling separately (and natively) in bug 738500 and bug 738501.
https://github.com/michaelrhanson/mozilla-central/commit/47ee891e46a82901f958d38baefdb3cf113bd944


A tryserver run for this patch is underway at:

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


bsmedberg, dietrich: You two gave r+ previously. I think we've addressed the nits y'all raised. But requesting re-review to make sure.

Note: the NSIS uninstaller work is included in this patch but is being reviewed by Robert Strong separately in bug 735518.


> > +  nsresult
> > +  AttemptGRELoadAndLaunch(char* greDir,
> > +                          int* result) {
> > +    nsresult rv;
> ...
> > +    SetDllDirectoryW(wideGreDir);
>
> This is no longer necessary now that bug 740919 is fixed, please remove it.

Removed!
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > +  { // Scope for first attempt at loading Firefox binaries
> > +
> > +    // Get the location of Firefox from our webapp.ini
> > +    // XXX: This string better be UTF-8...
> > +    rv = parser.GetString("WebappRT",
> > +                          "InstallDir",
> > +                          firefoxDir,
> > +                          MAXPATHLEN);
> > +    if(NS_SUCCEEDED(rv)) {
> > +      rv = AttemptLoadFromDir(firefoxDir, &result);
> > +      if(NS_SUCCEEDED(rv)) {
> > +        return result;
> > +      }
> > +    }
> > +  }
> > +
> > +  { // Scope for second attempt at loading Firefox binaries
> > +
> > +    rv = GetFirefoxDirFromRegistry(firefoxDir);
> > +    if(NS_SUCCEEDED(rv)) {
> > +      rv = AttemptLoadFromDir(firefoxDir, &result);
> > +      if(NS_SUCCEEDED(rv)) {
> > +        // XXX: Write gre dir location to webapp.ini
> > +        return result;
> > +      }
> > +    }
> > +  }
>
> What is the point of these extra scopes? They don't appear to hold any interesting locals, can they be removed?

I think they were at one point useful but can now be removed.  Removed!
https://github.com/michaelrhanson/mozilla-central/commit/239256a37f28f0cbeadb1d5e56e5d7360796105d


> > --- a/xpfe/appshell/src/nsAppShellService.cpp
> > +++ b/xpfe/appshell/src/nsAppShellService.cpp
> > @@ -121,8 +121,8 @@ nsAppShellService::CreateHiddenWindow()
> >    PRUint32    chromeMask = 0;
> >    nsAdoptingCString prefVal =
> >        Preferences::GetCString("browser.hiddenWindowChromeURL");
> > -  const char* hiddenWindowURL = prefVal.get() ? prefVal.get() : DEFAULT_HIDDENWINDOW_URL;
> > -  mApplicationProvidedHiddenWindow = prefVal.get() ? true : false;
> > +  mApplicationProvidedHiddenWindow = prefVal.get() && strcmp(prefVal.get(), DEFAULT_HIDDENWINDOW_URL) ? true : false;
> > +  const char* hiddenWindowURL = mApplicationProvidedHiddenWindow ? prefVal.get() : DEFAULT_HIDDENWINDOW_URL;
>
> This deserves an explanatory comment.

Good point, I added a comment describing the enhancement and explaining its purpose.
https://github.com/michaelrhanson/mozilla-central/commit/cb83470ac381f98f83d20a381058563296de8db8


> r=me with nits fixed

We also fixed the last remaining string leak.
https://github.com/michaelrhanson/mozilla-central/commit/a48863a09f475cebe3b36515302bebba0e9071c2

And we disabled the unnecessary registration of browser and services components to the app-startup category, to prevent them from loading needlessly (or harmfully).
https://github.com/michaelrhanson/mozilla-central/commit/80d930af9e107f7bd7636fffd6df429be69e30a9

And made webapp windows resizeable.
https://github.com/michaelrhanson/mozilla-central/commit/06341ed918cae2f3e898a203ed462cdf4870d076

And removed the js-ctypes implementation of the icon embedding and shortcut creation services, which we're tackling separately (and natively) in bug 738500 and bug 738501.
https://github.com/michaelrhanson/mozilla-central/commit/47ee891e46a82901f958d38baefdb3cf113bd944


A tryserver run for this patch is underway at:

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


bsmedberg, dietrich: You two gave r+ previously. I think we've addressed the nits y'all raised. But requesting re-review to make sure.

Note: the NSIS uninstaller work is included in this patch but is being reviewed by Robert Strong separately in bug 735518.

Comment 38

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

Attachment: potential followup: isolate runtime into webapprt/ subdir

This is a potential followup that isolates runtime files in their own app subdirectory within the Firefox installation directory.  The change prevents Firefox preferences from loading for WebappRT, and it reduces the number of hacks we must employ to make Firefox and WebappRT share a GRE dir.

However, I did need to move Firefox preferences from defaults/prefs/ to defaults/preferences/ and make sure the latter's files are read even when an app shares the GRE dir.

This patch assumes patch #3 has already been applied.  Its size is deceiving, as a fair amount of it is file location changes or reversions of changes made in patch #3.  A tryserver run is underway:

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

Comment 39

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

Attachment: patch v4: enable only on Mac/Win; fix style nits

TryServer revealed that the runtime was being built on Linux, although it doesn't (yet) support that OS.  Here's a patch that fixes the problem.

https://github.com/michaelrhanson/mozilla-central/commit/85d4369ece61d9c8a7a4067e644d3e3aaf490c90


I also noticed some style nits in webapprt.mm, and that it called XPCOMGlueShutdown, which bsmedberg said webapprt.cpp shouldn't do, so I fixed those issues.

https://github.com/michaelrhanson/mozilla-central/commit/039293c77c1263732ea0ca1d4a9ed099e1bd7036
https://github.com/michaelrhanson/mozilla-central/commit/0e9abc9797d581e78ef7fc3e903aad0590bd8bad
https://github.com/michaelrhanson/mozilla-central/commit/f85d7eb5e41005a10bbdf1049beac2e8af357605


New TryServer push:

https://tbpl.mozilla.org/?tree=Try&rev=982c33078b80

Comment 40

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

Attachment: followup v2: updated for patch v4

This updated runtime isolation followup is against the most recent patch v4.

TryServer run:

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

Comment 41

[Dietrich Ayala (:dietrich)]

Comment on attachment 614592 [details] [diff] [review]
patch v4: enable only on Mac/Win; fix style nits

Review of attachment 614592 [details] [diff] [review]:
-----------------------------------------------------------------

latest changes to the browser bits look ok, r=me.

Comment 42

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

After restarting a couple of test runs that ran into apparently-intermittent failures or infrastructure issues, all runs completed successfully for both patches:

https://tbpl.mozilla.org/?tree=Try&rev=982c33078b80
https://tbpl.mozilla.org/?tree=Try&rev=fbad3967d3a0

Comment 43

[Jason Smith [:jsmith]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #42)
> After restarting a couple of test runs that ran into apparently-intermittent
> failures or infrastructure issues, all runs completed successfully for both
> patches:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=982c33078b80
> https://tbpl.mozilla.org/?tree=Try&rev=fbad3967d3a0

Is there an associated Firefox build created for this that I can test? If so, what is available to test (looking to help out)?

Comment 44

[Anant Narayanan [:anant]]

(In reply to Jason Smith from comment #43)
> > https://tbpl.mozilla.org/?tree=Try&rev=982c33078b80
> > https://tbpl.mozilla.org/?tree=Try&rev=fbad3967d3a0
> 
> Is there an associated Firefox build created for this that I can test? If
> so, what is available to test (looking to help out)?

Yes, you can download the builds for various platforms at:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-982c33078b80/
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fbad3967d3a0/

For future reference, anything that's pushed to try and succeeds will have builds that be found in that directory, just look for the changeset.

Comment 45

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

(In reply to Anant Narayanan [:anant] from comment #44)
> Yes, you can download the builds for various platforms at:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 982c33078b80/
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> fbad3967d3a0/

However, only the launcher/shell are available to test in these builds.  Later today I'll build an integration branch with the full stack (mozApps API, installers for Windows and Mac, launchers, and shell) that you can use to test the complete set of functionality we plan to ship in Firefox 14.

Comment 46

[Jason Smith [:jsmith]]

With the try build, could you clarify how I can test this right now (looking to figure out what functionality subset I can start testing)? I have tested that I can successfully install the try build and use it.

Comment 47

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

You can test it by manually installing a webapp using the flow documented at:

https://etherpad.mozilla.org/webapprt-install-flow

Comment 48

[Jason Smith [:jsmith]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #47)
> You can test it by manually installing a webapp using the flow documented at:
> 
> https://etherpad.mozilla.org/webapprt-install-flow

Hmm okay. Just tried installing apps from apps.mozillalabs.com/appdir on Win 7 64-bit - I did not see the application get installed. No directory was created in %APPDATA%, no way to launch it (start menu or shortcut, but I think the shortcut may still be getting implemented?). Did I do something incorrect? Example steps I did was:

1. Go to apps.mozillalabs.com/appdir
2. Select to install BarFight
3. When the door-hanger appears, select install

Result - Nothing happened on my native machine.

Comment 49

[Jason Smith [:jsmith]]

For comment 48 - Same behavior is also seen on Win 7 32-bit.

Comment 50

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

(In reply to Jason Smith from comment #48)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #47)
> > You can test it by manually installing a webapp using the flow documented at:
> > 
> > https://etherpad.mozilla.org/webapprt-install-flow
> 
> Hmm okay. Just tried installing apps from apps.mozillalabs.com/appdir on Win
> 7 64-bit - I did not see the application get installed. No directory was
> created in %APPDATA%, no way to launch it (start menu or shortcut, but I
> think the shortcut may still be getting implemented?). Did I do something
> incorrect?

The tryserver builds for the patches in this bug do not include the installers, so to test them you need to manually install a webapp using the flow documented at:

https://etherpad.mozilla.org/webapprt-install-flow

I'm working on creating builds that do include the installers, which will be more suitable for testing.  I'll let you know when those are available via a post to the discussion forum.

Comment 51

[Benjamin Smedberg]

> So while the preference might not be needed, and it would certainly simplify the
> implementation to hardcode the value, it seems safer to provide it.

Indeed, I think you should provide it (for now), but not *use* it in this codepath.

> > > +      //force myself to the foreground.  exec-ing a non .app bundle normally leaves you in the background
> > > +      [NSApp activateIgnoringOtherApps:YES];
> >
> > I don't understand what this is doing here: we've already finished XRE_main and are in the shutdown process? Perhaps this was meant to go before XRE_main?

> Due to the way we launch another process from within our Mac pseudo-app, the webapp ends up launching in the background.  This fixes it.

I still don't think this is useful/correct. This line runs after XPCOM is shut down when there is no more process launching to do (AFAICT). And in fact we won't ever hit this line at all in the common (release build) case, once bug 662444 is done.

I'm going to mark r+ based on reading the linked github commits instead of the new patch. I hope that nothing else significant has changed in the meantime.

Comment 52

[Dan Walkowski]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #51)
 
> > > > +      //force myself to the foreground.  exec-ing a non .app bundle normally leaves you in the background
> > > > +      [NSApp activateIgnoringOtherApps:YES];
> > >
> > > I don't understand what this is doing here: we've already finished XRE_main and are in the shutdown process? Perhaps this was meant to go before XRE_main?
> 
> > Due to the way we launch another process from within our Mac pseudo-app, the webapp ends up launching in the background.  This fixes it.
> 
> I still don't think this is useful/correct. This line runs after XPCOM is
> shut down when there is no more process launching to do (AFAICT). And in
> fact we won't ever hit this line at all in the common (release build) case,
> once bug 662444 is done.
> 
> I'm going to mark r+ based on reading the linked github commits instead of
> the new patch. I hope that nothing else significant has changed in the
> meantime.

The launching into the background was apparently caused by the relaunch with the DYLD environment variable set.  I have just tested removal of the activateIgnoringOtherApps call, and it is no longer necessary.  

There is still a flip and relaunch when updating to the newest webapprt, but the app ends up in the foreground.

Comment 53

[Benjamin Smedberg]

Comment on attachment 614594 [details] [diff] [review]
followup v2: updated for patch v4

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in

> #ifdef MOZ_WEBAPP_RUNTIME
>-; [Webapp Runtime]
>-@BINPATH@/webapprt@BIN_SUFFIX@
>-@BINPATH@/chrome/webapprt@JAREXT@
>-@BINPATH@/chrome/webapprt.manifest
>-@BINPATH@/components/WebappRTComponents.manifest
>-@BINPATH@/components/WebappRTDirectoryProvider.js
>-@BINPATH@/components/WebappRTCommandLineHandler.js
>-@BINPATH@/webapprt.ini
>+[WebappRuntime]
>+@BINPATH@/webapprt/*
> #endif

I'd prefer that we list each file. It makes it easier to be sure that when we're adding or removing files that they are listed in the proper package-manifest or removed-files location. With the wildcard you won't get a packaging warning if a file is removed or renamed.

>diff --git a/webapprt/CommandLineHandler.js b/webapprt/CommandLineHandler.js
>diff --git a/webapprt/DirectoryProvider.js b/webapprt/DirectoryProvider.js

I have made the assumption that this has not actually changed. I wonder if in your workflow there is a way to provide the equivalent patch to an `hg move` so that I can just view the diff? If there are actually substantive changes let me know.

>diff --git a/webapprt/locales/en-US/webapprt/webapp.dtd b/webapprt/locales/en-US/webapprt/webapp.dtd

Moving the locale files to a new source location requires significant additional work related to l10n repackaging. I think that we should hold off this particular part of the patch for another time when we're not under time pressure. It shouldn't cause other problems.

r=me on the non-l10n bits. Per IRC discussion I believe you were going to name it webapprt-stub and put it in the root directory, which might require some undoing of the FINAL_TARGET changes in the two stub directories, but I don't think that's especially complicated or needs additional review.

Comment 54

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

Attachment: patch v5: address review comments

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #51)
> > So while the preference might not be needed, and it would certainly simplify the
> > implementation to hardcode the value, it seems safer to provide it.
> 
> Indeed, I think you should provide it (for now), but not *use* it in this
> codepath.

Ok, done.
https://github.com/michaelrhanson/mozilla-central/commit/4bff09b19a3f80d92423dcd03f2ac56bd943709d


> > > > +      //force myself to the foreground.  exec-ing a non .app bundle normally leaves you in the background
> > > > +      [NSApp activateIgnoringOtherApps:YES];
> > >
> > > I don't understand what this is doing here: we've already finished XRE_main and are in the shutdown process? Perhaps this was meant to go before XRE_main?
> 
> > Due to the way we launch another process from within our Mac pseudo-app, the webapp ends up launching in the background.  This fixes it.
> 
> I still don't think this is useful/correct. This line runs after XPCOM is
> shut down when there is no more process launching to do (AFAICT). And in
> fact we won't ever hit this line at all in the common (release build) case,
> once bug 662444 is done.

Indeed, removed.
https://github.com/michaelrhanson/mozilla-central/commit/f5484e319935eeed463728c471a7e560b4f05f10


> I'm going to mark r+ based on reading the linked github commits instead of
> the new patch. I hope that nothing else significant has changed in the
> meantime.

We've been focusing on the work required to land this initial implementation, so we should not have made any other significant changes.  To confirm, I just walked the commit graph for the last three weeks, and I didn't see any, except for the NSIS uninstaller work, for which Tim got review from Robert Strong over in bug 735518.


Per our conversations on IRC, I have also renamed the stub executable from webapprt[.exe] to webapprt-stub[.exe] to more accurately represent its function and pave the way for creating a webapprt/ subdirectory into which we place the shell files.
https://github.com/michaelrhanson/mozilla-central/commit/e96780d76555ec604fcd1e7e12c65395060eab1d


And Tim removed the icon embedding code from the Windows implementation, as it was triggering build failures (from some upstream changes in the last several days), and since we intend to replace it with another implementation based on redit.exe in short order (which Tim says is ready for review, but in the interests of landing the code that is already reviewed, we'll do that in a followup bug).
https://github.com/michaelrhanson/mozilla-central/commit/d53bc0ef7d95730cb5703aa57077b99c760bc430


Carrying forward review, and I have pushed this patch, plus the Mac installer patch, to tryserver:

https://tbpl.mozilla.org/?tree=Try&rev=8c624b552edc

If the results are positive, I'll land those two patches on inbound.  Hopefully the Windows installer patch (and the nsILocalFileWin.setShortcut patch on which it depends) are right behind it.  And the subdirectory isolation patch, which I'm still working on, and which I'll probably move to a new bug once this initial launcher/shell patch lands.

Comment 55

[Jason Smith [:jsmith]]

I think this is a known issue with the current try build, but just to double check - Tested the Win 7 32-bit try build. Every time I tried to install an application, I would get a DOMError thrown (happened on each application on the app directory). As a result, I could not install any application.

Comment 56

[Marco Castelluccio [:marco]]

(In reply to Jason Smith from comment #55)
> I think this is a known issue with the current try build, but just to double
> check - Tested the Win 7 32-bit try build. Every time I tried to install an
> application, I would get a DOMError thrown (happened on each application on
> the app directory). As a result, I could not install any application.

I've run into the same issue while working on Linux installation. The problem is that in dom/base/Webapps.js the manifest checking fails.
I don't know why it fails because I haven't investigated yet (for my purpose, commenting the code was enough).

Comment 57

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

Comment on attachment 615580 [details] [diff] [review]
patch v5: address review comments

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

Comment 58

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

Note: I noticed an issue with the Mac installer when testing the tryserver builds, so I resubmitted the patches to tryserver with a fix for the issue.

This doesn't affect the previous tryserver results for this bug, which remain valid; it just affects folks testing the builds the tryserver produced.  Testers should use the builds from this tryserver run instead:

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

Comment 59

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

Comment on attachment 614594 [details] [diff] [review]
followup v2: updated for patch v4

I filed bug 746156 on this followup and will complete the work there.

Comment 60

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #27)
> > > +    else {
> > > +      if (NS_SUCCEEDED(curDir->Exists(&exists)) && exists)
> > > +    aDirectories.AppendObject(curDir);
> > >      }
> > 
> > This is a behavior change. If it's intentional, can you please explain it?
> > Admittedly this function was underdocumented to begin with, and JARred
> > extension support was hacked into the middle, but this feels kinda strange
> > and the correct behavior needs to be documented.
> 
> I did this during a merge from upstream, and now I can't remember why, so I
> removed it, which doesn't seem to have caused any problems.

Erm, I intended to revert the change, but I ended up removing the two lines of code I had enclosed in the else block, and that busted loading default prefs from JARed extensions (bug 746457), which seemed like significant enough bustage (although not covered by tests) to warrant committing an urgent bustage fix before this morning's nightlies get built:

https://tbpl.mozilla.org/?rev=0c7e2911be75

Comment 61

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ef55c163a23a

Comment 62

[Jason Smith [:jsmith]]

Verified that this feature has landed. Followup bugs will be filed based on issues hit post landing of this feature.

Comment 63

[[e-mail]]

Comment on attachment 615580 [details] [diff] [review]
patch v5: address review comments

>+++ b/webapprt/win/webapp-uninstaller.nsi.in
>@@ -0,0 +1,163 @@
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+# You can obtain one at http://mozilla.org/MPL/2.0/.
Bah, preprocessed just to remove the comments...

Comment 64

[twilightinzero]

How exactly is this "fixed"? The original bug report was about a command line switch, and here we are with Firefox 16, claiming that this feature has landed, and instead, we have manifests that must be hosted and served by the site in question, and no way for a user to generate a webapp from a pre-existing website. Other browsers have been able to do this for a very long time. Why did we wait so long for this feature only for it to come out so convoluted?

Comment 65

[Dave Townsend [:mossop]]

(In reply to twilightinzero from comment #64)
> How exactly is this "fixed"? The original bug report was about a command
> line switch, and here we are with Firefox 16, claiming that this feature has
> landed, and instead, we have manifests that must be hosted and served by the
> site in question, and no way for a user to generate a webapp from a
> pre-existing website. Other browsers have been able to do this for a very
> long time. Why did we wait so long for this feature only for it to come out
> so convoluted?

The specifications for bugs change over time, just because we don't implement precisely what is specified in comment 0 doesn't mean the code for this feature hasn't landed and so this bug is considered FIXED. Follow-up issues or, what seems more likely from your comment, requests for different features should be filed as new bugs.