Closed Bug 762344 Opened 7 years ago Closed 7 years ago

Move metrofx BrowserApp functionality into firefox's nsBrowserApp

Categories

(Firefox for Metro Graveyard :: General, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jimm, Unassigned)

References

Details

(Whiteboard: [completed-elm])

Attachments

(3 obsolete files)

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.
I'll take a look at this
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
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?
Attachment #633630 - Flags: review?(jmathies)
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.
Attachment #633630 - Flags: review?(jmathies) → review+
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.
Depends on: metro-build
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.
One other addition, since this is shared source, the metro specific changes need to be #ifdef MOZ_METRO .. #endif.
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).
Actually attaching the patch that goes with the previous comment.  Carrying forward r+.  Testing ongoing.
Attachment #633630 - Attachment is obsolete: true
Attachment #634758 - Flags: review+
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.
Blocks: elm-merge
No longer blocks: 737833
No longer depends on: metro-build
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.
Attachment #634758 - Attachment is obsolete: true
Attachment #637265 - Flags: review?(jmathies)
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.
Attachment #637265 - Flags: review?(jmathies) → review+
Blocks: 768464
Depends on: 769528
Component: Shell Integration → General
Product: Firefox → Firefox for Metro
Whiteboard: completed-elm → [completed-elm][metro-it2]
Sorry this doesn't need to track it2, it's an elm only change.
Whiteboard: [completed-elm][metro-it2] → [completed-elm]
Morphing into a more general bug - land elm specific nsBrowserApp changes on mc.
Assignee: tabraldes → nobody
Attachment #637265 - Attachment is obsolete: true
Status: ASSIGNED → NEW
Note that one line of those elm specific nsBrowserApp changes is from bug 755724.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.