Closed Bug 1350886 Opened 5 years ago Closed 10 months ago

Remove MOZ_ANDROID_ACTIVITY_STREAM build flag

Categories

(Firefox for Android Graveyard :: Activity Stream, enhancement, P3)

All
Android
enhancement

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: sebastian, Unassigned)

References

Details

(Whiteboard: [MobileAS])

Attachments

(2 files)

It's not used anymore. In fact it was never used during build. It only existed to hide early builds of the new Activity Stream UI while we were still working on it.

Bug 1348820 removed the last code paths that used the flag. I tried to remove the flag too - but this resulted in some errors on try:
https://treeherder.mozilla.org/logviewer.html#?job_id=86041724&repo=autoland

I couldn't see an obvious reason for that so I landed the patches in bug 1348820 without removing the flag.
What's weird though is that there appears to be a comma missing here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/generate_build_config.py#40
Hi Sebastian!
I have added the patch for the bug. Please review it and let me know if any changes need to be done
https://reviewboard.mozilla.org/r/143446/
Flags: needinfo?(s.kaspari)
Comment on attachment 8871937 [details]
Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag.

https://reviewboard.mozilla.org/r/143446/#review147496

Thank you for your patch! :) Looking at DXR this constant shows up in some more places:
https://dxr.mozilla.org/mozilla-central/search?q=MOZ_ANDROID_ACTIVITY_STREAM&redirect=false
Attachment #8871937 - Flags: review?(s.kaspari) → review-
Please check the new patch and let me know if more changes need to be done.
I have removed the code related to MOZ_ANDROID_ACTIVITY_STREAM build flag in the new patch. Please have a look
Assignee: nobody → owaiskazi19
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Comment on attachment 8871937 [details]
Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag.

https://reviewboard.mozilla.org/r/143446/#review147648

It looks like something went wrong with your patch: This one only contains the last changes where you removed the comments. Can you try to create one patch that contains all changes?

::: commit-message-44e41:1
(Diff revision 3)
> +Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag-2 r?sebastian

There's a "-2" at the end of the commit message.
Attachment #8871937 - Flags: review?(s.kaspari) → review-
Uploaded the new patch. Please have a look
Flags: needinfo?(s.kaspari)
The change looks good to me. But for some kind of reason it seems to break testANRReporter on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca318a243ee0&selectedJob=103439937
Flags: needinfo?(s.kaspari)
@gbrown: Have you seen something like this before? I don't even know where to start debugging this.
Flags: needinfo?(gbrown)
Comment on attachment 8871937 [details]
Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag.

https://reviewboard.mozilla.org/r/143446/#review148724

As said before: The code looks good, but r- until we figure out why this change breaks testANRReporter.
Attachment #8871937 - Flags: review?(s.kaspari) → review-
(In reply to Sebastian Kaspari (:sebastian) from comment #14)
> Comment on attachment 8871937 [details]
> Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag
> 
> https://reviewboard.mozilla.org/r/143446/#review148724
> 
> As said before: The code looks good, but r- until we figure out why this
> change breaks testANRReporter.

Sure! Will wait
(In reply to Sebastian Kaspari (:sebastian) from comment #13)
> @gbrown: Have you seen something like this before? I don't even know where
> to start debugging this.

I bet :jchen can figure this out!
Flags: needinfo?(gbrown) → needinfo?(nchen)
Ah if you look here, https://hg.mozilla.org/try/rev/cb972100df0783b96feb8beb5d4fe563f1a8efef#l2.12

> -    for var in ('MOZ_ANDROID_ACTIVITY_STREAM'
> -                'MOZ_ANDROID_ANR_REPORTER',

Without a trailing comma, the two literals are combined together to form 'MOZ_ANDROID_ACTIVITY_STREAMMOZ_ANDROID_ANR_REPORTER'. So neither `MOZ_ANDROID_ANR_REPORTER` nor `MOZ_ANDROID_ACTIVITY_STREAM` was actually usable. Without `MOZ_ANDROID_ANR_REPORTER` defined, I suspect testANRReporter has been a no-op for a while. This new patch actually re-enabled `MOZ_ANDROID_ANR_REPORTER` / testANRReporter, and something else in the meantime has caused the ANR reporter to no longer work.

We don't really use the ANR reporter these days so I think it's okay to disable it, by removing the `MOZ_ANDROID_ANR_REPORTER` lines in the various configure.sh files.
Flags: needinfo?(nchen)
Should I remove all code of MOZ_ANDROID_ANR_REPORTER also?
Flags: needinfo?(s.kaspari)
Yeah, let's do that and then let's push the new patch to the try server again.
Flags: needinfo?(s.kaspari)
I don't think you have to delete `ANRReporter` itself, just unsetting `MOZ_ANDROID_ANR_REPORTER` should be fine.
Comment on attachment 8871937 [details]
Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag.

https://reviewboard.mozilla.org/r/143446/#review160744

Sorry, this took a while to get to.

::: mobile/android/base/AppConstants.java.in
(Diff revision 5)
> -    public static final boolean MOZ_ANDROID_ANR_REPORTER =
> -//#ifdef MOZ_ANDROID_ANR_REPORTER
> -    true;
> -//#else
> -    false;
> -//#endif

Like Jim mentioned above. We can keep all this code for now. We only need to make sure MOZ_ANDROID_ANR_REPORTER is not set anymore.
Attachment #8871937 - Flags: review?(s.kaspari) → review-
All open Activity Stream bugs are moving from the whiteboard tag, "[mobileAS]", to the Firefox for Android component, "Activity Stream", so that I can keep better track of these bugs as the new triage owner; I will send out an email shortly with additional details, caveats, etc.
Component: General → Activity Stream
Unassigning: I don't think this is actively being worked on. Please let me know if that's not the case, Owais!
Assignee: owaiskazi19 → nobody
Status: ASSIGNED → NEW
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.