Closed Bug 917310 Opened 6 years ago Closed 6 years ago

Apps install path is broken on b2g desktop for windows and mac

Categories

(Firefox Graveyard :: Web Apps, defect)

x86_64
Windows 7
defect
Not set

Tracking

(firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
Firefox 27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(2 files, 4 obsolete files)

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
Assignee: nobody → poirot.alex
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
Attachment #806008 - Flags: review?(fabrice)
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
Attachment #806008 - Flags: review?(fabrice)
Blocks: 915258
(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.
Attached patch Another ifdef layout (obsolete) — Splinter Review
Attachment #806008 - Attachment is obsolete: true
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.
Attachment #806082 - Flags: review?(fabrice)
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.
Attachment #806082 - Flags: review?(fabrice) → review-
Duplicate of this bug: 917484
Attached patch PatchSplinter Review
Attachment #806082 - Attachment is obsolete: true
Attachment #806350 - Flags: review?(fabrice)
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
Attachment #806350 - Flags: review?(fabrice) → review+
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.
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/9fc5d65192ae
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
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.
Attached patch Fixed ifdefsSplinter Review
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.
Attachment #806702 - Attachment is obsolete: true
Attachment #806719 - Flags: review?(fabrice)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #806758 - Flags: review?(fabrice)
Attachment #806758 - Flags: feedback?(poirot.alex)
(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.
Attachment #806719 - Flags: review?(fabrice) → review+
Attachment #806758 - Flags: review?(fabrice)
Attachment #806758 - Flags: feedback?(poirot.alex)
Attachment #806758 - Attachment is obsolete: true
Blocks: 917913
Double landed on m-c, so we can get this out faster:
https://hg.mozilla.org/mozilla-central/rev/e8bb95435a54
Fixed by comment 33
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 918611
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
Attachment #806719 - Flags: approval-mozilla-aurora?
Attachment #806719 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.