1330009 - Remove BrowserToolbar.setToolBarButtonsAlpha()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

BrowserToolbar.setToolBarButtonsAlpha() is unused and can be removed:

https://dxr.mozilla.org/mozilla-central/rev/7011ed1427de2b6f075c46cc6f4618d3e9fcd2a4/mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbar.java#658-663

--

To start, set up a build environment - you can see the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

To fix this bug, you need to remove the obsolete method:
https://dxr.mozilla.org/mozilla-central/rev/7011ed1427de2b6f075c46cc6f4618d3e9fcd2a4/mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbar.java#658-663

Then, you'll need to upload a patch - see:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "sebastian" and you can find me and other helpful folks in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Comment 1

[Pankaj Ahuja]

Hi Sebastian
I am new to open source and would like to contribute.
I feel this can be a good first bug for me and hope you assign it to me.
I am currently in the process of setting up the environment.(In reply to Sebastian Kaspari (:sebastian) from comment #0)
> BrowserToolbar.setToolBarButtonsAlpha() is unused and can be removed:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 7011ed1427de2b6f075c46cc6f4618d3e9fcd2a4/mobile/android/base/java/org/
> mozilla/gecko/toolbar/BrowserToolbar.java#658-663
> 
> --
> 
> To start, set up a build environment - you can see the instructions here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Simple_Firefox_for_Android_build
> 
> To fix this bug, you need to remove the obsolete method:
> https://dxr.mozilla.org/mozilla-central/rev/
> 7011ed1427de2b6f075c46cc6f4618d3e9fcd2a4/mobile/android/base/java/org/
> mozilla/gecko/toolbar/BrowserToolbar.java#658-663
> 
> Then, you'll need to upload a patch - see:
> http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/
> commits.html
> 
> If you need any help, you can reply to this bug, or feel free to message me
> on IRC - my nick is "sebastian" and you can find me and other helpful folks
> in #mobile. If you need IRC setup instructions, see
> https://wiki.mozilla.org/IRC

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Hi and welcome, Pankaj!

You can go ahead and write a patch for this bug. Once you upload a first patch we assign the bug to you. :)

Comment 3

[Francisco Aguiar]

I'm having a problem setting up my computer, in the mach package command i get the following message:

No command 'mach' found, did you mean:
 Command 'match' from package 'mailavenger' (universe)
mach: command not found

and i have my android phone connected to my pc in debugging mode

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

It should be in the root folder of your checkout of mozilla-central. So if you are inside that folder then just:
> ./mach build

Comment 5

[Francisco Aguiar]

im in the following directory:
maegner@maegner-X550JX:~/Documents/OpenSource/Mozilla$ ls
Bootstrap.py  mozilla-central

and i run ./mach build and i get:
bash: ./mach: No such file or directory

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Try inside the mozilla-central folder :)

Comment 7

[Francisco Aguiar]

yeah it worked, thx! currently building with de mach build command and when it's done i'll get to fixing the bug!

Comment 8

[Francisco Aguiar]

I've finished setting up and removed the method, now how do i make sure everything is okay with the app and finaly make my commit?

Comment 9

[Francisco Aguiar]

Attachment: Bug 1330009 - Removed BrowserToolbar.setToolBarButtonsAlpha()

Had a problem creating the message for the patch and didn't know how to change it afterwards.
I just deleted the function BrowserToolbar.setToolBarButtonsAlpha() previously located between the lines 658 to 663 from the file BrowserToolbar.java in the directory:
 mozilla-central/mobile/android/base/java/org/mozilla/gecko/toolbar

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

If you are using mercurial then you can edit the last commit (adding changes or updating the message) using:
> hg commit --amend

Comment 11

[Francisco Aguiar]

thank you :sebastian how that i've been assigned what am i supposed to do?

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

It would be helpful if you could update the patch with a commit message and upload the new patch. In the meantime I'll review it and push it to our 'try' servers that will run the tests and see if this causes any problems.

For future patches it's super helpful to not upload a patch file but instead push the file to mozreview:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

Comment 13

[Francisco Aguiar]

I've added the message but i'm having a problem making the push;
When i run the command hg push review
I get the following message:
pushing to https://reviewboard-hg.mozilla.org/autoreview
searching for appropriate review repository
redirecting push to https://reviewboard-hg.mozilla.org/gecko
running checkstyle... failed (e.g. Java was not installed) - ignoring.
    To see full error output, run locally with: `./mach gradle checkstyle`
(adding commit id to 1 changesets)
warning: Watchman unavailable: A non-recoverable condition has triggered.  Watchman needs your help!
The triggering condition was at timestamp=1484236701: inotify-add-watch(/home/maegner/Documents/OpenSource/Mozilla/mozilla-central/mobile/android/tests/browser/robocop/src) -> The user limit on the total number of inotify watches was reached; increase the fs.inotify.max_user_watches sysctl
All requests will continue to fail with this message until you resolve
the underlying problem.  You will find more information on fixing this at
https://facebook.github.io/watchman/docs/troubleshooting.html#poison-inotify-add-watch

abort: uncommitted changes

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Mh, in this case maybe upload a patch file for now.

(In reply to Francisco Aguiar from comment #13)
>     To see full error output, run locally with: `./mach gradle checkstyle`

Did you try running this command? Did it output any errors?

Comment 15

[Francisco Aguiar]

I got something like:
It looks like you tried to run a mach command from an invalid context. The gradle
command failed to meet the following conditions: 

  is_android - Must have an Android build.

Run |mach help| to show a list of all commands available to the current context.

Comment 16

[Francisco Aguiar]

Attachment: Now with message explaining the fix.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Francisco Aguiar from comment #15)
> I got something like:
> It looks like you tried to run a mach command from an invalid context. The
> gradle
> command failed to meet the following conditions: 
> 
>   is_android - Must have an Android build.
> 
> Run |mach help| to show a list of all commands available to the current
> context.

This is weird - did you actually go through the whole setup process and build the app?
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8826274 [details] [diff] [review]
Now with message explaining the fix.

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

The local build failed. It seems like this method needs to be removed from at least BrowserToolbarTablet too.

Please make sure you have setup the build correctly and can still build and run the app before uploading a new patch.

Comment 19

[Francisco Aguiar]

I will look it up and upload a new patch ASAP.
About the process to build the app i did but i had some problems with rust and some other software in the mean time so it might have corrupted the installation i'll make sure to re-build everything again to see where the push is having problems.

Comment 20

[Francisco Aguiar]

Attachment: Bug1330009

Complitely removed the setToolBarButtonsAlpha() method.

r?sebastian

Removed the setToolBarButtonsAlpha() methon from BrowserToolBar.java
and from BrowserToolBarTablet.java files, and checked in all the BrowserToolBar*.java files
in the directory mozilla-central/mobile/android/base/java/org/mozilla/gecko/toolbar

Review commit: https://reviewboard.mozilla.org/r/104832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104832/

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f18ac5367e0a6f482299f829b85eca22a91b33c
Bug 1330009 - Remove unused setToolBarButtonsAlpha() method. r=sebastian

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8826994 [details]
Bug1330009

https://reviewboard.mozilla.org/r/104832/#review105964

Thank you! This is looking good and I already landed the patch. :)

Comment 23

[Francisco Aguiar]

Thank you for helping me through the process. Now do I have to do anything else in regards to this bug?

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Everything is done here. The bug will be closed as soon as the code is merged to mozilla-central. :)

Comment 25

[Francisco Aguiar]

So should i set the status to Resolved?

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

No, this will happen automatically as soon as the code is merged.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f18ac5367e0