Last Comment Bug 755724 - (metro-build) Split platform and app resources up so that they can be loaded individually
(metro-build)
: Split platform and app resources up so that they can be loaded individually
Status: RESOLVED FIXED
[metro-preview][completed-elm][metro-...
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 6 votes (vote)
: mozilla21
Assigned To: Mike Hommey [:glandium]
:
: Gregory Szorc [:gps]
Mentors:
https://etherpad.mozilla.org/metro-build
: 817602 (view as bug list)
Depends on: 844128 872047 762864 775154 776611 new-packager 782890 783242 783243 787180 787184 787443 788463 789335 789344 789360 789461 790115 792050 792685 793767 798437 802254 802541 809001 817602 817879 817881 820053 820200 820201 820205 820232 820724 821038 821182 821327 822004 822097 826565 834018 834101 834104 834873 839793 840094 840555 840734 841011 841094 842334 844553 870529 933803
Blocks: elm-merge 779902 810714 840146 840598
  Show dependency treegraph
 
Reported: 2012-05-16 06:44 PDT by Jim Mathies [:jimm]
Modified: 2014-10-20 15:38 PDT (History)
35 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
24+


Attachments
example (1.68 KB, patch)
2012-05-16 06:44 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch (15.06 KB, patch)
2012-05-16 11:39 PDT, Jim Mathies [:jimm]
benjamin: review-
Details | Diff | Splinter Review
patch v.2 (20.69 KB, patch)
2012-05-17 04:16 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
chrome.manifest (3.06 KB, text/plain)
2012-05-17 04:17 PDT, Jim Mathies [:jimm]
no flags Details
browser.manifest (1.55 KB, text/plain)
2012-05-17 04:17 PDT, Jim Mathies [:jimm]
no flags Details
components.manifest (86 bytes, text/plain)
2012-05-17 04:18 PDT, Jim Mathies [:jimm]
no flags Details
mc compat patch (20.64 KB, patch)
2012-05-17 04:27 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
fennec xul migration to browser (diff off elm tip) (543.41 KB, patch)
2012-05-24 13:38 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
base file copies (141.75 KB, patch)
2012-05-24 14:28 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
base patch (94.86 KB, patch)
2012-05-24 14:35 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
Part A, rev. 1 - Add DIST_SUBDIR makefile variable [checked in] (2.76 KB, patch)
2012-06-04 08:10 PDT, Benjamin Smedberg [:bsmedberg]
ted: review+
mh+mozilla: checkin+
Details | Diff | Splinter Review
Part B, rev. 1 - use DIST_SUBDIR in makefiles and launch using the subdir (59.01 KB, patch)
2012-06-04 08:13 PDT, Benjamin Smedberg [:bsmedberg]
mh+mozilla: review-
Details | Diff | Splinter Review
Part C, rev. 1 - Move ScopedAppData into the XPCOM glue [checked in] (17.26 KB, patch)
2012-06-04 12:00 PDT, Benjamin Smedberg [:bsmedberg]
mh+mozilla: review+
mh+mozilla: checkin+
Details | Diff | Splinter Review
Part B, rev. 2 - use DIST_SUBDIR in makefiles and launch using the subdir (59.37 KB, patch)
2012-06-22 08:59 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Splinter Review
Part B, rev. 2.1 - use DIST_SUBDIR in makefiles and launch using the subdir (39.47 KB, patch)
2012-07-14 18:36 PDT, Justin Wood (:Callek)
no flags Details | Diff | Splinter Review
Part B, rev. 2.2 - use DIST_SUBDIR in makefiles and launch using the subdir (39.38 KB, patch)
2012-07-15 02:21 PDT, Justin Wood (:Callek)
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (PoC) (8.33 KB, patch)
2012-08-30 12:54 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (PoC) (8.63 KB, patch)
2012-08-30 14:06 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (PoC) (9.10 KB, patch)
2012-08-30 14:32 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (PoC) (9.15 KB, patch)
2012-08-30 15:21 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (PoC) (9.17 KB, patch)
2012-09-05 11:08 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Use DIST_SUBDIR for XPIs too (2.35 KB, patch)
2012-12-11 00:48 PST, Mike Hommey [:glandium]
ted: review+
mh+mozilla: checkin+
Details | Diff | Splinter Review
Don't use the xulrunner stub when building Firefox against a libxul SDK (11.98 KB, patch)
2012-12-27 05:07 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (26.77 KB, patch)
2012-12-27 05:09 PST, Mike Hommey [:glandium]
jmathies: review+
Details | Diff | Splinter Review
Don't use the xulrunner stub when building Firefox against a libxul SDK (11.98 KB, patch)
2012-12-27 05:25 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Don't use the xulrunner stub when building Firefox against a libxul SDK (12.08 KB, patch)
2012-12-27 07:58 PST, Mike Hommey [:glandium]
benjamin: review+
mh+mozilla: checkin+
Details | Diff | Splinter Review
Initialize trace malloc before calling NS_NewLocalFile in nsBrowserApp.cpp (2.72 KB, patch)
2013-01-18 03:00 PST, Mike Hommey [:glandium]
benjamin: review+
mh+mozilla: checkin+
Details | Diff | Splinter Review
Move browser application in a subdirectory (26.75 KB, patch)
2013-01-25 10:48 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (26.79 KB, patch)
2013-01-25 13:13 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (26.45 KB, patch)
2013-02-06 03:11 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (26.47 KB, patch)
2013-02-06 03:32 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Move browser application in a subdirectory (26.14 KB, patch)
2013-02-11 04:55 PST, Mike Hommey [:glandium]
jmathies: review+
Details | Diff | Splinter Review

Description Jim Mathies [:jimm] 2012-05-16 06:44:19 PDT
Created attachment 624368 [details] [diff] [review]
example

This is for work in getting the metro browser integrated with the firefox build/install. Both browsers will need use of the root chrome.manifest file to pick up various platform components. But the two will use separate browser components. 

This change allows us to add app-id exclusions to the root chrome.manifest, so for example we might have:

manifest components/BrowserComponents.manifest application={fxid}
manifest metro/components/BrowserComponents.manifest application={metroid}

attached is a patch that adds this to rules.mk and adds one exclusion for BrowserComponents.manifest as an example.
Comment 1 Jim Mathies [:jimm] 2012-05-16 11:39:22 PDT
Created attachment 624468 [details] [diff] [review]
patch

A more complete patch with some additional changes to JarMaker and target_libs.mk.
Comment 2 Benjamin Smedberg [:bsmedberg] 2012-05-16 12:34:23 PDT
Comment on attachment 624468 [details] [diff] [review]
patch

Why is the restriction in browser/* windows-only? Is Metro Firefox really going to have a separate app ID? Isn't that going to cause problems dealing with profiles and extensions? And will this cause problems for webrt?

In terms of implementation, I don't think we should do this for jar.mn chrome at all. Note that any location that has a jar.mn with "browser.jar" would have to have the exact same app id, or else you'd end up with multiple entries in the root chrome.manifest:

manifest chrome/browser.manifest
manifest chrome/browser.manifest application={appID}

which would do you no good at all. Instead, I think that each entry in the jar.mn probably ought to have the application marker if that's what we really want, e.g. at http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/jar.mn#2

make that

% skin browser classic/1.0 %skin/classic/browser/ os=WINNT osversion<6 application={appid]

You have a similar problem for components, because with the current patch you'd end up with *both*

manifest components/components.manifest
manifest components/components/manifest application={appID}

in the root chrome.manifest. You should almost certainly be modifying the "binary-component" registration line instead. And instead of adding an ifdef and repeating the rules, just use a variable. e.g. in browser/components/build/Makefile.in just write

COMPONENT_REGISTRATION_MODIFIERS += application={appID}

and then in the rule:

+	@$(PYTHON) $(MOZILLA_DIR)/config/buildlist.py $(FINAL_TARGET)/components/components.manifest "binary-component $(SHARED_LIBRARY) $(COMPONENT_REGISTRATION_MODIFIERS)"
Comment 3 Jim Mathies [:jimm] 2012-05-16 14:22:41 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Comment on attachment 624468 [details] [diff] [review]
> patch
> 
> Why is the restriction in browser/* windows-only?

I didn't see any reason to add the extra chrome.manifest strings to other platform builds.

> Is Metro Firefox really going to have a separate app ID?

AFAICT there are two options here. One we can set to xre root folder for the metro browser to its subfolder (dist/bin/metro) and place everything there it would need to run. This would isolate the two browsers and there would be no need to share a root chrome manifest file or a need for two app ids. The issue I see with this is that the size of the duplicated code is large. (components for example one meg compressed and most of that is from platform.) To avoid this the other option is to share a root chrome manifest and the generic resources both browser use and isolate app specifics like chrome, modules, and app components in two locations. In this case we need a separate app id so we can split things up in the root chrome.manifest. 

> Isn't that going to cause problems dealing
> with profiles and extensions?

The shared root does have a down side, there's at least one issue with default prefs I have to solve. (Currently when the root is shared defaults/* gets loaded by both.) Default extensions probably have a similar problem.

The alternate would be to duplicate a lot of bcode, or maybe hack chrome manifest processing so that apps can access code outside their root folder. Both of these options seemed like bad options to me. 

As far as profiles go, initially we just want to use two profiles to get things going which I'll be trying to configure. But ultimately both browsers would share a single profile. Not sure how either scenario ties into the app id, is the profile affected by that in some way?

> And will this cause problems for webrt?

webrt should be fine. It is set up similarly to the way I'm setting up the metro browser. The only difference is it sets the root xre folder to the dist/bin/webaprt subfolder when starting up, so there's no shared chrome.manifest. 
 

> In terms of implementation, I don't think we should do this for jar.mn
> chrome at all. Note that any location that has a jar.mn with "browser.jar"
> would have to have the exact same app id, or else you'd end up with multiple
> entries in the root chrome.manifest:
> 
> manifest chrome/browser.manifest
> manifest chrome/browser.manifest application={appID}

Yes, this is why I had to edit all the makefiles around jars. Didn't like that much but it accomplished the desired outcome.

> 
> which would do you no good at all. Instead, I think that each entry in the
> jar.mn probably ought to have the application marker if that's what we
> really want, e.g. at
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/jar.
> mn#2
> 
> make that
> 
> % skin browser classic/1.0 %skin/classic/browser/ os=WINNT osversion<6
> application={appid]


Hmm, so I thought about doing it this way. You still run into the same problem though, if someone forgets the id, you get something loaded into the wrong browser. *shrug*. I'm fine with either solution, the madefile fix was simpler. I can scrap that and put something like this together if you'd like.

> You have a similar problem for components, because with the current patch
> you'd end up with *both*
> 
> manifest components/components.manifest
> manifest components/components/manifest application={appID}
> 
> in the root chrome.manifest. You should almost certainly be modifying the
> "binary-component" registration line instead. And instead of adding an ifdef

I wanted the id restriction in the root chrome.manifest file vs. components/components.manifest. For example:

components/component.manifest:
binary-component browsercomps.dll

isn't an issue when:

chrome.manifest:
manifest components/components.manifest application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}

components.manifest never gets loaded.

> and repeating the rules, just use a variable. e.g. in
> browser/components/build/Makefile.in just write
> 
> COMPONENT_REGISTRATION_MODIFIERS += application={appID}
> 
> and then in the rule:
> 
> +	@$(PYTHON) $(MOZILLA_DIR)/config/buildlist.py
> $(FINAL_TARGET)/components/components.manifest "binary-component
> $(SHARED_LIBRARY) $(COMPONENT_REGISTRATION_MODIFIERS)"

will do. Thanks for the feedback.
Comment 4 Jim Mathies [:jimm] 2012-05-16 16:37:53 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> which would do you no good at all. Instead, I think that each entry in the
> jar.mn probably ought to have the application marker if that's what we
> really want, e.g. at
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/jar.
> mn#2
> 
> make that
> 
> % skin browser classic/1.0 %skin/classic/browser/ os=WINNT osversion<6
> application={appid]
> 

So in looking at implementing this my one concern is that if we don't add ids to the chrome manifest, both browsers will have to open and parse the sub dir manifest files of both browsers. For firefox we have 16 manifests listed in chrome.manifest that are fx specific, inside those there are 156 lines of text. For metro, we have two manifests with 98 lines of text. I can do some tests, but this seems like it might hit ts in both browsers.
Comment 5 Jim Mathies [:jimm] 2012-05-16 16:42:56 PDT
Maybe we could keep the ids in the root for things that aren't in common containers, so for example components.manifest and browser.manifest could have sub ids but the components listed in the root like fuelApplication.manifest could have ids in the chrome.manifest file. There would still be added parsing but it wouldn't be as bad.
Comment 6 Jim Mathies [:jimm] 2012-05-17 04:16:48 PDT
Created attachment 624701 [details] [diff] [review]
patch v.2

Updates per previous comments.

This adds the dis to individual components while leaving the generic lists alone. It adds a little overhead to the metro browser, but shouldn't hurt fx to much.

I'll attach some example manifests that result from this.
Comment 7 Jim Mathies [:jimm] 2012-05-17 04:17:28 PDT
Created attachment 624702 [details]
chrome.manifest
Comment 8 Jim Mathies [:jimm] 2012-05-17 04:17:48 PDT
Created attachment 624703 [details]
browser.manifest
Comment 9 Jim Mathies [:jimm] 2012-05-17 04:18:13 PDT
Created attachment 624704 [details]
components.manifest
Comment 10 Jim Mathies [:jimm] 2012-05-17 04:19:50 PDT
(In reply to Jim Mathies [:jimm] from comment #6)
> This adds the dis to individual components while leaving the generic lists

dis -> ids
Comment 11 Jim Mathies [:jimm] 2012-05-17 04:27:39 PDT
Created attachment 624705 [details] [diff] [review]
mc compat patch

The original was off elm, this applies to mc.
Comment 12 Jim Mathies [:jimm] 2012-05-17 06:03:17 PDT
pushed to try:

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=6ff55fc37d10

One addition I had to add was to sync the two mk file changes with js/src/config.
Comment 13 Benjamin Smedberg [:bsmedberg] 2012-05-17 06:10:40 PDT
I think that we should not have separate app IDs, although I'd like gavin to help confirm this decision.

In terms of packaging, I think we should extend what webrt is doing, specifically:

ship the metro UI rooted from metro/ and metro/chrome.manifest
ship the desktop UI rooted from browser/ and browser/chrome.manifest
ship the core platform in platform/chrome.manifest (or toolkti/chrome.manifest, let the bikeshedding begin!)

This is kinda what we have today, except that the desktop and platform stuff is still combined and platform isn't in a subdirectory. I can probably make this separation happen pretty quickly.

As for profiles, will the metro browser and the desktop browser be running at the same time? The application ID is very much tied up with extensions and how extensions are installed and updated, and we cannot effectively have two different application IDs using the same profile, because all of the extension manager cache information will be confused.
Comment 14 Jim Mathies [:jimm] 2012-05-17 07:02:56 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> I think that we should not have separate app IDs, although I'd like gavin to
> help confirm this decision.
> 
> In terms of packaging, I think we should extend what webrt is doing,
> specifically:
> 
> ship the metro UI rooted from metro/ and metro/chrome.manifest
> ship the desktop UI rooted from browser/ and browser/chrome.manifest
> ship the core platform in platform/chrome.manifest (or
> toolkit/chrome.manifest, let the bikeshedding begin!)

This would make implementing this much simpler, I'd rather have metro running in it's own root. 

> 
> This is kinda what we have today, except that the desktop and platform stuff
> is still combined and platform isn't in a subdirectory. I can probably make
> this separation happen pretty quickly.

AFAICT with mixed chrome, components, locale resources, and now webapprt, I'm not sure how you separate these "quickly", but I'd love to see how you do it. :) 

> As for profiles, will the metro browser and the desktop browser be running
> at the same time?

That is the goal. Metro apps suspend when in the background, but desktop apps remain active even if you are not on the desktop. Having certain areas of the profile in complete sync is important. For example passwords, favorites, and the awesome bar need to be in sync, but other areas like cache can be separate.

However this is not a priority, getting things blended into mc so we can get nightly builds is, so two separate profiles for starters is ok.

> The application ID is very much tied up with extensions
> and how extensions are installed and updated, and we cannot effectively have
> two different application IDs using the same profile, because all of the
> extension manager cache information will be confused.

Ok, so sounds like your plan to split platform and app resources up is needed. What can I do to help?
Comment 15 Jim Mathies [:jimm] 2012-05-18 04:38:27 PDT
bsmedberg - this blocks a lot of other work. You mentioned splitting these resources up should be pretty easy to do, can provide some pointers?

The general idea here from what I understand would be to split platform / app chrome, components, extensions, defaults, modules, and plugins up so that the platform and the app store these resources in different folders. Also there's dictionaries and hyphenation as well which look to come from platform.

The problem I ran into with setting a root xre folder for metro (dist/bin/metro) was that platform resources were located in the upper (dist/bin/*) folders and couldn't be accessed. For example a manifest line like

manifest ../chrome/toolkit.manifest

in the metro sub folder doesn't work, since these manifest lines are restricted to the root.

I'm not sure how we should make this work. If we have resources split, for example under components:

dist/bin/components/toolkit/(resources)
dist/bin/components/metro/(resources)

we would still have to share a common manifest, and that would require the use of separate app ids. So I'm not sure how this should come together.
Comment 16 Benjamin Smedberg [:bsmedberg] 2012-05-18 10:52:10 PDT
WebRT already stores their app in a subdir, see http://mxr.mozilla.org/mozilla-central/source/webapprt/Makefile.in#19

I think I can make a more generalize case of splitting pretty easily, we just need to avoid having a default for FINAL_TARGET and have each makefile specify one separately. You could ship the metro "app" in a subdir just using this. However, the desktop UI and the platform components would still be mixed together.

The bootstrap code already loads the "platform" and the "app" separately, built in as part of XULRunner support. We might need to tweak where it looks for the platform by default (currently in the root of omnijar), but that should also be pretty straightforward.
Comment 17 Jim Mathies [:jimm] 2012-05-21 13:38:12 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #16)
> WebRT already stores their app in a subdir, see
> http://mxr.mozilla.org/mozilla-central/source/webapprt/Makefile.in#19
> 
> I think I can make a more generalize case of splitting pretty easily, we
> just need to avoid having a default for FINAL_TARGET and have each makefile
> specify one separately. You could ship the metro "app" in a subdir just
> using this.

I don't see how this would work with shared platform components, modules, and preferences - I can use FINAL_TARGET to place app resources in a sub dir but I don't see how the metro browser would then be able to load resources below that in the directory structure.
Comment 18 Jim Mathies [:jimm] 2012-05-24 11:27:16 PDT
With a great deal of make work and proper xre/app roots, I've managed to get everything separated out and loading properly with the exception of browser components listed in the root manifest. I'm currently just deleting entries fx adds to test. So I think if we can get the root manifest split up we should be in good shape.
Comment 19 Benjamin Smedberg [:bsmedberg] 2012-05-24 11:44:59 PDT
So why are the browser components in the root manifest? I had a patch in progress too, so we should make sure we aren't conflicting eachother.
Comment 20 Jim Mathies [:jimm] 2012-05-24 13:38:48 PDT
Created attachment 626947 [details] [diff] [review]
fennec xul migration to browser (diff off elm tip)
Comment 21 Jim Mathies [:jimm] 2012-05-24 13:42:09 PDT
Comment on attachment 626947 [details] [diff] [review]
fennec xul migration to browser (diff off elm tip)

hmm, some of my hg copies didn't seem to end up in the right place. let me try again.
Comment 22 Jim Mathies [:jimm] 2012-05-24 14:24:40 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #19)
> So why are the browser components in the root manifest? I had a patch in
> progress too, so we should make sure we aren't conflicting eachother.

I haven't worked on the splitting up of chrome.manifest resources. I've just been working on getting the metro browser code integrated in with the fx desktop build. I have everything working without any changes to the runtime *except* for the fact that firefox still places its entries in the root chrome.manifest file. To test I just go in and delete those entries. I have metro browser resources in dist/bin/metro, and I have it loading from there and loading runtime resources from dist/bin.
Comment 23 Jim Mathies [:jimm] 2012-05-24 14:28:40 PDT
Created attachment 626970 [details] [diff] [review]
base file copies
Comment 24 Jim Mathies [:jimm] 2012-05-24 14:35:04 PDT
Created attachment 626973 [details] [diff] [review]
base patch
Comment 25 Jim Mathies [:jimm] 2012-05-25 08:28:49 PDT
Comment on attachment 626973 [details] [diff] [review]
base patch

these changes are checked in on elm now.
Comment 26 Benjamin Smedberg [:bsmedberg] 2012-06-04 08:10:59 PDT
Created attachment 629782 [details] [diff] [review]
Part A, rev. 1 - Add DIST_SUBDIR makefile variable [checked in]
Comment 27 Benjamin Smedberg [:bsmedberg] 2012-06-04 08:13:18 PDT
Created attachment 629783 [details] [diff] [review]
Part B, rev. 1 - use DIST_SUBDIR in makefiles and launch using the subdir
Comment 28 Mike Hommey [:glandium] 2012-06-04 08:29:01 PDT
Comment on attachment 629783 [details] [diff] [review]
Part B, rev. 1 - use DIST_SUBDIR in makefiles and launch using the subdir

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

::: browser/app/nsBrowserApp.cpp
@@ +3,5 @@
>   * 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/. */
>  
>  #include "nsXULAppAPI.h"
> +#include "mozilla/AppData.h"

Where does that come from?

@@ +160,5 @@
>    }
>  
> +  ScopedAppData appData(&sAppData);
> +  nsCOMPtr<nsILocalFile> exeFile;
> +  rv = XRE_GetBinaryPath(argv[0], getter_AddRefs(exeFile));

You can use mozilla::BinaryPath::GetFile.

::: browser/base/Makefile.in
@@ +9,5 @@
>  VPATH   = @srcdir@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
> +DIST_SUBDIR = browser

I think it would be much simpler if it was implicit from the fact that we're under browser/. Especially, I'm afraid new files will lack this (especially those merged from fx-team). (And I'm afraid this effectively breaks ff-on-xr builds)
Comment 29 Benjamin Smedberg [:bsmedberg] 2012-06-04 09:02:12 PDT
Oh, appdata.h was a patch in the stack that is no longer needed, I'll fix that. I'm not sure why mozilla::BinaryPath is better than XRE_GetBinaryPath, but I can make that change...

I don't think there is any maintainable way to implicitly make the directories under browser/ use DIST_SUBDIR, especially because we want stuff like "make -C browser/components" to work, and because at least browser/app *doesn't* set DIST_SUBDIR. And I don't think that this will necessarily break FF-on-XR build at all: the FF-on-XR builds will just have a browser/ subdirectory as they have a webapprt/ subdir and would have a metro/ subdir on Windows, if we cared about Windows at all in that configuration.

Eventually I wanted to put the platform in a subdir also so that every makefile will choose what subdir they ship to, but that would require even more extensive changes than necessary now, since many of the custom platform makefiles don't use FINAL_TARGET.
Comment 30 Mike Hommey [:glandium] 2012-06-04 09:31:34 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #29)
> Oh, appdata.h was a patch in the stack that is no longer needed, I'll fix
> that. I'm not sure why mozilla::BinaryPath is better than XRE_GetBinaryPath,
> but I can make that change...

You don't need to load the function, contrary to XRE_GetBinaryPath.

> I don't think there is any maintainable way to implicitly make the
> directories under browser/ use DIST_SUBDIR, especially because we want stuff
> like "make -C browser/components" to work,

I think this is workable. something like:
DIST_SUBDIR := $(if $(filter $(abspath $(topsrcdir))/$(MOZ_BUILD_APP)%,$(abspath $(srcdir))),$(MOZ_BUILD_APP))

> and because at least browser/app *doesn't* set DIST_SUBDIR.

It could reset it.

> And I don't think that this will necessarily
> break FF-on-XR build at all: the FF-on-XR builds will just have a browser/
> subdirectory as they have a webapprt/ subdir and would have a metro/ subdir
> on Windows, if we cared about Windows at all in that configuration.

In a FF-on-XR build, application.ini is still going to be in dist/bin, not dist/bin/browser. How would XR pick stuff under browser, then? Also, make package would create a package with a browser/ subdirectory, and make install would install under /usr/lib/firefox-version/browser. It would seem that an empty DIST_SUBDIR is a proper way to build the FF part of FF-on-XR.

> Eventually I wanted to put the platform in a subdir also so that every
> makefile will choose what subdir they ship to, but that would require even
> more extensive changes than necessary now, since many of the custom platform
> makefiles don't use FINAL_TARGET.

The problem with explicit DIST_SUBDIR is that if you don't use it, it still builds, ships the file at a wrong location, and all that goes unnoticed. Which is why implicit would really be better.
Now, if platform was in a subdir as well, DIST_SUBDIR being empty would install at the wrong location and that could be detected. But as you say, it's much more extensive.
Comment 31 Benjamin Smedberg [:bsmedberg] 2012-06-04 12:00:40 PDT
Created attachment 629871 [details] [diff] [review]
Part C, rev. 1 - Move ScopedAppData into the XPCOM glue [checked in]

Oops, I did need that patch
Comment 32 Benjamin Smedberg [:bsmedberg] 2012-06-04 12:20:45 PDT
In a FF-on-XR build, application.ini should end up in dist/bin/browser and the files should all be there. This is because that FF will also ship metro and webapprt in the same filesystem.

I understand the concern about missing DIST_SUBDIR being a silent error, but I really don't think the other solutions are *better*, especially when almost all these errors are easy to see in the packaging manifest.
Comment 33 Mike Hommey [:glandium] 2012-06-04 13:10:28 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #32)
> In a FF-on-XR build, application.ini should end up in dist/bin/browser and
> the files should all be there.

Your patch doesn't do that ;)

> This is because that FF will also ship metro and webapprt in the same filesystem.

So, a FF-on-XR would be $ffdir/browser, $ffdir/webapprt and $ffdir/metro? Where would the xulrunner stub go? Where should the stub look for xulrunner? $ffdir/*/xulrunner? $ffdir/xulrunner? With the current code, the xulrunner stub needs to go next to application.ini. And it looks for a xulrunner subdirectory under the same directory. So you'd have $ffdir/browser/xulrunner-stub (renamed) and $ffdir/browser/xulrunner, and likewise for webapprt and metro? (also, the build system currently copies xulrunner from the libxul sdk info $dist/bin/xulrunner, so with that scheme, that'd make 3 copies)

> I understand the concern about missing DIST_SUBDIR being a silent error, but
> I really don't think the other solutions are *better*, especially when
> almost all these errors are easy to see in the packaging manifest.

I'm convinced we'd see pretty quickly dist/bin/some/file (as opposed to dist/bin/browser/some/file) added to packaging manifest because the corresponding Makefile won't have DIST_SUBDIR. Our build system is already too complex and error-prone, I don't think adding one more way people can screw things up, get confused, or waste time trying to understand why their build doesn't work, is very helpful :(
Comment 34 Benjamin Smedberg [:bsmedberg] 2012-06-04 13:46:50 PDT
(In reply to Mike Hommey [:glandium] from comment #33)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #32)
> > In a FF-on-XR build, application.ini should end up in dist/bin/browser and
> > the files should all be there.
> 
> Your patch doesn't do that ;)

No, but there's additional work you'd have to do for FF-on-XR anyway.

> So, a FF-on-XR would be $ffdir/browser, $ffdir/webapprt and $ffdir/metro?

Yes.

> Where would the xulrunner stub go? Where should the stub look for xulrunner?

You wouldn't be able to use the stock XULRunner stub, we'd have to use the nsBrowserApp stub with some tweaks to use a different location for XULRunner. But presumably it would continue to go in ffdir/xulrunner
Comment 35 Mike Hommey [:glandium] 2012-06-04 13:59:05 PDT
In case we go with the "include $(topsrcdir)/first-sub-dir/common.mk" pattern discussed on irc, adding the following to config/config.mk should be enough:

-include $(firstword $(subst /, ,$(patsubst $(abspath $(topsrcdir))/%,%,$(abspath $(srcdir)))))/common.mk
Comment 36 Mike Hommey [:glandium] 2012-06-05 05:34:16 PDT
Comment on attachment 629871 [details] [diff] [review]
Part C, rev. 1 - Move ScopedAppData into the XPCOM glue [checked in]

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

One thing: it might be better, for hg blame, to hg cp toolkit/xre/nsAppData.cpp toolkit/xre/CreateAppData.cpp and then do the necessary changes.
Comment 37 Benjamin Smedberg [:bsmedberg] 2012-06-05 12:55:38 PDT
I tried the `hg cp` route, but it gives unpleasant warnings about divergent renames which are probably harmless but I didn't want to tempt fate. So I'll leave it this way, if that's ok.
Comment 38 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-07 12:33:15 PDT
FWIW you should ignore the "divergent rename" warnings, they're absolutely useless and don't represent anything to be worried about.
Comment 39 Mike Hommey [:glandium] 2012-06-08 05:32:47 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #31)
> Created attachment 629871 [details] [diff] [review]
> Part C, rev. 1 - Move ScopedAppData into the XPCOM glue
> 
> Oops, I did need that patch

I've successfully built and used webapprt with FF-on-XR without this patch (but got other problems). Are you sure you need it?
Comment 40 Benjamin Smedberg [:bsmedberg] 2012-06-08 05:37:28 PDT
This patch was to make part B compile for metro and really has little to do with webapprt except that it's along for the ride.
Comment 42 Mike Hommey [:glandium] 2012-06-19 14:00:23 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #41)
> Part A: https://hg.mozilla.org/integration/mozilla-inbound/rev/d977dae53ff9
> Part C: https://hg.mozilla.org/integration/mozilla-inbound/rev/efe2ccacf040

Could you land a fixup applying the webapprt changes in webapprt/gtk2, too?
Comment 44 Benjamin Smedberg [:bsmedberg] 2012-06-22 08:59:51 PDT
Created attachment 635773 [details] [diff] [review]
Part B, rev. 2 - use DIST_SUBDIR in makefiles and launch using the subdir

Rebased and BinaryPath fixed. Marking r? for ted to at least decide whether the browser-wide makefile changes are correct or which other solution is most maintainable.

This still requires packaging and then l10n-repack changes.
Comment 45 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-06-25 06:48:41 PDT
Comment on attachment 635773 [details] [diff] [review]
Part B, rev. 2 - use DIST_SUBDIR in makefiles and launch using the subdir

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

::: browser/app/profile/extensions/Makefile.in
@@ +7,5 @@
>  topsrcdir  = @top_srcdir@
>  srcdir     = @srcdir@
>  VPATH      = @srcdir@
>  
> +DISTROEXT = $(call core_abspath,$(DIST))/bin/browser/distribution/extensions

Hm, if I'm understanding the implications of this bug correctly... this will be the "right" destination, in that $XREAppDist/extensions will map here. However, if desktop and metro have the same app ID, are sharing a profile, yet have different app directories, then launching metro won't install/update the distribution extensions. And I think that means they'd only be installed/updated if the desktop app is what's launched *first* directly after an update. In which case, we'd want distribution extensions to be in a common directory (ie, $(DIST)/bin/distribution/extensions).
Comment 46 Benjamin Smedberg [:bsmedberg] 2012-06-25 07:15:38 PDT
They will not have the same app ID and will not share a profile. In addition, we do not intend to support any extensions in the Metro side for the time being.
Comment 47 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-06-25 19:38:17 PDT
Ah, ok - that clears things up. Thanks.
Comment 48 Benjamin Smedberg [:bsmedberg] 2012-06-26 07:48:49 PDT
Joey is going to take this bug since I am stuck working on Flash stuff for a couple weeks. Joey, basically what's left for this is packaging mechanics. In particular, my plan was:

* take everything from about http://hg.mozilla.org/mozilla-central/annotate/74e503bfa575/toolkit/mozapps/installer/packager.mk#l459 to to about 458 and refactor that into a python script. That way desktop and platform and webapprt (and the future metro) can use the same code path.

The omnijar cache will also need to be tweaked, but I'm not sure exactly how: are we going to have one cache for desktop and a separate one for platform, or are they shared? I had something like this in my tree, but untested:

-ifdef LIBXUL_SDK
 PRECOMPILE_DIR=XCurProcD
 PRECOMPILE_RESOURCE=app
 PRECOMPILE_GRE=$(LIBXUL_DIST)/bin
-else
-PRECOMPILE_DIR=GreD
-PRECOMPILE_RESOURCE=gre
-PRECOMPILE_GRE=$$PWD
-endif
 
 # Silence the unzip step so we don't print any binary data from the comment field.
 GENERATE_CACHE = \
-  $(_ABS_RUN_TEST_PROGRAM) $(LIBXUL_DIST)/bin/xpcshell$(BIN_SUFFIX) -g "$(PRECOMPILE_GRE)" -a "$$PWD" -f $(call core_abspath,$(MOZILLA_DIR)/toolkit/mozapps/installer/precompile_cache.js) -e "populate_startupcache('$(PRECOMPILE_DIR)', '$(OMNIJAR_NAME)', 'startupCache.zip');" && \
+  $(_ABS_RUN_TEST_PROGRAM) $(LIBXUL_DIST)/bin/xpcshell$(BIN_SUFFIX) -g "$(PRECOMPILE_GRE)" -a "$(abspath $(DIST)/bin/browser)" -f $(call core_abspath,$(MOZILLA_DIR)/toolkit/mozapps/installer/precompile_cache.js) -e "populate_startupcache('$(PRECOMPILE_DIR)', '$(OMNIJAR_NAME)', 'startupCache.zip');" && \

With those changes we should be able to get tryserver/en-US nightlies to work, which is sufficient to unblock Jim. One of us can worry about fixing l10n repackaging later.
Comment 49 Mike Hommey [:glandium] 2012-06-26 07:57:56 PDT
startup cache needs to be stored in the omnijar containing the stuff being cached. So jsloader/resource/gre paths would be in the "main" omni.jar, and jsloader/resource/app paths would be in browser/metro omni.jar.
Comment 50 Joey Armstrong [:joey] 2012-06-28 08:45:46 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #48)
> In particular, my plan was:
> 
> * take everything from about
> http://hg.mozilla.org/mozilla-central/annotate/74e503bfa575/toolkit/mozapps/
> installer/packager.mk#l459 to to about 458 and refactor that into a python
> script. That way desktop and platform and webapprt (and the future metro)
> can use the same code path.

Unit tests should be setup before poking too hard at internals of the packager makefile.  Chance for breakage or omissions will probably be high if the code moves in bulk.
Comment 51 Ted Mielczarek [:ted.mielczarek] 2012-06-29 12:46:47 PDT
Comment on attachment 635773 [details] [diff] [review]
Part B, rev. 2 - use DIST_SUBDIR in makefiles and launch using the subdir

I'm on vacation for the next week, so if you want this looked at you'll need someone else.
Comment 52 Jim Mathies [:jimm] 2012-07-01 08:26:55 PDT
Joey, any estimate on when you'll be able to get to this? This bug is holding up a lot of integration work for the metro front end.
Comment 53 Joey Armstrong [:joey] 2012-07-02 08:26:14 PDT
(In reply to Jim Mathies [:jimm] from comment #52)
> Joey, any estimate on when you'll be able to get to this? This bug is
> holding up a lot of integration work for the metro front end.

Jim, no ETA yet.  packager.mk internals will not be trivial to rip apart and could cause breakage if any variant behavior is induced.  There is no unit testing protecting this area aside from the final packaged targets.

fyi> I'll be on vacation in the Adirondacks starting next week.
Comment 54 Chris Cooper [:coop] 2012-07-04 10:49:45 PDT
(In reply to Joey Armstrong [:joey] from comment #53)
> fyi> I'll be on vacation in the Adirondacks starting next week.

Joey leaves on vacation on Friday (6th-16th). Someone else with build config chops is going to have to pick this up if it needs progress in the interim.
Comment 55 Johnathan Nightingale [:johnath] 2012-07-05 07:27:36 PDT
It definitely needs help before then, it's blocking a lot of Metro work, but I am worried because the skillset is pretty specialized AIUI. Joey - can we get you to post your WIP stuff so that would-be pinch hitters can pick up where you left off?
Comment 56 Johnathan Nightingale [:johnath] 2012-07-05 14:25:51 PDT
(In reply to Johnathan Nightingale [:johnath] from comment #55)
> It definitely needs help before then, it's blocking a lot of Metro work, but
> I am worried because the skillset is pretty specialized AIUI. Joey - can we
> get you to post your WIP stuff so that would-be pinch hitters can pick up
> where you left off?

(increasingly close to joey-PTO ping?)
Comment 57 Chris Cooper [:coop] 2012-07-12 13:32:06 PDT
(In reply to Johnathan Nightingale [:johnath] from comment #56)
> (increasingly close to joey-PTO ping?)

Joey is back on Monday (16th), but I'm unsure how much progress he made here before he left. 

In the interim, I've asked Callek to have a look and see whether he can provide any insight and/or help until Joey returns.
Comment 58 Justin Wood (:Callek) 2012-07-14 18:36:13 PDT
Created attachment 642303 [details] [diff] [review]
Part B, rev. 2.1 - use DIST_SUBDIR in makefiles and launch using the subdir

This is part B, unbitrotted
Comment 59 Justin Wood (:Callek) 2012-07-15 02:21:15 PDT
Created attachment 642355 [details] [diff] [review]
Part B, rev. 2.2 - use DIST_SUBDIR in makefiles and launch using the subdir

Unbitrotted better (no nsILocalFile, corrects some syntax errors/typos by ben) This gets the compile run correctly (afaict), of course when run from dist/bin/firefox.exe we get an inability to load a webpage, or do much of anything it seems.

We still have packaging bustage, (with xptlink) and xptlink parts of packaging are a black box to me, so unless I can find a magic solution to fix it in a few hours of work, I'm handing back to Joey for him to pickup on monday.
Comment 60 Mike Hommey [:glandium] 2012-07-15 03:01:45 PDT
(In reply to Justin Wood (:Callek) from comment #59)
> Created attachment 642355 [details] [diff] [review]
> Part B, rev. 2.2 - use DIST_SUBDIR in makefiles and launch using the subdir
> 
> Unbitrotted better (no nsILocalFile, corrects some syntax errors/typos by
> ben) This gets the compile run correctly (afaict), of course when run from
> dist/bin/firefox.exe we get an inability to load a webpage, or do much of
> anything it seems.
> 
> We still have packaging bustage, (with xptlink) and xptlink parts of
> packaging are a black box to me, so unless I can find a magic solution to
> fix it in a few hours of work, I'm handing back to Joey for him to pickup on
> monday.

There's a spurious ifdef XP_MACOSX in package-manifest.in that makes half of the package manifest to be ignored. This further shows that a few more things are missing in the package-manifest.in wrt files being moved under dist/bin/browser. There's also a correction to be made to browser/locales/jar.mn for webapprt, where ../webapprt/chrome/@AB_CD@.jar needs to be replaced with ../../webapprt/chrome/@AB_CD@.jar.

For xptlink, the browser and toolkit xpts need to be put in different sections of the package-manifest so that xptlink can generate different xpt files. However, it currently doesn't support to put the different xpt files in different directories.

The following part that does write new manifests probably needs some attention, too.
Comment 61 Justin Wood (:Callek) 2012-07-15 23:17:54 PDT
Handing back to joey since he is coming back, I spent a bunch of time on trying to cleanup this first part of the patch over friday and the weekend, sadly more time that I should have (been away from Cpp editing for too long) And I don't know xptlink stuff properly :(

the #ifdef XP_MACOSX stray should be deleted, but I'm not posting a new patch for that.

Hopefully this unbitrotting gives Joey something to go on, and that he can take this and run with it better, I expect he'll be able to churn it out much faster than I could.
Comment 62 Brian R. Bondy [:bbondy] 2012-08-11 06:51:11 PDT
Any update on this? This is the main bug holding us up from merging the Metro work into m-c. If you want, Jim or I can jump in to help if you aren't available to work on it soon.
Comment 63 Brian R. Bondy [:bbondy] 2012-08-11 06:51:52 PDT
Thanks for your work on it so far by the way.
Comment 64 Joey Armstrong [:joey] 2012-08-16 06:32:16 PDT
A list of tasks for the metro build are logged here.  Feel free to add anything missing:  https://joey.etherpad.mozilla.org/metro-build
Comment 65 Jim Mathies [:jimm] 2012-08-20 08:49:06 PDT
Adding a few of the build config bugs that have been filed.
Comment 66 Mike Hommey [:glandium] 2012-08-30 12:54:22 PDT
Created attachment 657000 [details] [diff] [review]
Move browser application in a subdirectory (PoC)

This replaces Part B, rev 2.2.

This requires the patches from bug 787165, bug 785871, bug 780561, bug 787180, and bug 787184.
Comment 67 Mike Hommey [:glandium] 2012-08-30 14:06:23 PDT
Created attachment 657023 [details] [diff] [review]
Move browser application in a subdirectory (PoC)

Forgot to hg add the additional files in previous patch.
Comment 68 Jim Mathies [:jimm] 2012-08-30 14:22:22 PDT
(In reply to Mike Hommey [:glandium] from comment #66)
> Created attachment 657000 [details] [diff] [review]
> Move browser application in a subdirectory (PoC)
> 
> This replaces Part B, rev 2.2.
> 
> This requires the patches from bug 787165, bug 785871, bug 780561, bug
> 787180, and bug 787184.

Mike, is it too early to pull these down and start experimenting on metro builds?
Comment 69 Mike Hommey [:glandium] 2012-08-30 14:32:36 PDT
Created attachment 657040 [details] [diff] [review]
Move browser application in a subdirectory (PoC)

There were some problems with the package manifest.
Comment 70 Mike Hommey [:glandium] 2012-08-30 14:43:25 PDT
(In reply to Jim Mathies [:jimm] from comment #68)
> Mike, is it too early to pull these down and start experimenting on metro
> builds?

I'm not sure it works properly on windows yet, but the iteration without jar/omnijar support did, so if it's broken, it should be a trivial fix. Provided it works, it should be straightforward to enable metro builds on top of that.
You just need to add two sections to browser/installer/package.manifest: [metro] and [metro-omni]. The new packager is smart, and handles most things that are registered in chrome.manifest, so you only need to add that file to any of the two sections to have all chrome registered (except if you have resource registrations that use paths that are not part of chrome). Check the [browser] and [browser-omni] sections for ideas.
Comment 71 Mike Hommey [:glandium] 2012-08-30 15:21:48 PDT
Created attachment 657065 [details] [diff] [review]
Move browser application in a subdirectory (PoC)

This, and the update to bug 780561, should make it build on windows.
Comment 72 Mike Hommey [:glandium] 2012-08-31 01:50:55 PDT
(In reply to Mike Hommey [:glandium] from comment #71)
> Created attachment 657065 [details] [diff] [review]
> Move browser application in a subdirectory (PoC)
> 
> This, and the update to bug 780561, should make it build on windows.

It does build on windows (if you don't count l10n issues). There are still issues to solve with the resulting builds (xpcshell tests breakage, several talos breakages, and leaks).
Comment 73 Mike Hommey [:glandium] 2012-08-31 10:06:54 PDT
(In reply to Mike Hommey [:glandium] from comment #72)
> It does build on windows (if you don't count l10n issues). There are still
> issues to solve with the resulting builds (xpcshell tests breakage, several
> talos breakages, and leaks).

bug 787443 and bug 782890 make things better, but there still are issues.
Comment 74 Mike Hommey [:glandium] 2012-09-05 11:08:49 PDT
Created attachment 658552 [details] [diff] [review]
Move browser application in a subdirectory (PoC)

Refreshed against latest patch from bug 780561.
Comment 75 Jim Mathies [:jimm] 2012-09-06 11:43:54 PDT
(In reply to Mike Hommey [:glandium] from comment #74)
> Created attachment 658552 [details] [diff] [review]
> Move browser application in a subdirectory (PoC)
> 
> Refreshed against latest patch from bug 780561.

I'm not sure what the original goal was here. With these changes we get firefox.exe in dist/bin/browser. This won't work since the exe depends on all the platform libs in dist/bin. There's a PROGRAM_DEST defined in /browser/app/Makefile.in, so I think the idea here was to move firefox.exe down manually. But PROGRAM_DEST currently isn't being used in any way by the file.

I landed a one-off fix on elm that gets things running again - 

https://hg.mozilla.org/projects/elm/rev/950e45478d14 

But this isn't the right fix. We end up with two copies of firefox.exe, one in browser and one in the root.
Comment 76 Jim Mathies [:jimm] 2012-09-06 11:48:50 PDT
Ah, I think it was supposed to be PROGRAMS_DEST with an 'S'.
Comment 77 Mike Hommey [:glandium] 2012-09-06 12:08:02 PDT
(In reply to Jim Mathies [:jimm] from comment #76)
> Ah, I think it was supposed to be PROGRAMS_DEST with an 'S'.

Yeah, turns out i had to update this patch after i completed bug 787184 (the previous patch was partial)
Comment 78 Brian R. Bondy [:bbondy] 2012-09-28 04:35:01 PDT
Should we be requesting that RelEng turns on all OS for tinderbox/nightly testing of this work on other platforms as well? If you want, I can post a bug for that.
Perhaps we could add a new commit message flag similar to DONTBUILD but something like ALLPLATFORMS so we don't waste resources.
Comment 79 Brian R. Bondy [:bbondy] 2012-09-28 04:36:36 PDT
Comment 78 was in regards to elm by the way which only builds Windows currently.
Comment 80 Mike Hommey [:glandium] 2012-09-28 05:50:45 PDT
I'd say it should be fine for now. We can do one-offs on try.
Comment 81 Brian R. Bondy [:bbondy] 2012-09-28 07:45:29 PDT
OK cool, at least for Nightly builds we should test upgrades on all platforms before we push everything to m-c.
Comment 82 Jim Mathies [:jimm] 2012-11-17 08:56:19 PST
removing a couple dep bugs that block bug 789461
Comment 83 Jim Mathies [:jimm] 2012-12-06 03:49:05 PST
Moving new package manifest bugs down to the new-packager bug.
Comment 84 Mike Hommey [:glandium] 2012-12-11 00:48:06 PST
Created attachment 690768 [details] [diff] [review]
Use DIST_SUBDIR for XPIs too

This is required for l10n.
Comment 85 Mike Hommey [:glandium] 2012-12-11 07:40:14 PST
Comment on attachment 690768 [details] [diff] [review]
Use DIST_SUBDIR for XPIs too

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfbea498e32a
Comment 86 Ed Morley [:emorley] 2012-12-12 02:11:27 PST
https://hg.mozilla.org/mozilla-central/rev/cfbea498e32a
Comment 87 Mike Hommey [:glandium] 2012-12-27 05:07:57 PST
Created attachment 696013 [details] [diff] [review]
Don't use the xulrunner stub when building Firefox against a libxul SDK

Most of the additional code in nsBrowserApp.cpp is stolen from xulrunner/stub/nsXULStub.cpp.
Comment 88 Mike Hommey [:glandium] 2012-12-27 05:09:08 PST
Created attachment 696014 [details] [diff] [review]
Move browser application in a subdirectory
Comment 89 Mike Hommey [:glandium] 2012-12-27 05:10:06 PST
*** Bug 817602 has been marked as a duplicate of this bug. ***
Comment 90 Mike Hommey [:glandium] 2012-12-27 05:25:03 PST
Created attachment 696018 [details] [diff] [review]
Don't use the xulrunner stub when building Firefox against a libxul SDK

With a typo fixed.
Comment 91 Jim Mathies [:jimm] 2012-12-27 05:32:54 PST
(In reply to Mike Hommey [:glandium] from comment #88)
> Created attachment 696014 [details] [diff] [review]
> Move browser application in a subdirectory

Should package manifest instructions be organized by gre/app? Maybe this isn't possible but it seems rather disorganized to have intermixed @BINPATH@/gre-thing and @BINPATH@/browser/app-thing throughout.
Comment 92 Jim Mathies [:jimm] 2012-12-27 05:34:08 PST
(Maybe a cleanup and re-org the browser manifest bug might be better in a follow up?)
Comment 93 Mike Hommey [:glandium] 2012-12-27 06:08:08 PST
(In reply to Jim Mathies [:jimm] from comment #91)
> (In reply to Mike Hommey [:glandium] from comment #88)
> > Created attachment 696014 [details] [diff] [review]
> > Move browser application in a subdirectory
> 
> Should package manifest instructions be organized by gre/app? Maybe this
> isn't possible but it seems rather disorganized to have intermixed
> @BINPATH@/gre-thing and @BINPATH@/browser/app-thing throughout.

On one hand, I agree, but on the other, it makes the patch less easy to review, and errors easier to slide in.
Comment 94 Mike Hommey [:glandium] 2012-12-27 07:58:48 PST
Created attachment 696046 [details] [diff] [review]
Don't use the xulrunner stub when building Firefox against a libxul SDK

With the memory leak of xreDirectory plugged.
Comment 95 Jim Mathies [:jimm] 2012-12-31 11:59:23 PST
(In reply to Mike Hommey [:glandium] from comment #88)
> Created attachment 696014 [details] [diff] [review]
> Move browser application in a subdirectory

Added today over on elm after an mc merge - 

https://hg.mozilla.org/projects/elm/rev/5357c90ace51
Comment 96 Jim Mathies [:jimm] 2012-12-31 13:13:08 PST
We also needed a new appdir directive for sessionstore tests added in bug 532150 -

https://hg.mozilla.org/projects/elm/rev/174b33cde180
Comment 98 Ed Morley [:emorley] 2013-01-04 10:02:18 PST
https://hg.mozilla.org/mozilla-central/rev/f438f50b8bb3
Comment 99 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-01-16 10:49:05 PST
This added multiple XPCOM static ctor/dtor warnings.
Comment 100 Mike Hommey [:glandium] 2013-01-16 10:56:40 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #99)
> This added multiple XPCOM static ctor/dtor warnings.

which ones?
Comment 101 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-01-16 11:03:39 PST
Kyle Huey@KYLEHUEY-PC /c/dev/mozilla-inbound
$ ./obj-i686-pc-mingw32/dist/bin/firefox.exe -no-remote -P test
WARNING: XPCOM objects created/destroyed from static ctor/dtor: file c:/dev/mozi
lla-inbound/xpcom/base/nsTraceRefcntImpl.cpp, line 141
WARNING: XPCOM objects created/destroyed from static ctor/dtor: file c:/dev/mozi
lla-inbound/xpcom/base/nsTraceRefcntImpl.cpp, line 141
WARNING: XPCOM objects created/destroyed from static ctor/dtor: file c:/dev/mozi
lla-inbound/xpcom/base/nsTraceRefcntImpl.cpp, line 141
WARNING: XPCOM objects created/destroyed from static ctor/dtor: file c:/dev/mozi
lla-inbound/xpcom/base/nsTraceRefcntImpl.cpp, line 141
Comment 102 Mike Hommey [:glandium] 2013-01-18 03:00:09 PST
Created attachment 703838 [details] [diff] [review]
Initialize trace malloc before calling NS_NewLocalFile in nsBrowserApp.cpp

This removes the trace malloc warnings from comment 101. I don't think it's worth caring to run NS_LogTerm in case InitXPCOMGlue fails (which would make things much more complicated, since both NS_LogInit/NS_LogTerm can only run after XPCOMGlueStartup)
Comment 104 :Ms2ger (⌚ UTC+1/+2) 2013-01-19 10:26:10 PST
https://hg.mozilla.org/mozilla-central/rev/ab31d2237244
Comment 105 Mike Hommey [:glandium] 2013-01-25 10:48:51 PST
Created attachment 706487 [details] [diff] [review]
Move browser application in a subdirectory

Refreshed against current m-c
Comment 106 Mike Hommey [:glandium] 2013-01-25 13:13:17 PST
Created attachment 706557 [details] [diff] [review]
Move browser application in a subdirectory

Added missing browser/chrome.manifest, necessary with the current packager code.
Comment 107 Jim Mathies [:jimm] 2013-02-06 02:55:52 PST
Should we bump the CLOBBER file for this? I think we'll want people to clear out their obj directories to get the right file layout.
Comment 108 Mike Hommey [:glandium] 2013-02-06 03:02:07 PST
(In reply to Jim Mathies [:jimm] from comment #107)
> Should we bump the CLOBBER file for this? I think we'll want people to clear
> out their obj directories to get the right file layout.

Yes we should. I have an update for the patch, so i'll put it in there.
Comment 109 Mike Hommey [:glandium] 2013-02-06 03:11:41 PST
Created attachment 710608 [details] [diff] [review]
Move browser application in a subdirectory

Refreshed against current m-c. I also changed the location of the testpilot addon in the package-manifest, because that's where the Makefile installs it, but i haven't checked if the distribution/extensions directory is actually used from there.
Comment 110 Mike Hommey [:glandium] 2013-02-06 03:32:03 PST
Created attachment 710614 [details] [diff] [review]
Move browser application in a subdirectory

With XPI_ROOT_APPID
Comment 111 Matt Brubeck (:mbrubeck) 2013-02-08 07:10:25 PST
Adding the add-on-compat keyword, because if I understand correctly this will break add-ons using resource: URIs that point to app when they should point to gre.
Comment 112 Jim Mathies [:jimm] 2013-02-09 05:29:20 PST
Note, there were changes to the packager manifest in the latest mc merge to elm.
Comment 113 Mike Hommey [:glandium] 2013-02-11 04:55:00 PST
Created attachment 712403 [details] [diff] [review]
Move browser application in a subdirectory
Comment 114 Mike Hommey [:glandium] 2013-02-11 05:23:43 PST
https://hg.mozilla.org/mozilla-central/rev/784b9beebe90
Comment 115 Jan Beich 2013-02-11 08:17:26 PST
--disable-updater is broken

gmake[1]: Entering directory `/m-c/obj/browser/installer'
/m-c/obj/_virtualenv/bin/python ../../../toolkit/mozapps/installer/packager.py -DMOZ_GLUE_IN_PROGRAM -DNO_NSPR_10_SUPPORT -DAB_CD=en-US -DMOZ_APP_NAME=firefox -DPREF_DIR=defaults/preferences -DMOZ_ENABLE_GNOME_COMPONENT=1 -DMOZ_GTK2=1 -DMOZ_NATIVE_NSPR=1 -DMOZ_NATIVE_NSS=1 -DJAREXT= -DMOZ_CHILD_PROCESS_NAME=plugin-container -DMOZ_JSDEBUGGER -DDLL_PREFIX=lib -DDLL_SUFFIX=.so -DBIN_SUFFIX= -DBINPATH=bin \
        --format omni \
        --removals ../../../browser/installer/removed-files.in \
         \
         \
        --jarlogs /m-c/obj/browser/installer/../../jarlog//en-US \
        --optimizejars \
         \
        package-manifest ../../dist ../../dist/firefox \

Error: /m-c/obj/browser/installer/package-manifest:379: Missing file(s): bin/icons/updater.png
Traceback (most recent call last):
  File "../../../toolkit/mozapps/installer/packager.py", line 364, in <module>
    main()
  File "../../../toolkit/mozapps/installer/packager.py", line 320, in main
    copier.add(mozpack.path.join(binpath, 'removed-files'), removals)
  File "/usr/local/lib/python2.7/contextlib.py", line 24, in __exit__
    self.gen.next()
  File "/m-c/python/mozbuild/mozpack/errors.py", line 129, in accumulate
    raise AccumulatedErrors()
mozpack.errors.AccumulatedErrors
gmake[1]: *** [stage-package] Error 1
Comment 116 Mike Hommey [:glandium] 2013-02-11 08:20:50 PST
(In reply to Jan Beich from comment #115)
> Error: /m-c/obj/browser/installer/package-manifest:379: Missing file(s):
> bin/icons/updater.png

Please file a followup bug.
Comment 117 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-02-13 12:00:16 PST
> Cross-post from bug 827354:

I've done some basic dogfooding of Nightly builds since landing these changes and have found no regressions. 

Testing included:
* Running Mozmill functional automation
* Running Mozmill update automation
* Running manual update tests with partial and complete updates
* Testing installation and startup (stub and normal installer)
* Testing crash reporter and session restore
* Testing various Web Developer tools
* Testing private browsing
* Testing add-ons and the add-ons manager
* Testing video playback (html and flash)
* Testing PDF.js and Social API

Platforms Tested:
* Windows 7 64-bit
* Windows 8 32-bit
* Mac OSX 10.8
* Ubuntu 12.04 32-bit

Addons Tested:
* Adblock Plus 2.2.2
* Crash Me Now! Advanced 0.4
* Download Statusbar 0.9.10
* DownloadHelper 4.9.13
* DownThemAll! 2.0.15
* Easy YouTube Video Downloader 6.7
* Firebug 1.11.1
* FlashGot 1.5.4.1
* Greasemonkey 1.7.1
* ImTranslator 5.1.1
* InvisibleHand 3.8.28
* NoScript 2.6.4.4
* WOT 20130129

I will be advising our community to keep an eye out for any issues and escalate them as appropriate. Please let me know if there are any high risk areas you think we've missed.
Comment 118 Mike Kaply [:mkaply] 2013-02-13 12:12:09 PST
Is there any possibility this broke autoconfig?

https://bugzilla.mozilla.org/show_bug.cgi?id=841011

This is the only patch that is even in the ballpark.
Comment 119 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-02-13 13:38:17 PST
Jim asked me via IRC to do some testing around themes/personas given bug 841094. I've been able to confirm that issue but not been able to find any other regressions around it.
Comment 120 Mike Kaply [:mkaply] 2013-04-15 07:48:09 PDT
This needs a section in the release notes since it has a major effect on enterprises using Firefox.

Things like distribution, defaults/preferences and more are now all located under the browser directory. Autoconfig is still at the top.

See:

https://bugzilla.mozilla.org/show_bug.cgi?id=841011
https://bugzilla.mozilla.org/show_bug.cgi?id=842334
Comment 121 :Gijs Kruitbosch 2013-04-23 07:44:17 PDT
Setting dev-doc-needed per comment #120 and eg. bug 844517. I'll write something up on https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/21 and then someone from the dev doc team can polish if need be. :-)
Comment 122 bhavana bajaj [:bajaj] 2013-05-16 12:46:06 PDT
(In reply to Michael Kaply (mkaply) from comment #120)
> This needs a section in the release notes since it has a major effect on
> enterprises using Firefox.
> 
> Things like distribution, defaults/preferences and more are now all located
> under the browser directory. Autoconfig is still at the top.
> 
> See:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=841011
> https://bugzilla.mozilla.org/show_bug.cgi?id=842334

I will make sure to include a relnote this for Firefox 24 which is when the next ESR is targeted.
Comment 123 Benjamin Smedberg [:bsmedberg] 2013-05-16 12:52:22 PDT
bajaj, I'm not sure there's anything left to relnote, since that change mostly got backed out in bug 842334.
Comment 124 neil@parkwaycc.co.uk 2014-10-08 15:49:40 PDT
Comment on attachment 696046 [details] [diff] [review]
Don't use the xulrunner stub when building Firefox against a libxul SDK

>+  if (!FileExists(exePath)) {
>+#if defined(LIBXUL_SDK) && defined(XP_MACOSX)
>+    // Check for <bundle>/Contents/Frameworks/XUL.framework/libxpcom.dylib
>+    bool greFound = false;
...
>+  }
>+  if (!greFound) {
>+#endif
>+    Output("Could not find the Mozilla runtime.\n");
>+    return NS_ERROR_FAILURE;
>+  }
Obviously nobody has ever tried building Firefox on the Mac against the libxul SDK; this isn't going to compile because greFound is scoped inside the previous block.
Comment 125 Mike Hommey [:glandium] 2014-10-08 16:27:29 PDT
(In reply to neil@parkwaycc.co.uk from comment #124)
> Obviously nobody has ever tried building Firefox on the Mac against the
> libxul SDK; this isn't going to compile because greFound is scoped inside
> the previous block.

Huh? Both are in the same #if #endif block.

(And nowadays, building against the libxul sdk is not even possible)
Comment 126 :Gavin Sharp [email: gavin@gavinsharp.com] 2014-10-20 13:28:57 PDT
(In reply to Mike Hommey [:glandium] from comment #125)
> Huh? Both are in the same #if #endif block.

Yeah, but greFound is declared inside the "if (!FileExists(exePath))" block, and then referenced outside of it.

> (And nowadays, building against the libxul sdk is not even possible)

Should we file a bug to rip out the ifdefs, then?
Comment 127 Mike Hommey [:glandium] 2014-10-20 15:38:41 PDT
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #126)
> > (And nowadays, building against the libxul sdk is not even possible)
> 
> Should we file a bug to rip out the ifdefs, then?

bsmedberg wanted to wait to be sure we wouldn't bring it back (bug 1038639 comment 3). The option removal is now in firefox 33, which has just been released. Let's wait a bit more and see if anyone complains during the fx33 cycle, and then let's remove it.

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