Remove MOZ_ANDROID_ACTIVITY_STREAM build flag

NEW
Unassigned

Status

()

P3
normal
2 years ago
2 months ago

People

(Reporter: sebastian, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MobileAS])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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 hidden (mozreview-request)

Comment 3

2 years ago
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)
(Reporter)

Comment 4

2 years ago
mozreview-review
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-
Comment hidden (mozreview-request)

Comment 6

2 years ago
Please check the new patch and let me know if more changes need to be done.
Comment hidden (mozreview-request)

Comment 8

2 years ago
I have removed the code related to MOZ_ANDROID_ACTIVITY_STREAM build flag in the new patch. Please have a look
(Reporter)

Updated

2 years ago
Assignee: nobody → owaiskazi19
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 9

2 years ago
mozreview-review
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-
Comment hidden (mozreview-request)

Comment 11

2 years ago
Uploaded the new patch. Please have a look
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 12

2 years ago
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)
(Reporter)

Comment 13

2 years ago
@gbrown: Have you seen something like this before? I don't even know where to start debugging this.
Flags: needinfo?(gbrown)
(Reporter)

Comment 14

2 years ago
mozreview-review
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-

Comment 15

2 years ago
(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)

Comment 18

2 years ago
Should I remove all code of MOZ_ANDROID_ANR_REPORTER also?
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 19

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 23

a year ago
mozreview-review
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
You need to log in before you can comment on or make changes to this bug.