993062 - Remove unused showRemoteTabs and showTabs methods

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

This method is unused:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1291
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#530

Also, following this logic a bit more, it looks like we can remove the showTabs method declaration in GeckoApp, since it's only used in BrowserApp:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#532

Comment 1

[zliu.ur]

Hi:

I am new to Mozilla mobile and would like to work on this bug as a starter. Can you guide me through this?

Thanks in advance

Zack

Comment 2

[zliu.ur]

(In reply to :Margaret Leibovic from comment #0)
> This method is unused:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#1291
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.
> java#530
> 
> Also, following this logic a bit more, it looks like we can remove the
> showTabs method declaration in GeckoApp, since it's only used in BrowserApp:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.
> java#532

Comment 3

[:Margaret Leibovic]

Hi Zack!

First of all, do you have a build environment set up? If not, you'll want to follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

After that, you should remove the unused methods I pointed to in the bug description, re-build and test your local build to make sure you didn't accidentally break anything, then generate a patch for me to review. You can follow directions for that here:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

When you attach the patch to this bug you should set the review? flag to [e-mail], and I will review your patch for you!

Comment 4

[zliu.ur]

Hi Margaret:

I tried to build the source code, but I have a problem when the configure process reports an error saying program dx was not found. I dig it a little bit and it seems that the dx (with a bunch of other programs) was in $(SDK_DIR)/build_tools while the configure file is trying to find them in $(SDK_DIR)/tools directory. I am using the SDK version android-4.4.2

It seems to be reported as Bug 941889 marked as solved. Is there a patch I should update? Please advice

Thanks for your help

Regards

Zack


(In reply to :Margaret Leibovic from comment #3)
> Hi Zack!
> 
> First of all, do you have a build environment set up? If not, you'll want to
> follow the directions here:
> https://wiki.mozilla.org/Mobile/Fennec/Android
> 
> After that, you should remove the unused methods I pointed to in the bug
> description, re-build and test your local build to make sure you didn't
> accidentally break anything, then generate a patch for me to review. You can
> follow directions for that here:
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
> 
> When you attach the patch to this bug you should set the review? flag to
> [e-mail], and I will review your patch for you!

Comment 5

[zliu.ur]

Hi Margaret:

Update, I followed the modification in the patch found here Bug 941889 and add android-4.4.2 in the list. and it worked for me. But probably we need to open another bug and solve this one thing for all.

Regards

Zack.

(In reply to :Margaret Leibovic from comment #3)
> Hi Zack!
> 
> First of all, do you have a build environment set up? If not, you'll want to
> follow the directions here:
> https://wiki.mozilla.org/Mobile/Fennec/Android
> 
> After that, you should remove the unused methods I pointed to in the bug
> description, re-build and test your local build to make sure you didn't
> accidentally break anything, then generate a patch for me to review. You can
> follow directions for that here:
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
> 
> When you attach the patch to this bug you should set the review? flag to
> [e-mail], and I will review your patch for you!

Comment 6

[:Margaret Leibovic]

(In reply to zliu.ur from comment #5)
> Hi Margaret:
> 
> Update, I followed the modification in the patch found here Bug 941889 and
> add android-4.4.2 in the list. and it worked for me. But probably we need to
> open another bug and solve this one thing for all.
> 
> Regards
> 
> Zack.

Thanks for pointing this out. I dropped a link to this bug to nalexander yesterday, but I never followed up to see if he saw this. He does work to maintain our build system, so I want to make sure he knows about this.

So, did you get your build working after all that? Let me know if you need more help!

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #6)
> (In reply to zliu.ur from comment #5)
> > Hi Margaret:
> > 
> > Update, I followed the modification in the patch found here Bug 941889 and
> > add android-4.4.2 in the list. and it worked for me. But probably we need to
> > open another bug and solve this one thing for all.
> > 
> > Regards
> > 
> > Zack.
> 
> Thanks for pointing this out. I dropped a link to this bug to nalexander
> yesterday, but I never followed up to see if he saw this. He does work to
> maintain our build system, so I want to make sure he knows about this.

I saw it go by :)  Let me have a look and see if there's any way to extract the path from meta-data.

Comment 8

[zliu.ur]

Hi Margaret:

I get the build working. For this bug, I have a question:

The showRemoteTabs() is not used anywhere, so it should be removed. 
But the showTabs() is used in the other functions like
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#206
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1286

And these functions are used somewhere in the program. Should I just remove the showRemoteTabs() and generate the patch?

Thanks for your help

Regards

Zack


(In reply to :Margaret Leibovic from comment #6)
> (In reply to zliu.ur from comment #5)
> > Hi Margaret:
> > 
> > Update, I followed the modification in the patch found here Bug 941889 and
> > add android-4.4.2 in the list. and it worked for me. But probably we need to
> > open another bug and solve this one thing for all.
> > 
> > Regards
> > 
> > Zack.
> 
> Thanks for pointing this out. I dropped a link to this bug to nalexander
> yesterday, but I never followed up to see if he saw this. He does work to
> maintain our build system, so I want to make sure he knows about this.
> 
> So, did you get your build working after all that? Let me know if you need
> more help!

Comment 9

[Nick Alexander :nalexander [he/him]]

> I saw it go by :)  Let me have a look and see if there's any way to extract
> the path from meta-data.

I pushed a patch to Bug 960640 that will hopefully address this more fully.  Testing appreciated!

Comment 10

[:Margaret Leibovic]

(In reply to zliu.ur from comment #8)

> The showRemoteTabs() is not used anywhere, so it should be removed. 
> But the showTabs() is used in the other functions like
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#206
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#1286
> 
> And these functions are used somewhere in the program. Should I just remove
> the showRemoteTabs() and generate the patch?

Sorry, I should have been more clear about the showTabs method. In comment 0, I mentioned that we should just remove the showTabs declaration in GeckoApp, since it's only used in BrowserApp. So we're not removing the method entirely, just one declaration of it.

Once you do that, you should generate a patch and attach it to the bug. Thanks!

Comment 11

[zliu.ur]

Attachment: remove unused methods showRemoteTabs and declearation of showTabs

Comment 12

[zliu.ur]

(In reply to :Margaret Leibovic from comment #10)
> (In reply to zliu.ur from comment #8)
> 
> > The showRemoteTabs() is not used anywhere, so it should be removed. 
> > But the showTabs() is used in the other functions like
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> > java#206
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> > java#1286
> > 
> > And these functions are used somewhere in the program. Should I just remove
> > the showRemoteTabs() and generate the patch?
> 
> Sorry, I should have been more clear about the showTabs method. In comment
> 0, I mentioned that we should just remove the showTabs declaration in
> GeckoApp, since it's only used in BrowserApp. So we're not removing the
> method entirely, just one declaration of it.
> 
> Once you do that, you should generate a patch and attach it to the bug.
> Thanks!

Hi Margaret:

I generated the patch, can you review it for me?

Any update, please let me know

Best Regards

Zack

Comment 13

[:Margaret Leibovic]

Comment on attachment 8405821 [details] [diff] [review]
remove unused methods showRemoteTabs and declearation of showTabs

These bugzilla review flags always confuse newcomers, we should really add some more help text or something. When uploading a patch, you should use the review? flag and specify an appropriate reviewer. In this case, I will review your patch :)

Comment 14

[:Margaret Leibovic]

Comment on attachment 8405821 [details] [diff] [review]
remove unused methods showRemoteTabs and declearation of showTabs

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

Great, thanks! I just have one small comment to address. You should address this comment, then upload a new version of the patch that's formatted for checkin (with the bug number and reviewer in the commit message), then you can set the checkin-needed keyword on the bug.

::: mobile/android/base/BrowserApp.java
@@ +1285,5 @@
>      @Override
>      public void showPrivateTabs() {
>          showTabs(TabsPanel.Panel.PRIVATE_TABS);
>      }
> +    

Nit: Remove trailing whitespace.

Comment 15

[zliu.ur]

Attachment: bug993062-remove-unused-methods-update1.patch

updated from the previous patch, removed the unused methods and remove the trailing new line.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8407069 [details] [diff] [review]
bug993062-remove-unused-methods-update1.patch

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

Awesome, thanks!

Comment 17

[Carsten Book [:Tomcat]]

checked-in on fx-team as
https://hg.mozilla.org/integration/fx-team/rev/6f221ba93943

Zack, congrats to your first patch, welcome to mozilla and thanks for contributing!

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f221ba93943