Closed Bug 993062 Opened 6 years ago Closed 6 years ago

Remove unused showRemoteTabs and showTabs methods

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: Margaret, Assigned: zliu.ur)

Details

(Whiteboard: [mentor=margaret][lang=java][good first bug])

Attachments

(1 file, 1 obsolete file)

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
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
(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
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 margaret.leibovic@gmail.com, and I will review your patch for you!
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
> margaret.leibovic@gmail.com, and I will review your patch for you!
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
> margaret.leibovic@gmail.com, and I will review your patch for you!
Assignee: nobody → zliu.ur
Status: NEW → ASSIGNED
(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!
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
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!
> 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!
(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!
(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 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 :)
Attachment #8405821 - Flags: review+ → review?(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.
Attachment #8405821 - Flags: review?(margaret.leibovic) → review+
updated from the previous patch, removed the unused methods and remove the trailing new line.
Attachment #8407069 - Flags: review?(margaret.leibovic)
Comment on attachment 8407069 [details] [diff] [review]
bug993062-remove-unused-methods-update1.patch

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

Awesome, thanks!
Attachment #8407069 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
Attachment #8405821 - Attachment is obsolete: true
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!
Keywords: checkin-needed
Whiteboard: [mentor=margaret][lang=java][good first bug] → [mentor=margaret][lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6f221ba93943
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=margaret][lang=java][good first bug][fixed-in-fx-team] → [mentor=margaret][lang=java][good first bug]
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.