Remove MOZ_ANDROID_TAB_QUEUE build flag

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: swaroop.rao, Mentored)

Tracking

35 Branch
Firefox 46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We shipped this feature, and I don't think we plan to ever turn it off. Let's get rid of this flag.
Mentor: s.kaspari
Note: we'll still need to keep TabQueueHelper.TAB_QUEUE_ENABLED around, since we use that to disable this feature on Gingerbread.
Summary: Remove TAB_QUEUE_ENABLED build flag → Remove MOZ_ANDROID_TAB_QUEUE build flag
I can do this. I am newcomer and want to start helping on this project.
(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
Yes, I have it setup, and with Android studio.
(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.
(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. :)
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.
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
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.
(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.
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).
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.
Attachment #8699119 - Flags: review?(s.kaspari)
Attachment #8699119 - Flags: review?(nalexander)
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.
Attachment #8699119 - Flags: review?(s.kaspari) → feedback+
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.
Flags: needinfo?(s.kaspari)
(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.
Flags: needinfo?(s.kaspari)
Assignee: nobody → swaroop.rao
Status: NEW → ASSIGNED
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.
Attachment #8699119 - Attachment is obsolete: true
Attachment #8699119 - Flags: review?(nalexander)
Attachment #8699513 - Flags: review?(s.kaspari)
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? :)
Attachment #8699513 - Flags: review?(s.kaspari) → review+
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.
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?
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.
Flags: needinfo?(s.kaspari)
(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. :)
Flags: needinfo?(s.kaspari)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f51c6ba540f4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
You need to log in before you can comment on or make changes to this bug.