Jim Mathies [:jimm] Reporter
	Description • 12 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 1 • 12 years ago

A more complete patch with some additional changes to JarMaker and target_libs.mk.

	Benjamin Smedberg
	Comment 2 • 12 years ago

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)"

	Jim Mathies [:jimm] Reporter
	Comment 3 • 12 years ago

(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.

	Jim Mathies [:jimm] Reporter
	Comment 4 • 12 years ago

(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.

	Jim Mathies [:jimm] Reporter
	Comment 5 • 12 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 6 • 12 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 10 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #6)
> This adds the dis to individual components while leaving the generic lists

dis -> ids

	Jim Mathies [:jimm] Reporter
	Comment 11 • 12 years ago

The original was off elm, this applies to mc.

	Jim Mathies [:jimm] Reporter
	Comment 12 • 12 years ago

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.

	Benjamin Smedberg
	Comment 13 • 12 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 14 • 12 years ago

(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?

	Jim Mathies [:jimm] Reporter
	Comment 15 • 12 years ago

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.

	Benjamin Smedberg
	Comment 16 • 12 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 17 • 12 years ago

(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.

	Jim Mathies [:jimm] Reporter
	Comment 18 • 12 years ago

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.

	Benjamin Smedberg
	Comment 19 • 12 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 21 • 12 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 22 • 12 years ago

(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.

	Jim Mathies [:jimm] Reporter
	Comment 25 • 12 years ago

Comment on attachment 626973 [details] [diff] [review]
base patch

these changes are checked in on elm now.

	Mike Hommey [:glandium] Assignee
	Comment 28 • 12 years ago

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)

	Benjamin Smedberg
	Comment 29 • 12 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 30 • 12 years ago

(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.

	Benjamin Smedberg
	Comment 31 • 12 years ago

Oops, I did need that patch

	Benjamin Smedberg
	Comment 32 • 12 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 33 • 12 years ago

(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 :(

	Benjamin Smedberg
	Comment 34 • 12 years ago

(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

	Mike Hommey [:glandium] Assignee
	Comment 35 • 12 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 36 • 12 years ago

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.

	Benjamin Smedberg
	Comment 37 • 12 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 38 • 12 years ago

FWIW you should ignore the "divergent rename" warnings, they're absolutely useless and don't represent anything to be worried about.

	Mike Hommey [:glandium] Assignee
	Comment 39 • 12 years ago

(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?

	Benjamin Smedberg
	Comment 40 • 12 years ago

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.

	Benjamin Smedberg
	Comment 41 • 12 years ago

Part A: https://hg.mozilla.org/integration/mozilla-inbound/rev/d977dae53ff9
Part C: https://hg.mozilla.org/integration/mozilla-inbound/rev/efe2ccacf040

	Mike Hommey [:glandium] Assignee
	Comment 42 • 12 years ago

(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?

	Ed Morley [:emorley]
	Comment 43 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/efe2ccacf040
https://hg.mozilla.org/mozilla-central/rev/d977dae53ff9

	Benjamin Smedberg
	Comment 44 • 12 years ago

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.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 45 • 12 years ago

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).

	Benjamin Smedberg
	Comment 46 • 12 years ago

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.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 47 • 12 years ago

Ah, ok - that clears things up. Thanks.

	Benjamin Smedberg
	Comment 48 • 12 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 49 • 12 years ago

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.

	Joey Armstrong [:joey]
	Comment 50 • 12 years ago

(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.

	(not currently active) Ted Mielczarek
	Comment 51 • 12 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 52 • 12 years ago

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.

	Joey Armstrong [:joey]
	Comment 53 • 12 years ago

(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.

	Chris Cooper [:coop] (he/him)
	Comment 54 • 12 years ago

(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.

	Johnathan Nightingale [:johnath]
	Comment 55 • 12 years ago

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?

	Johnathan Nightingale [:johnath]
	Comment 56 • 12 years ago

(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?)

	Chris Cooper [:coop] (he/him)
	Comment 57 • 12 years ago

(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.

	Justin Wood (:Callek)
	Comment 58 • 12 years ago

This is part B, unbitrotted

	Justin Wood (:Callek)
	Comment 59 • 12 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 60 • 12 years ago

(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.

	Justin Wood (:Callek)
	Comment 61 • 12 years ago

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.

	Brian R. Bondy [:bbondy]
	Comment 62 • 12 years ago

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.

	Brian R. Bondy [:bbondy]
	Comment 63 • 12 years ago

Thanks for your work on it so far by the way.

	Joey Armstrong [:joey]
	Comment 64 • 12 years ago

A list of tasks for the metro build are logged here.  Feel free to add anything missing:  https://joey.etherpad.mozilla.org/metro-build

	Jim Mathies [:jimm] Reporter
	Comment 65 • 12 years ago

Adding a few of the build config bugs that have been filed.

	Mike Hommey [:glandium] Assignee
	Comment 66 • 12 years ago

This replaces Part B, rev 2.2.

This requires the patches from bug 787165, bug 785871, bug 780561, bug 787180, and bug 787184.

	Mike Hommey [:glandium] Assignee
	Comment 67 • 12 years ago

Forgot to hg add the additional files in previous patch.

	Jim Mathies [:jimm] Reporter
	Comment 68 • 12 years ago

(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?

	Mike Hommey [:glandium] Assignee
	Comment 69 • 12 years ago

There were some problems with the package manifest.

	Mike Hommey [:glandium] Assignee
	Comment 70 • 12 years ago

(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.

	Mike Hommey [:glandium] Assignee
	Comment 71 • 12 years ago

This, and the update to bug 780561, should make it build on windows.

	Mike Hommey [:glandium] Assignee
	Comment 72 • 12 years ago

(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).

	Mike Hommey [:glandium] Assignee
	Comment 73 • 12 years ago

(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.

	Mike Hommey [:glandium] Assignee
	Comment 74 • 12 years ago

Refreshed against latest patch from bug 780561.

	Jim Mathies [:jimm] Reporter
	Comment 75 • 12 years ago

(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.

	Jim Mathies [:jimm] Reporter
	Comment 76 • 12 years ago

Ah, I think it was supposed to be PROGRAMS_DEST with an 'S'.

	Mike Hommey [:glandium] Assignee
	Comment 77 • 12 years ago

(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)

	Brian R. Bondy [:bbondy]
	Comment 78 • 12 years ago

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.

	Brian R. Bondy [:bbondy]
	Comment 79 • 12 years ago

Comment 78 was in regards to elm by the way which only builds Windows currently.

	Mike Hommey [:glandium] Assignee
	Comment 80 • 12 years ago

I'd say it should be fine for now. We can do one-offs on try.

	Brian R. Bondy [:bbondy]
	Comment 81 • 12 years ago

OK cool, at least for Nightly builds we should test upgrades on all platforms before we push everything to m-c.

	Jim Mathies [:jimm] Reporter
	Comment 82 • 12 years ago

removing a couple dep bugs that block bug 789461

	Jim Mathies [:jimm] Reporter
	Comment 83 • 12 years ago

Moving new package manifest bugs down to the new-packager bug.

	Mike Hommey [:glandium] Assignee
	Comment 84 • 12 years ago

This is required for l10n.

	Mike Hommey [:glandium] Assignee
	Comment 85 • 12 years ago

Comment on attachment 690768 [details] [diff] [review]
Use DIST_SUBDIR for XPIs too

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

	Ed Morley [:emorley]
	Comment 86 • 12 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 87 • 12 years ago

Most of the additional code in nsBrowserApp.cpp is stolen from xulrunner/stub/nsXULStub.cpp.

	Mike Hommey [:glandium] Assignee
	Comment 90 • 12 years ago

With a typo fixed.

	Jim Mathies [:jimm] Reporter
	Comment 91 • 12 years ago

(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.

	Jim Mathies [:jimm] Reporter
	Comment 92 • 12 years ago

(Maybe a cleanup and re-org the browser manifest bug might be better in a follow up?)

	Mike Hommey [:glandium] Assignee
	Comment 93 • 12 years ago

(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.

	Mike Hommey [:glandium] Assignee
	Comment 94 • 12 years ago

With the memory leak of xreDirectory plugged.

	Jim Mathies [:jimm] Reporter
	Comment 95 • 12 years ago

(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

	Jim Mathies [:jimm] Reporter
	Comment 96 • 12 years ago

We also needed a new appdir directive for sessionstore tests added in bug 532150 -

https://hg.mozilla.org/projects/elm/rev/174b33cde180

	Mike Hommey [:glandium] Assignee
	Comment 97 • 11 years ago

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

	Ed Morley [:emorley]
	Comment 98 • 11 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 99 • 11 years ago

This added multiple XPCOM static ctor/dtor warnings.

	Mike Hommey [:glandium] Assignee
	Comment 100 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #99)
> This added multiple XPCOM static ctor/dtor warnings.

which ones?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 101 • 11 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 102 • 11 years ago

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)

	Mike Hommey [:glandium] Assignee
	Comment 103 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 104 • 11 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 105 • 11 years ago

Refreshed against current m-c

	Mike Hommey [:glandium] Assignee
	Comment 106 • 11 years ago

Added missing browser/chrome.manifest, necessary with the current packager code.

	Jim Mathies [:jimm] Reporter
	Comment 107 • 11 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 108 • 11 years ago

(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.

	Mike Hommey [:glandium] Assignee
	Comment 109 • 11 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 110 • 11 years ago

With XPI_ROOT_APPID

	Matt Brubeck (:mbrubeck)
	Comment 111 • 11 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 112 • 11 years ago

Note, there were changes to the packager manifest in the latest mc merge to elm.

	Mike Hommey [:glandium] Assignee
	Comment 114 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/784b9beebe90

	Jan Beich
	Comment 115 • 11 years ago

--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

	Mike Hommey [:glandium] Assignee
	Comment 116 • 11 years ago

(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.

	u279076
	Comment 117 • 11 years ago

> 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.

	Mike Kaply [:mkaply]
	Comment 118 • 11 years ago

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.

	u279076
	Comment 119 • 11 years ago

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.