Remove BrowserToolbar.setToolBarButtonsAlpha()

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
General
P5
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: sebastian, Assigned: Francisco Aguiar, Mentored)

Tracking

unspecified
Firefox 53
All
Android
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 months ago
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

5 months 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
(Reporter)

Comment 2

5 months ago
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

5 months 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
(Reporter)

Comment 4

5 months ago
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

5 months 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
(Reporter)

Comment 6

5 months ago
Try inside the mozilla-central folder :)
(Assignee)

Comment 7

5 months 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

5 months 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

5 months ago
Created attachment 8825981 [details] [diff] [review]
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
Attachment #8825981 - Flags: review?(s.kaspari)
(Reporter)

Comment 10

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

Updated

4 months ago
Assignee: nobody → franciscommaguiar
Status: NEW → ASSIGNED
(Assignee)

Comment 11

4 months ago
thank you :sebastian how that i've been assigned what am i supposed to do?
(Reporter)

Comment 12

4 months ago
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

4 months 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
(Reporter)

Comment 14

4 months ago
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

4 months 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

4 months ago
Created attachment 8826274 [details] [diff] [review]
Now with message explaining the fix.
Attachment #8825981 - Attachment is obsolete: true
Attachment #8825981 - Flags: review?(s.kaspari)
Attachment #8826274 - Flags: review?(s.kaspari)
(Reporter)

Comment 17

4 months ago
(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
(Reporter)

Comment 18

4 months ago
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

4 months 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

4 months ago
Attachment #8826994 - Flags: review?(s.kaspari)
(Reporter)

Comment 21

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

Comment 22

4 months 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

4 months ago
Thank you for helping me through the process. Now do I have to do anything else in regards to this bug?
(Reporter)

Comment 24

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

Comment 25

4 months ago
So should i set the status to Resolved?
(Reporter)

Comment 26

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

Comment 27

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