Bug 755724 (metro-build)

Split platform and app resources up so that they can be loaded individually

RESOLVED FIXED in mozilla21

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jimm, Assigned: glandium)

Tracking

(Depends on: 2 bugs, {addon-compat, dev-doc-needed})

Trunk
mozilla21
x86_64
Windows 7
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 24+)

Details

(Whiteboard: [metro-preview][completed-elm][metro-mvp][LOE:3][metro-it2], URL)

Attachments

(6 attachments, 26 obsolete attachments)

2.76 KB, patch
ted
: review+
glandium
: checkin+
Details | Diff | Splinter Review
17.26 KB, patch
glandium
: review+
glandium
: checkin+
Details | Diff | Splinter Review
2.35 KB, patch
ted
: review+
glandium
: checkin+
Details | Diff | Splinter Review
12.08 KB, patch
bsmedberg
: review+
glandium
: checkin+
Details | Diff | Splinter Review
2.72 KB, patch
bsmedberg
: review+
glandium
: checkin+
Details | Diff | Splinter Review
26.14 KB, patch
jimm
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
Attachment #624368 - Flags: feedback?(benjamin)
(Reporter)

Comment 1

5 years ago
Created attachment 624468 [details] [diff] [review]
patch

A more complete patch with some additional changes to JarMaker and target_libs.mk.
Attachment #624368 - Attachment is obsolete: true
Attachment #624368 - Flags: feedback?(benjamin)
Attachment #624468 - Flags: review?(benjamin)
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)"
Attachment #624468 - Flags: review?(benjamin) → review-
(Reporter)

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

5 years ago
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.
Attachment #624468 - Attachment is obsolete: true
Attachment #624701 - Flags: review?(benjamin)
(Reporter)

Updated

5 years ago
Attachment #624701 - Attachment is patch: true
(Reporter)

Comment 7

5 years ago
Created attachment 624702 [details]
chrome.manifest
(Reporter)

Comment 8

5 years ago
Created attachment 624703 [details]
browser.manifest
(Reporter)

Comment 9

5 years ago
Created attachment 624704 [details]
components.manifest
(Reporter)

Comment 10

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

Comment 11

5 years ago
Created attachment 624705 [details] [diff] [review]
mc compat patch

The original was off elm, this applies to mc.
(Reporter)

Comment 12

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

Comment 14

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

Updated

5 years ago
Blocks: 737833, 750903
(Reporter)

Comment 15

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

Comment 17

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

Updated

5 years ago
Attachment #624701 - Attachment is obsolete: true
Attachment #624701 - Flags: review?(benjamin)
(Reporter)

Updated

5 years ago
Attachment #624705 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Assignee: jmathies → nobody
Summary: add the ability to apply an appid restriction to component manifests in chrome.manifest → Split platform and app resources up so that they can be loaded individually
(Reporter)

Comment 18

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

Comment 20

5 years ago
Created attachment 626947 [details] [diff] [review]
fennec xul migration to browser (diff off elm tip)
Attachment #624702 - Attachment is obsolete: true
Attachment #624703 - Attachment is obsolete: true
Attachment #624704 - Attachment is obsolete: true
(Reporter)

Comment 21

5 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.
Attachment #626947 - Attachment is obsolete: true
(Reporter)

Comment 22

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

Comment 23

5 years ago
Created attachment 626970 [details] [diff] [review]
base file copies
(Reporter)

Comment 24

5 years ago
Created attachment 626973 [details] [diff] [review]
base patch
(Reporter)

Comment 25

5 years ago
Comment on attachment 626973 [details] [diff] [review]
base patch

these changes are checked in on elm now.
Attachment #626973 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #626970 - Attachment is obsolete: true
Assignee: nobody → benjamin
Created attachment 629782 [details] [diff] [review]
Part A, rev. 1 - Add DIST_SUBDIR makefile variable [checked in]
Attachment #629782 - Flags: review?(ted.mielczarek)
Created attachment 629783 [details] [diff] [review]
Part B, rev. 1 - use DIST_SUBDIR in makefiles and launch using the subdir
Attachment #629783 - Flags: review?(mh+mozilla)
(Assignee)

Comment 28

5 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)
Attachment #629783 - Flags: review?(mh+mozilla) → review-
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.
(Assignee)

Comment 30

5 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.
Created attachment 629871 [details] [diff] [review]
Part C, rev. 1 - Move ScopedAppData into the XPCOM glue [checked in]

Oops, I did need that patch
Attachment #629871 - Flags: review?(mh+mozilla)
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.
(Assignee)

Comment 33

5 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 :(
(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
(Assignee)

Comment 35

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

Comment 36

5 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.
Attachment #629871 - Flags: review?(mh+mozilla) → review+
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.
FWIW you should ignore the "divergent rename" warnings, they're absolutely useless and don't represent anything to be worried about.
(Assignee)

Updated

5 years ago
Depends on: 762833
(Assignee)

Updated

5 years ago
Depends on: 762864
(Assignee)

Comment 39

5 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?
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.
Attachment #629782 - Flags: review?(ted.mielczarek) → review+
(Reporter)

Updated

5 years ago
Blocks: 762344
Part A: https://hg.mozilla.org/integration/mozilla-inbound/rev/d977dae53ff9
Part C: https://hg.mozilla.org/integration/mozilla-inbound/rev/efe2ccacf040
Whiteboard: leave open
(Assignee)

Comment 42

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

Updated

5 years ago
Whiteboard: leave open → [leave open]
https://hg.mozilla.org/mozilla-central/rev/efe2ccacf040
https://hg.mozilla.org/mozilla-central/rev/d977dae53ff9
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.
Attachment #635773 - Flags: review?(ted.mielczarek)
Attachment #629783 - Attachment is obsolete: true
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).
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.
Ah, ok - that clears things up. Thanks.
(Reporter)

Updated

5 years ago
No longer blocks: 762344
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.
Assignee: benjamin → joey
(Assignee)

Comment 49

5 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.
(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 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.
Attachment #635773 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 52

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

Updated

5 years ago
No longer blocks: 737833, 750903
(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.

Updated

5 years ago
Assignee: joey → nobody
(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.
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?
(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?)
(Reporter)

Updated

5 years ago
Attachment #629782 - Attachment description: Part A, rev. 1 - Add DIST_SUBDIR makefile variable → Part A, rev. 1 - Add DIST_SUBDIR makefile variable [checked in]
(Reporter)

Updated

5 years ago
Attachment #629871 - Attachment description: Part C, rev. 1 - Move ScopedAppData into the XPCOM glue → Part C, rev. 1 - Move ScopedAppData into the XPCOM glue [checked in]
(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.
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
Attachment #635773 - Attachment is obsolete: true
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.
Attachment #642303 - Attachment is obsolete: true
(Assignee)

Comment 60

5 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.
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.
Assignee: nobody → joey

Updated

5 years ago
Depends on: 776611
(Assignee)

Updated

5 years ago
Depends on: 780561
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.
Thanks for your work on it so far by the way.
A list of tasks for the metro build are logged here.  Feel free to add anything missing:  https://joey.etherpad.mozilla.org/metro-build

Updated

5 years ago
No longer depends on: 780561
See Also: → bug 780561
(Reporter)

Updated

5 years ago
(Reporter)

Comment 65

5 years ago
Adding a few of the build config bugs that have been filed.
Depends on: 780561, 775154, 783242, 783243

Updated

5 years ago
Assignee: joey → mh+mozilla

Updated

5 years ago
Alias: metro-build
(Assignee)

Updated

5 years ago
Depends on: 787180
(Assignee)

Updated

5 years ago
Depends on: 787184
(Assignee)

Comment 66

5 years ago
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.

Updated

5 years ago
Blocks: 779902
(Assignee)

Comment 67

5 years ago
Created attachment 657023 [details] [diff] [review]
Move browser application in a subdirectory (PoC)

Forgot to hg add the additional files in previous patch.
(Assignee)

Updated

5 years ago
Attachment #657000 - Attachment is obsolete: true
(Reporter)

Comment 68

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

Comment 69

5 years ago
Created attachment 657040 [details] [diff] [review]
Move browser application in a subdirectory (PoC)

There were some problems with the package manifest.
(Assignee)

Updated

5 years ago
Attachment #657023 - Attachment is obsolete: true
(Assignee)

Comment 70

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

Updated

5 years ago
Attachment #642355 - Attachment is obsolete: true
(Assignee)

Comment 71

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #657040 - Attachment is obsolete: true
(Assignee)

Comment 72

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

Updated

5 years ago
Depends on: 787443
(Assignee)

Updated

5 years ago
Depends on: 782890
(Assignee)

Comment 73

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

Updated

5 years ago
Depends on: 788463
(Assignee)

Comment 74

5 years ago
Created attachment 658552 [details] [diff] [review]
Move browser application in a subdirectory (PoC)

Refreshed against latest patch from bug 780561.
(Assignee)

Updated

5 years ago
Attachment #657065 - Attachment is obsolete: true
(Reporter)

Comment 75

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

Comment 76

5 years ago
Ah, I think it was supposed to be PROGRAMS_DEST with an 'S'.
(Assignee)

Comment 77

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

Updated

5 years ago
Depends on: 789335
(Reporter)

Updated

5 years ago
Depends on: 789344
(Reporter)

Updated

5 years ago
Depends on: 789360
(Reporter)

Updated

5 years ago
Depends on: 789461
(Reporter)

Updated

5 years ago
Depends on: 789469
(Reporter)

Updated

5 years ago
Depends on: 789529
(Reporter)

Updated

5 years ago
Depends on: 789582
(Reporter)

Updated

5 years ago
Whiteboard: [leave open] → [leave open][metro-preview]
(Assignee)

Updated

5 years ago
Depends on: 790115
(Reporter)

Updated

5 years ago
Depends on: 791694
(Assignee)

Updated

5 years ago
Depends on: 792050
(Assignee)

Updated

5 years ago
Depends on: 792347
(Reporter)

Updated

5 years ago
Depends on: 792580
(Assignee)

Updated

5 years ago
Depends on: 792685
(Reporter)

Updated

5 years ago
Whiteboard: [leave open][metro-preview] → [leave open][metro-preview][completed-elm]
(Reporter)

Updated

5 years ago
Depends on: 794076
Depends on: 793767
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 78 was in regards to elm by the way which only builds Windows currently.
(Assignee)

Comment 80

5 years ago
I'd say it should be fine for now. We can do one-offs on try.
OK cool, at least for Nightly builds we should test upgrades on all platforms before we push everything to m-c.
(Assignee)

Updated

5 years ago
Depends on: 798437
(Assignee)

Updated

5 years ago
Depends on: 802254
(Assignee)

Updated

5 years ago
Depends on: 802541
(Reporter)

Updated

5 years ago
Blocks: 807593
(Reporter)

Updated

5 years ago
(Reporter)

Updated

5 years ago
Depends on: 809001
(Reporter)

Updated

5 years ago
Depends on: 809140
(Reporter)

Updated

5 years ago
No longer depends on: 809140
(Reporter)

Updated

5 years ago
Blocks: 771271
(Reporter)

Updated

5 years ago
Depends on: 810307
(Reporter)

Updated

5 years ago
No longer depends on: 810307

Updated

5 years ago
Blocks: 810714
(Reporter)

Updated

5 years ago
Whiteboard: [leave open][metro-preview][completed-elm] → [leave open][metro-preview][completed-elm][metro-mvp][LOE:2]
(Reporter)

Updated

5 years ago
Whiteboard: [leave open][metro-preview][completed-elm][metro-mvp][LOE:2] → [leave open][metro-preview][completed-elm][metro-mvp][LOE:3]
(Reporter)

Comment 82

5 years ago
removing a couple dep bugs that block bug 789461
No longer depends on: 789469, 791694
(Assignee)

Updated

5 years ago
No longer depends on: 762833
(Reporter)

Updated

5 years ago
No longer blocks: 771271, 807593
(Reporter)

Updated

5 years ago
No longer depends on: 794076
(Reporter)

Updated

5 years ago
Depends on: 817602
(Reporter)

Updated

5 years ago
Depends on: 817881
(Reporter)

Updated

5 years ago
Depends on: 817879
(Reporter)

Comment 83

5 years ago
Moving new package manifest bugs down to the new-packager bug.
No longer depends on: 789529, 789582, 792580
(Reporter)

Updated

5 years ago
Whiteboard: [leave open][metro-preview][completed-elm][metro-mvp][LOE:3] → [leave open][metro-preview][completed-elm][metro-mvp][LOE:3][metro-it2]
(Reporter)

Updated

5 years ago
No longer depends on: 789461
(Reporter)

Updated

5 years ago
No longer depends on: 792347
(Assignee)

Updated

5 years ago
Depends on: 820053
(Reporter)

Updated

5 years ago
Depends on: 820200
(Reporter)

Updated

5 years ago
Depends on: 820201
(Reporter)

Updated

5 years ago
Depends on: 820205
(Assignee)

Comment 84

5 years ago
Created attachment 690768 [details] [diff] [review]
Use DIST_SUBDIR for XPIs too

This is required for l10n.
Attachment #690768 - Flags: review?(ted)
(Reporter)

Updated

5 years ago
Depends on: 820232
Attachment #690768 - Flags: review?(ted) → review+
(Assignee)

Comment 85

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfbea498e32a
Attachment #690768 - Flags: checkin+
(Assignee)

Updated

5 years ago
Depends on: 820724
https://hg.mozilla.org/mozilla-central/rev/cfbea498e32a
Depends on: 821038
(Assignee)

Updated

5 years ago
Depends on: 789461
(Assignee)

Updated

5 years ago
Depends on: 821182
(Reporter)

Updated

5 years ago
Depends on: 821327
(Reporter)

Updated

5 years ago
Depends on: 822004
(Assignee)

Updated

5 years ago
Depends on: 822097
(Assignee)

Comment 87

5 years ago
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.
Attachment #696013 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #658552 - Attachment is obsolete: true
(Assignee)

Comment 88

5 years ago
Created attachment 696014 [details] [diff] [review]
Move browser application in a subdirectory
Attachment #696014 - Flags: review?(jmathies)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 817602
(Assignee)

Comment 90

5 years ago
Created attachment 696018 [details] [diff] [review]
Don't use the xulrunner stub when building Firefox against a libxul SDK

With a typo fixed.
Attachment #696018 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #696013 - Attachment is obsolete: true
Attachment #696013 - Flags: review?(benjamin)
(Reporter)

Comment 91

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

Comment 92

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

Comment 93

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

Comment 94

5 years ago
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.
Attachment #696046 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #696018 - Attachment is obsolete: true
Attachment #696018 - Flags: review?(benjamin)
(Reporter)

Updated

5 years ago
Attachment #696014 - Flags: review?(jmathies) → review+
(Reporter)

Comment 95

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

Comment 96

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

https://hg.mozilla.org/projects/elm/rev/174b33cde180
Depends on: 825872
(Assignee)

Updated

5 years ago
No longer depends on: 825872
Attachment #696046 - Flags: review?(benjamin) → review+
(Assignee)

Comment 97

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f438f50b8bb3
https://hg.mozilla.org/mozilla-central/rev/f438f50b8bb3
(Reporter)

Updated

5 years ago
Depends on: 826565
This added multiple XPCOM static ctor/dtor warnings.
(Assignee)

Comment 100

5 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@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
(Assignee)

Comment 102

5 years ago
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)
Attachment #703838 - Flags: review?(benjamin)
Attachment #703838 - Flags: review?(benjamin) → review+
(Assignee)

Comment 103

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab31d2237244
https://hg.mozilla.org/mozilla-central/rev/ab31d2237244
(Reporter)

Updated

5 years ago
Depends on: 834018
(Reporter)

Updated

5 years ago
Depends on: 834101
(Reporter)

Updated

5 years ago
Depends on: 834104
(Assignee)

Updated

5 years ago
Attachment #629871 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #629782 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696046 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #703838 - Flags: checkin+
(Assignee)

Comment 105

5 years ago
Created attachment 706487 [details] [diff] [review]
Move browser application in a subdirectory

Refreshed against current m-c
(Assignee)

Updated

5 years ago
Attachment #696014 - Attachment is obsolete: true
(Assignee)

Comment 106

5 years ago
Created attachment 706557 [details] [diff] [review]
Move browser application in a subdirectory

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

Updated

5 years ago
Attachment #706487 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Depends on: 834873
(Reporter)

Comment 107

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

Comment 108

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

Comment 109

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #706557 - Attachment is obsolete: true
(Assignee)

Comment 110

4 years ago
Created attachment 710614 [details] [diff] [review]
Move browser application in a subdirectory

With XPI_ROOT_APPID
(Assignee)

Updated

4 years ago
Attachment #710608 - Attachment is obsolete: true
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.
Keywords: addon-compat
(Reporter)

Comment 112

4 years ago
Note, there were changes to the packager manifest in the latest mc merge to elm.
(Reporter)

Updated

4 years ago
Depends on: 839793
(Assignee)

Comment 113

4 years ago
Created attachment 712403 [details] [diff] [review]
Move browser application in a subdirectory
Attachment #712403 - Flags: review?(jmathies)
(Assignee)

Updated

4 years ago
Attachment #710614 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #712403 - Flags: review?(jmathies) → review+
(Assignee)

Comment 114

4 years ago
https://hg.mozilla.org/mozilla-central/rev/784b9beebe90
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Comment 115

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

Comment 116

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

Updated

4 years ago
Blocks: 840146
(Reporter)

Updated

4 years ago
Whiteboard: [leave open][metro-preview][completed-elm][metro-mvp][LOE:3][metro-it2] → [metro-preview][completed-elm][metro-mvp][LOE:3][metro-it2]

Updated

4 years ago
Depends on: 840555

Updated

4 years ago
Blocks: 840598

Updated

4 years ago
Depends on: 840734
Depends on: 841094
> 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.
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.
(Reporter)

Updated

4 years ago
Depends on: 841011
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.

Updated

4 years ago
Depends on: 841456
(Reporter)

Updated

4 years ago
No longer depends on: 841456
(Assignee)

Updated

4 years ago
Depends on: 840094

Updated

4 years ago
Depends on: 842334
(Assignee)

Updated

4 years ago
Depends on: 844128
Depends on: 844553
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
relnote-firefox: --- → ?

Comment 121

4 years ago
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. :-)
Keywords: dev-doc-needed

Updated

4 years ago
Depends on: 872047
(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.
relnote-firefox: ? → 24+
bajaj, I'm not sure there's anything left to relnote, since that change mostly got backed out in bug 842334.
(Reporter)

Updated

4 years ago
Depends on: 870529
(Assignee)

Updated

4 years ago
Depends on: 933803
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.
(Assignee)

Comment 125

3 years ago
(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)
(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?
(Assignee)

Comment 127

3 years ago
(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.
You need to log in before you can comment on or make changes to this bug.