Closed Bug 921637 Opened 6 years ago Closed 6 years ago

Activities have stopped working in firefox nightly

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

(Whiteboard: [c= p=3 s= u=])

Attachments

(2 files, 3 obsolete files)

As with 910975 and 893188 before it, activities have stopped working. Need to spend the time on a real implementation eventually.
Note activities appear to work somewhat better in production firefox 24.0.
Whiteboard: [c= p=3 s= u=]
Attached file Gaia Patch
Attached patch Gecko Patch (obsolete) — Splinter Review
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Comment on attachment 815631 [details] [diff] [review]
Gecko Patch

Hi Fabrice, could you give this a review? This is essentially just moving some code around to expose activities to the browser. Activities are still disabled to content as the dom.sysmsg.enabled is false by default.
Attachment #815631 - Flags: review?(fabrice)
Comment on attachment 815630 [details]
Gaia Patch

Hi Fabrice, after you review the gecko patch if you could give your R+ for the gaia part as well it would be appreciated. We're just deleting the extension as we can leverage the code from the platform. Thanks!
Attachment #815630 - Flags: review?(fabrice)
Comment on attachment 815631 [details] [diff] [review]
Gecko Patch

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

You can't move a b2g specific component under dom/, you need to provide one for each product that you want to support. So here you want one in browser/, and you will need r+ from a browser peer.
Attachment #815631 - Flags: review?(fabrice) → review-
Attached patch Gecko Patch (obsolete) — Splinter Review
Attachment #815631 - Attachment is obsolete: true
Attached patch Gecko Patch (obsolete) — Splinter Review
Attachment #815972 - Attachment is obsolete: true
Gecko patch has been greatly simplified. Decided to keep the current implementation of ActivitiesGlue inside the b2g folder, and leave the one in the gaia addon alone. In the future I would like to consolidate these somehow.
Comment on attachment 815978 [details] [diff] [review]
Gecko Patch

Hi Gavin, Dolske,

I am looking for a peer/owner to review this code and am not sure who is best suited to do so. If you could either review this or pass it along it would be appreciated.

Basically I would like to start including bits of MozActivities to help FirefoxOS workflow, and also potentially have native browser activities some day.

Thanks!
Attachment #815978 - Flags: review?(gavin.sharp)
Attachment #815978 - Flags: review?(dolske)
Given that this is a web-exposed DOM feature, as I understand it, a Gecko DOM peer should sign off on the package-manifest change.

(We really should fix bug 526333...)
Comment on attachment 815978 [details] [diff] [review]
Gecko Patch

(and you probably want to add it to Android's manifest too, at that point)
Attachment #815978 - Flags: review?(gavin.sharp)
Attachment #815978 - Flags: review?(dolske)
Comment on attachment 815978 [details] [diff] [review]
Gecko Patch

Hi Boris, Ben,

I'm looking for a DOM peer to review this if possible. Wondering if either of you could take a look, or redirect to the appropriate reviewer.

Thanks!
Attachment #815978 - Flags: review?(bzbarsky)
Attachment #815978 - Flags: review?(bent.mozilla)
Comment on attachment 815978 [details] [diff] [review]
Gecko Patch

I think Jonas is going to have to decide here.

Activities don't work on FF-desktop so this change seems wrong. However, the goal is to make activities work in FF-desktop-running-Gaia, something Gaia devs like to do a lot, so there is value to having this available (we will need to make sure it is hidden from normal web content and such). And then (as far as I understood) we are going to try to move Gaia folks off of this development model since it is so different from the builds we actually ship, so we may not want to do this at all.
Attachment #815978 - Flags: review?(jonas)
Attachment #815978 - Flags: review?(bzbarsky)
Attachment #815978 - Flags: review?(bent.mozilla)
Thanks for your comments bent. I was under the assumption that we would want activities in Firefox some day, and that this would be a necessary change to have them. Everything should be guarded by the dom.sysmsg.enabled preference for now.

Gaia developers have no decent alternative besides Firefox nightly yet, and I think it's going to be a while before we do. I've gotten quite a few pings to have this fixed, so if we can't land this soon then we'll have to keep maintaining our hacky version inside of gaia.
Comment on attachment 815978 [details] [diff] [review]
Gecko Patch

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

Please put this under a build flag so that it's easy to remove once we have a proper development environment that doesn't rely on the gaia-in-firefox solution?

r=me with that.
Attachment #815978 - Flags: review?(jonas) → review+
Target Milestone: --- → 1.2 C3(Oct25)
Attached patch Gecko patchSplinter Review
Attachment #815978 - Attachment is obsolete: true
Comment on attachment 817400 [details] [diff] [review]
Gecko patch

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #16)
> Comment on attachment 815978 [details] [diff] [review]
> Please put this under a build flag so that it's easy to remove once we have
> a proper development environment that doesn't rely on the gaia-in-firefox
> solution?

Hi Jonas, I've added an ifdef here. I just wanted to confirm this change with you (we default the ifdef to true but can disable easily). Marking you as reviewer again, thanks!
Attachment #817400 - Flags: review?(jonas)
Comment on attachment 817400 [details] [diff] [review]
Gecko patch

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

Sorry for taking so long with this. We're looking at another solution which I hope will be better than doing this. But for now this seems ok.
Attachment #817400 - Flags: review?(jonas) → review+
Comment on attachment 815630 [details]
Gaia Patch

Vivien - could you review this patch if you have the time? We can essentially bundle most of the activity code within Firefox, and only ship the glue code in gaia. Reducing the duplicate code will be a big win. Thanks!
Attachment #815630 - Flags: review?(21)
*Come back, activity!*
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #19)
> Comment on attachment 817400 [details] [diff] [review]
> Gecko patch
> 
> Review of attachment 817400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for taking so long with this. We're looking at another solution which
> I hope will be better than doing this. But for now this seems ok.

I hope this patch really works, because internally activities depend on system messages and thus are governed by the MOZ_SYSMSG config variable.
(In reply to Fabrice Desré [:fabrice] from comment #22)
> (In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from
> comment #19)
> I hope this patch really works, because internally activities depend on
> system messages and thus are governed by the MOZ_SYSMSG config variable.

I couldn't find that config variable, AFAIK they are always shipped and enabled/disabled by the dom.sysmsg.enabled preference. If it's disbaled (which it is by default), it simply shouldn't work. Do you think that could be bad?
oh yep, I forgot you removed MOZ_SYS_MSG. So I guess you turn on the pref in your add-on and provide the glue there?
(In reply to Fabrice Desré [:fabrice] from comment #24)
> oh yep, I forgot you removed MOZ_SYS_MSG. So I guess you turn on the pref in
> your add-on and provide the glue there?

Yes, we currently ship the glue in an add-on, and the gaia build sets the preference.
Comment on attachment 815630 [details]
Gaia Patch

Best patch ever.
Attachment #815630 - Flags: review?(21) → review+
Attachment #815630 - Flags: review?(fabrice)
Adding checkin-needed for gecko patch.
Keywords: checkin-needed
Setting [leave open] so the bug isn't resolved when this hits m-c. Please make sure to resolve it when the Gaia patch is landed.

https://hg.mozilla.org/integration/b2g-inbound/rev/59b57fd2af61
Keywords: checkin-needed
Whiteboard: [c= p=3 s= u=] → [c= p=3 s= u=][leave open]
Whiteboard: [c= p=3 s= u=][leave open] → [c= p=3 s= u=]
Target Milestone: 1.2 C3(Oct25) → ---
Going to land gaia patch now: https://github.com/mozilla-b2g/gaia/commit/4cb64a1f97efe4865a4f8712c1e86cd0279fcaf7

This appears to work when I build locally, but not for the normal nightly channel. It's probably just a matter of enabling MOZ_ACTIVITIES for FF nightly. I'm not entirely sure how to do this though, so if anyone does, please point me to some example code.

Will be opening a new bug to track this to not cause confusion here.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 937745
You need to log in before you can comment on or make changes to this bug.