If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove MOZ_ANDROID_ACTIVITY_STREAM build flag

NEW
Unassigned

Status

()

Firefox for Android
Activity Stream
P3
normal
7 months ago
22 days ago

People

(Reporter: sebastian, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MobileAS])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

7 months 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

7 months 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

5 months 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

5 months 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

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

Comment 8

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

Updated

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

Comment 9

5 months 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

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

Comment 12

5 months 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

5 months 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

5 months 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

5 months 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

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

Comment 19

5 months 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

3 months 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
Duplicate of this bug: 1333606
You need to log in before you can comment on or make changes to this bug.