762344 - Move metrofx BrowserApp functionality into firefox's nsBrowserApp

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Jim Mathies [:jimm]]

We currently have two exes which we register separately but we really don't need to. What we can do is detect which environment we are launched into in fx's nsBrowserApp and load the appropriate app environment (browser or metro). We can then remove metrofirefox.exe entirely and register the same browser for both environments in the registry. This should give us the ability to set Firefox as the default metro browser through the desktop browser.

Comment 1

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

I'll take a look at this

Comment 2

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

Attachment: Patch v1 - Merge browser/metro/app/BrowserApp.cpp into browser/app/nsBrowserApp.cpp. Remove files/logic for building metrofirefox.exe

Only one executable will be built as a result of this patch: firefox.exe.  metrofirefox.exe is no longer built.

If you modify the chrome.manifest file as specified in [1], then the EXE can be used like metrofirefox.exe:
  It can be launched in metro mode from the "Metro Firefox" tile
  It can be passed "-desktop" to launch the metro codepath but in desktop mode

If you do not modify the chrome.manifest file, the EXE can be used like firefox.exe:
  Launching it with no args launches desktop Firefox
  I haven't done any testing beyond this

[1] https://wiki.mozilla.org/Firefox/Windows_8_Integration#Metro_Builds


There was some logic in the metro BrowserApp.cpp that I've copied over that is "#ifdef MAEMO" - Jimm: is this cruft?

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 633630 [details] [diff] [review]
Patch v1 - Merge browser/metro/app/BrowserApp.cpp into browser/app/nsBrowserApp.cpp.  Remove files/logic for building metrofirefox.exe

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

Before landing this on elm, would you mind updating the registration scripts as well?

http://mxr.mozilla.org/projects-central/source/elm/browser/metro/shell/installscripts/

::: browser/app/nsBrowserApp.cpp
@@ +35,5 @@
>  
>  #include "nsXPCOMPrivate.h" // for MAXPATHLEN and XPCOM_DLL
>  
>  #include "mozilla/Telemetry.h"
> +#if MOZ_PLATFORM_MAEMO == 6

Remove any code targeting this platform.

Comment 4

[Jim Mathies [:jimm]]

Please don't close this out after landing on elm, once bug 755724 is fixed, we can recheck this work to be sure it's compatible with the work there, then land these changes on mc.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 633630 [details] [diff] [review]
Patch v1 - Merge browser/metro/app/BrowserApp.cpp into browser/app/nsBrowserApp.cpp.  Remove files/logic for building metrofirefox.exe

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

::: browser/app/nsBrowserApp.cpp
@@ +171,5 @@
>  
> +  bool metro = false;
> +
> +  if (argc > 1) {
> +    if (IsArg(argv[1], "ServerName:DefaultBrowserServer")) {

Also Tim, could you add a comment here explaining where this comes from.

Comment 6

[Jim Mathies [:jimm]]

One other addition, since this is shared source, the metro specific changes need to be #ifdef MOZ_METRO .. #endif.

Comment 7

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

I'm having some local debugging issues that I'm working through, but I'm posting the current patch here since the changes requested have been made.


(In reply to Jim Mathies [:jimm] from comment #3)
> Comment on attachment 633630 [details] [diff] [review]
> Patch v1 - Merge browser/metro/app/BrowserApp.cpp into
> browser/app/nsBrowserApp.cpp.  Remove files/logic for building
> metrofirefox.exe
> 
> Review of attachment 633630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Before landing this on elm, would you mind updating the registration scripts
> as well?
> 
> http://mxr.mozilla.org/projects-central/source/elm/browser/metro/shell/
> installscripts/

The patch included a change to the makefile in that directory that effectively changed the targets of the scripts from "metrofirefox.exe" to "firefox.exe" - the scripts worked in my testing with this change, is there anything else that needs to be updated in that dir?

> 
> ::: browser/app/nsBrowserApp.cpp
> @@ +35,5 @@
> >  
> >  #include "nsXPCOMPrivate.h" // for MAXPATHLEN and XPCOM_DLL
> >  
> >  #include "mozilla/Telemetry.h"
> > +#if MOZ_PLATFORM_MAEMO == 6
> 
> Remove any code targeting this platform.

Removed.

(In reply to Jim Mathies [:jimm] from comment #4)
> Please don't close this out after landing on elm, once bug 755724 is fixed,
> we can recheck this work to be sure it's compatible with the work there,
> then land these changes on mc.

Will do... or won't do ;)

(In reply to Jim Mathies [:jimm] from comment #5)
> Comment on attachment 633630 [details] [diff] [review]
> Patch v1 - Merge browser/metro/app/BrowserApp.cpp into
> browser/app/nsBrowserApp.cpp.  Remove files/logic for building
> metrofirefox.exe
> 
> Review of attachment 633630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/nsBrowserApp.cpp
> @@ +171,5 @@
> >  
> > +  bool metro = false;
> > +
> > +  if (argc > 1) {
> > +    if (IsArg(argv[1], "ServerName:DefaultBrowserServer")) {
> 
> Also Tim, could you add a comment here explaining where this comes from.

Added this comment.

(In reply to Jim Mathies [:jimm] from comment #6)
> One other addition, since this is shared source, the metro specific changes
> need to be #ifdef MOZ_METRO .. #endif.

Added this ifdef.


Once I've worked through the issues on my local build I'll re-request review.  I've also split the changes to `Output()` into a separate patch since those are a separate issue (bug 762310).

Comment 8

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

Attachment: Patch v2 - Split out `Output` changes, add `#ifdef MOZ_METRO`, remove `MOZ_MAEMO` code.

Actually attaching the patch that goes with the previous comment.  Carrying forward r+.  Testing ongoing.

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 634758 [details] [diff] [review]
Patch v2 - Split out `Output` changes, add `#ifdef MOZ_METRO`, remove `MOZ_MAEMO` code.

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

::: browser/app/nsBrowserApp.cpp
@@ +225,5 @@
> +  xreDirectory.forget(&appData->xreDirectory);
> +
> +  // The app dir should be in dist/bin/metro where app resources are based
> +  rv = XRE_GetFileFromPath(workingDir, getter_AddRefs(appDirectory));
> +  if (NS_FAILED(XRE_GetFileFromPath(workingDir, getter_AddRefs(appDirectory)))) {

Nit - double call to XRE_GetFileFromPath for appDirectory. Probably brought over from the original BrowserApp code. You can take the first call out.

Comment 10

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

Attachment: Patch v3 - Update CommandExecuteHandler. Remove double-call to XRE_GetFileFromPath.

Thanks for working with me on this, Jim.  This patch works in my local testing without removing the extra lines from 'chrome.manifest'.

One thing I've noticed is that sometimes when launching in metro mode I get something of a blank shell; all the UI appears correctly but tapping on most buttons or the url bar does nothing.  Also no content is displayed in the URL bar or content window.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 637265 [details] [diff] [review]
Patch v3 - Update CommandExecuteHandler. Remove double-call to XRE_GetFileFromPath.

Feel free to land this on elm. note, fixed elm bugs should be left open but tagged with 'completed-elm' in the whiteboard.

Comment 12

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

http://hg.mozilla.org/projects/elm/rev/822a0f36212f

Comment 13

[Jim Mathies [:jimm]]

Sorry this doesn't need to track it2, it's an elm only change.

Comment 14

[Jim Mathies [:jimm]]

Morphing into a more general bug - land elm specific nsBrowserApp changes on mc.

Comment 15

[Mike Hommey [:glandium]]

Note that one line of those elm specific nsBrowserApp changes is from bug 755724.

Comment 16

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.