1350886 - Remove MOZ_ANDROID_ACTIVITY_STREAM build flag

Status: RESOLVED INCOMPLETE

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 2

[Owais Kazi]

Attachment: Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag.

Review commit: https://reviewboard.mozilla.org/r/143446/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143446/

Comment 3

[Owais Kazi]

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/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 5

[Owais Kazi]

Comment on attachment 8871937 [details]
Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143446/diff/1-2/

Comment 6

[Owais Kazi]

Please check the new patch and let me know if more changes need to be done.

Comment 7

[Owais Kazi]

Comment on attachment 8871937 [details]
Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143446/diff/2-3/

Comment 8

[Owais Kazi]

I have removed the code related to MOZ_ANDROID_ACTIVITY_STREAM build flag in the new patch. Please have a look

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 10

[Owais Kazi]

Comment on attachment 8871937 [details]
Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143446/diff/3-4/

Comment 11

[Owais Kazi]

Uploaded the new patch. Please have a look

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

@gbrown: Have you seen something like this before? I don't even know where to start debugging this.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 15

[Owais Kazi]

(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

Comment 16

[Geoff Brown [:gbrown]]

(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!

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 18

[Owais Kazi]

Should I remove all code of MOZ_ANDROID_ANR_REPORTER also?

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, let's do that and then let's push the new patch to the try server again.

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

I don't think you have to delete `ANRReporter` itself, just unsetting `MOZ_ANDROID_ANR_REPORTER` should be fine.

Comment 21

[Owais Kazi]

Attachment: Name added to author list

Review commit: https://reviewboard.mozilla.org/r/150084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150084/

Comment 22

[Owais Kazi]

Comment on attachment 8871937 [details]
Bug 1350886 - Removed MOZ_ANDROID_ACTIVITY_STREAM build flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143446/diff/4-5/

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Unassigning: I don't think this is actively being worked on. Please let me know if that's not the case, Owais!

Comment 27

[BMO Automation]

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.