Remove BrowserToolbar.setToolBarButtonsAlpha()

RESOLVED FIXED in Firefox 53

Status

()

defect
P5
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: franciscomaguiar, Mentored)

Tracking

unspecified
Firefox 53
All
Android
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

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

2 years ago
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
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. :)
(Assignee)

Comment 3

2 years ago
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
It should be in the root folder of your checkout of mozilla-central. So if you are inside that folder then just:
> ./mach build
(Assignee)

Comment 5

2 years ago
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
Try inside the mozilla-central folder :)
(Assignee)

Comment 7

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

Comment 8

2 years ago
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?
(Assignee)

Comment 9

2 years ago
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
Attachment #8825981 - Flags: review?(s.kaspari)
If you are using mercurial then you can edit the last commit (adding changes or updating the message) using:
> hg commit --amend
Assignee: nobody → franciscommaguiar
Status: NEW → ASSIGNED
(Assignee)

Comment 11

2 years ago
thank you :sebastian how that i've been assigned what am i supposed to do?
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
(Assignee)

Comment 13

2 years ago
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
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?
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
Attachment #8825981 - Attachment is obsolete: true
Attachment #8825981 - Flags: review?(s.kaspari)
Attachment #8826274 - Flags: review?(s.kaspari)
(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 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.
Attachment #8826274 - Flags: review?(s.kaspari) → review-
(Assignee)

Comment 19

2 years ago
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8826994 - Flags: review?(s.kaspari)
(Reporter)

Comment 22

2 years ago
mozreview-review
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. :)
Attachment #8826994 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 23

2 years ago
Thank you for helping me through the process. Now do I have to do anything else in regards to this bug?
Everything is done here. The bug will be closed as soon as the code is merged to mozilla-central. :)
(Assignee)

Comment 25

2 years ago
So should i set the status to Resolved?
No, this will happen automatically as soon as the code is merged.

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f18ac5367e0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.