Gaia doesn't launch on b2g desktop for mac and windows due to a regression introduced in bug 905881. We should move the #ifdef B2G first here: https://hg.mozilla.org/mozilla-central/rev/e6a9da6e6a8c#l6.117

Comment on attachment 806008 [details] [diff] [review] Fix app install path for b2g desktop on Windows and Mac r=fabrice https://tbpl.mozilla.org/?tree=Try&rev=31069bf7346e

Comment on attachment 806008 [details] [diff] [review] Fix app install path for b2g desktop on Windows and Mac r=fabrice Review of attachment 806008 [details] [diff] [review]: ----------------------------------------------------------------- Actually, I think we need to make it even clearer what depends on the application and what depends on the platform. So we could have: #ifdef MOZ_B2G // All b2g builds #ifdef MOZ_WIDGET_GONK #endif #elifdef MOZ_FENNEC // All fennec #elifdef MOZ_PHOENIX // Firefox #ifdef XP_WIN #elifdef XP_UNIX #elifdef XP_MACOSX #endif #elifdef MOZ_WEBAPP_RUNTIME // You guessed it... #else // Anything unsupported, like Metro ASSERT('!BOOM!') #endif

(In reply to Fabrice Desré [:fabrice] from comment #3) > Actually, I think we need to make it even clearer what depends on the > application and what depends on the platform. I think we should just have a definition for each platform, like: #ifdef MOZ_B2G #elifdef MOZ_FENNEC #elifdef XP_WIN #elifdef XP_MACOSX #elifdef XP_UNIX #endif After bug 910473 we'll have the B2G and Fennec part in their own directory. We shouldn't have a different code path for the Webapp Runtime and Firefox.

B2G is not a platform, it's a product, that runs on different platforms. Same for firefox desktop and the webapprt.

(In reply to Fabrice Desré [:fabrice] from comment #5) > B2G is not a platform, it's a product, that runs on different platforms. > Same for firefox desktop and the webapprt. B2G has always the same installation path on every platform, so there's no need to have #ifdef MOZ_WIDGET_GONK #endif Firefox desktop and the Webapp Runtime also have the same installation directory, there's no need to have different code paths for them.

(In reply to Marco Castelluccio [:marco] from comment #6) > (In reply to Fabrice Desré [:fabrice] from comment #5) > > B2G is not a platform, it's a product, that runs on different platforms. > > Same for firefox desktop and the webapprt. > > B2G has always the same installation path on every platform, so there's no > need to have > #ifdef MOZ_WIDGET_GONK > #endif > > Firefox desktop and the Webapp Runtime also have the same installation > directory, there's no need to have different code paths for them. You're missing the point here. I want the top level #ifdef to be products, not platforms. If a product behaves the same on all platforms, fine, no other nested #ifdefs. But keeping #ifdef MOZ_B2G at the same level as #ifdef XP_WIN is just wrong.

(In reply to Fabrice Desré [:fabrice] from comment #7) > You're missing the point here. I want the top level #ifdef to be products, > not platforms. If a product behaves the same on all platforms, fine, no > other nested #ifdefs. But keeping #ifdef MOZ_B2G at the same level as #ifdef > XP_WIN is just wrong. Oh, ok. I agree.

Comment on attachment 806082 [details] [diff] [review] Another ifdef layout It could have been simplier as webapps runtime and firefox are the same today, but #if defined(xxx) || defined(yyy) doesn't seem to work in js files.

I'd prefer the other ifdef layout with an explanatory comment: #ifdef MOZ_B2G #elifdef MOZ_FENNEC // Firefox and Webapp Runtime share the same installation path format #elifdef XP_WIN #elifdef XP_MACOSX #elifdef XP_UNIX

It's up to Fabrice to decide the ifdef layout, but anyway the patch won't work on Mac. On Mac both XP_MACOSX and XP_UNIX are defined, so you should put XP_MACOSX before XP_UNIX.

Comment on attachment 806350 [details] [diff] [review] Patch Review of attachment 806350 [details] [diff] [review]: ----------------------------------------------------------------- r=me Let's do that for now, and clean this up in Bug 910473

Could/should we have tests that would catch this in the future? This seems like a pretty bad regression to not catch on TBPL when bug 905881 landed.

I think the issue is that this didn't manifest on Linux, which is why TBPL didn't catch it. Is it possible to have a version of the gaia-ui-tests job on TBPL run on OS X?

(In reply to Bob Silverberg [:bsilverberg] from comment #19) > I think the issue is that this didn't manifest on Linux, which is why TBPL > didn't catch it. Is it possible to have a version of the gaia-ui-tests job > on TBPL run on OS X? This is also why I didn't catch the problem in my local testing. I tested the patch in Mac, Windows, Linux, Android, B2G on Linux, but I didn't test in B2G on other platforms. I'm really sorry for this!

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18) > https://hg.mozilla.org/mozilla-central/rev/9fc5d65192ae Seems like this patch broke the homescreen for me on the device. I can see this error with logcat: E/GeckoConsole( 689): [JavaScript Error: "[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXULAppInfo.ID]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: resource://gre/modules/WebappOSUtils.jsm :: <TOP_LEVEL> :: line 83" data: no]" {file: "resource://gre/modules/WebappOSUtils.jsm" line: 83}] I wonder if this is related to the fact that the homescreen is runned into a child process.

This patch actually work. Verified by running gaia on device, and running dom/apps,toolkit/devtools/apps tests on firefox. I haven't been able to run webappsrt tests, and I don't have a b2g desktop build ready.

MOZ_PHOENIX is defined for the Webapp Runtime, so the |#elifdef MOZ_WEBAPP_RUNTIME| isn't needed.

MOZ_PHOENIX is also defined for Metro, so we wouldn't throw on Metro.

(In reply to Marco Castelluccio [:marco] from comment #25) > MOZ_PHOENIX is defined for the Webapp Runtime, so the |#elifdef > MOZ_WEBAPP_RUNTIME| isn't needed. Perfect is the enemy of good.

(In reply to Marco Castelluccio [:marco] from comment #26) > MOZ_PHOENIX is also defined for Metro, so we wouldn't throw on Metro. About this, you could do: #elifdef MOZ_METRO ... #elifdef MOZ_PHOENIX ... #endif I agree the other problem is a nit that you could avoid fixing.

I tested on device, desktop-linux and deskop-mac with Alex's patch. They all ran fine. We need to unblock b2g so I'm just gonna land that.

This is exactly like ochameau's patch, the only difference is #ifdef MOZ_METRO to exclude Metro. Could someone test this on device? Testing the patch on b2g desktop and on the try server isn't enough, I didn't know that and I'm really sorry for the breakage.

(In reply to Fabrice Desré [:fabrice] from comment #29) > I tested on device, desktop-linux and deskop-mac with Alex's patch. They all > ran fine. We need to unblock b2g so I'm just gonna land that. I'm ok with this.

Double landed on m-c, so we can get this out faster: https://hg.mozilla.org/mozilla-central/rev/e8bb95435a54

Fixed by comment 33

Comment on attachment 806719 [details] [diff] [review] Fixed ifdefs [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 905881 reached ff26 branch, whereas this regression fix didn't. Nor the other regression in bug 918611. User impact if declined: Testing completed (on m-c, etc.): baked on m-c and regression have been fixed. Risk to taking this patch (and alternatives if risky): gaia won't start on b2g desktop 1.2. String or IDL/UUID changes made by this patch: none If we approve uplift here, we should uplift the two revisions being pushed in this bug: https://hg.mozilla.org/mozilla-central/rev/9fc5d65192ae https://hg.mozilla.org/mozilla-central/rev/6933a2fda563 and also the changeset from bug 918611: https://hg.mozilla.org/mozilla-central/rev/d5de24a40c49

Pushed as a single roll-up patch. https://hg.mozilla.org/releases/mozilla-aurora/rev/a98fdbb5d68d