1222607 - Remove MOZ_ANDROID_TAB_QUEUE build flag

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We shipped this feature, and I don't think we plan to ever turn it off. Let's get rid of this flag.

Comment 1

[:Margaret Leibovic]

Note: we'll still need to keep TabQueueHelper.TAB_QUEUE_ENABLED around, since we use that to disable this feature on Gingerbread.

Comment 2

[rchawla]

I can do this. I am newcomer and want to start helping on this project.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to rchawla from comment #2)
> I can do this. I am newcomer and want to start helping on this project.

Hi! :)

I saw you already have another bug assigned, so I assume you already have set up a build environment? https://wiki.mozilla.org/Mobile/Fennec/Android

Let me know if you need any help. Either here or on IRC (#mobile): https://wiki.mozilla.org/IRC

Comment 4

[rchawla]

Yes, I have it setup, and with Android studio.

Comment 5

[rchawla]

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> (In reply to rchawla from comment #2)
> > I can do this. I am newcomer and want to start helping on this project.
> 
> Hi! :)
> 
> I saw you already have another bug assigned, so I assume you already have
> set up a build environment? https://wiki.mozilla.org/Mobile/Fennec/Android
> 
> Let me know if you need any help. Either here or on IRC (#mobile):
> https://wiki.mozilla.org/IRC

Can I get assigned to this bug? I want to get familiar with the release process, and this is a simple task to try.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to rchawla from comment #5)
> Can I get assigned to this bug? I want to get familiar with the release
> process, and this is a simple task to try.

We generally assign after a patch has been uploaded. So, you can start at any time - this one is reserved for you. :)

Comment 7

[swaroop.rao]

I'd like to take this up but I need to know where I can find the MOZ_ANDROID_TAB_QUEUE flag. I tried doing a recursive 'find' from the base directory of the source code tree but couldn't figure out which file I should be making the changes in.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #7)
> I'd like to take this up but I need to know where I can find the
> MOZ_ANDROID_TAB_QUEUE flag. I tried doing a recursive 'find' from the base
> directory of the source code tree but couldn't figure out which file I
> should be making the changes in.

swaroop: `find` searches for file names; you'd want `grep` (or `awk`, or `ag`, or ...).  In general, use DXR for this type of search.  For example, https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=MOZ_ANDROID_TAB_QUEUE&redirect=true.

Comment 9

[swaroop.rao]

Nick: My bad! I should have been clear about the way I described what I did. In reality, what I did was a recursive find with a grep, as in:
find . -type f -exec grep -il MOZ_ANDROID_TAB_QUEUE {} \;

I will try DXR now.

Comment 10

[swaroop.rao]

There seem to be a couple of ways of doing this:
1. Remove line 97 in mobile/android/confvars.sh
https://dxr.mozilla.org/mozilla-central/source/mobile/android/confvars.sh#97

2. Remove the only usage of the flag in the code in the TabQueuHelper class.
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueHelper.java#34

There are a few other seemingly relevant places that I could make the change but I'm not sure about those:
1. https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#885
2. https://dxr.mozilla.org/mozilla-central/source/configure.in#4862 & https://dxr.mozilla.org/mozilla-central/source/configure.in#8578
3. https://dxr.mozilla.org/mozilla-central/source/mobile/android/confvars.sh#97

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #10)
> There seem to be a couple of ways of doing this:
> 1. Remove line 97 in mobile/android/confvars.sh
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/confvars.sh#97
> 
> 2. Remove the only usage of the flag in the code in the TabQueuHelper class.
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/tabqueue/TabQueueHelper.java#34
> 
> There are a few other seemingly relevant places that I could make the change
> but I'm not sure about those:
> 1.
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.
> build#885
> 2. https://dxr.mozilla.org/mozilla-central/source/configure.in#4862 &
> https://dxr.mozilla.org/mozilla-central/source/configure.in#8578
> 3.
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/confvars.sh#97

The point is to remove every reference and use the fact that the define is always true to update the logic.  So you need to remove the option from configure.in, and the setting in confvars.sh, and then make the logic unconditional in moz.build, etc.

Comment 12

[Nick Alexander :nalexander [he/him]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1220892 was a similar ticket -- observe that Part 1 removed the logic, and then did a bunch of clean-up.  I assume the clean-up here will be small, since the flag is true (not false).

Comment 13

[swaroop.rao]

Attachment: Patch containing all changes to remove usage of MOZ_ANDROID_TAB_QUEUE build flag

I have made changes to remove all references to the MOZ_ANDROID_TAB_QUEUE build flag in the project. In cases where there was conditional code within a #ifndef or #ifdef, I removed code that was within an #ifndef block and retained code that was within an #ifdef block because MOZ_ANDROID_TAB_QUEUE is basically always true.

I did a complete build from scratch using ./mach clobber followed by ./mach bootstrap and ./mach build. All of them completed without errors. I did another find/grep in the whole folder structure to see if there were any occurrences of the MOZ_ANDROID_TAB_QUEUE build flag. I didn't find any except for some files within the .hg folder structure (some *.i files and the patch file itself). I didn't actually do a ./mach package and try installing it on my phone. Let me know if I need to do that before submitting the patch.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8699119 [details] [diff] [review]
Patch containing all changes to remove usage of MOZ_ANDROID_TAB_QUEUE build flag

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

This is looking good; except for the small error below.

(In reply to swaroop.rao from comment #13)
> I didn't actually do a ./mach package and try
> installing it on my phone. Let me know if I need to do that before
> submitting the patch.

You could do a smoke test on a device and just see whether the tab queue feature is still functional. :)

::: mobile/android/base/AndroidManifest.xml.in
@@ -65,5 @@
> -#ifdef MOZ_ANDROID_TAB_QUEUE
> -    <!-- Tab Queue -->
> -    <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/>
> -#endif
> -

This should be the other way around: We need to keep this permission.

Comment 15

[swaroop.rao]

Yes, that was an error on my part. I didn't notice that it was #ifdef. I have now corrected the oversight, created a build and installed on my phone (Nexus 6).

However, I don't see any way to enable the feature. I tried following the instructions at https://support.mozilla.org/en-US/kb/open-links-background-later-viewing-firefox-android#w_enable-tab-queuing. I went to Settings, but I don't see any option called Customize. If somebody can help me out with instructions to do a smoke test, I will do that before trying to upload the patch file.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to swaroop.rao from comment #15)
> However, I don't see any way to enable the feature. I tried following the
> instructions at
> https://support.mozilla.org/en-US/kb/open-links-background-later-viewing-
> firefox-android#w_enable-tab-queuing. I went to Settings, but I don't see
> any option called Customize. If somebody can help me out with instructions
> to do a smoke test, I will do that before trying to upload the patch file.

Yeah, we recently reorganized the settings structure. You can now find it here: General -> Open multiple links.

Alternatively we should show you a prompt to enable the feature after opening four links from external apps.

Comment 17

[swaroop.rao]

Attachment: Modified patch containing changes to remove usage of MOZ_ANDROID_TAB_QUEUE build flag

I have created a new patch after fixing the error that was reported and also done a quick smoke test in my phone. The tab queue feature worked as expected.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8699513 [details] [diff] [review]
Modified patch containing changes to remove usage of MOZ_ANDROID_TAB_QUEUE build flag

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

Great patch. Thank you!

It looks like you already have commit access level 1 to push this patch to try? :)

Comment 19

[swaroop.rao]

Yes, I do but my SSH key is not valid anymore, so the push to the try server is failing. I have generated a new keypair but I don't know how to create a request to Server Operations to update my public key. I tried creating a bug in the mozilla.org project but I didn't see a 'Server Operations' component in the drop-down list.

Comment 20

[swaroop.rao]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff4f7aab66ba

Comment 21

[swaroop.rao]

Results of try job:
https://treeherder.mozilla.org/logviewer.html#?job_id=14751168&repo=try

Is there anything else needed from me for this bug?

Comment 22

[swaroop.rao]

I don't see a way to set the 'checkin-needed' flag for this bug, so I'll wait for further instructions on next steps.

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6a83d09046f

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to swaroop.rao from comment #20)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff4f7aab66ba

Your try run did not cover Gingerbread/2.3 (build & tests). I just did another push for you:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6a83d09046f

(In reply to swaroop.rao from comment #22)
> I don't see a way to set the 'checkin-needed' flag for this bug, so I'll
> wait for further instructions on next steps.

If the second try run is all good then you can add checkin-needed to the "keyword" field of the bug. Bugzilla should auto-complete for this field. :)

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f51c6ba540f4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f51c6ba540f4