1197441 - Remove BrowserToolbarBase.setButtonEnabled

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

To start, set up a build environment - you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

Then, you'll need to create a patch to upload - see
https://wiki.mozilla.org/Mobile/Fennec/Android#Creating_commits_and_submitting_patches

To fix this bug, remove `BrowserToolbarBase.setButtonEnabled` [1] and replace it's calls with `button.setEnabled`.

Why? bug 1197413 removed the alpha changing code from this method, so now it serves no purpose.

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

Thanks and happy coding! ^_^

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbarTabletBase.java?rev=d548ba019a7c#179

Comment 1

[Prateek Arora]

Attachment: BrowserToolbarTabletBase.java -- deleted setButtonEnabled function

Hey, I deleted the setButtonEnabled function. To find its calls, I used grep, but couldn't find any. Is there any easier way you guys use to find the affected code ? I am attaching the changed file BrowserToolbarTabletBase.java

Comment 2

[Prateek Arora]

(In reply to Prateek Arora from comment #1)
> Created attachment 8654585 [details] [diff] [review]
> BrowserToolbarTabletBase.java -- deleted setButtonEnabled function
> 
> Hey, I deleted the setButtonEnabled function. To find its calls, I used
> grep, but couldn't find any. Is there any easier way you guys use to find
> the affected code ? I am attaching the changed file
> BrowserToolbarTabletBase.java

I think I got it. In the same file, replace setButtonEnabled with button.setEnabled ?

Comment 3

[Prateek Arora]

Attachment: BrowserToolbarTabletBase.java

Understood the comment#0 a little better. Made the necessary changes. Removed setButtonEnabled calls with corresponding button.setEnabled in BrowserToolbarTablet and BrowserToolbarTabletBase files.

Should I attach both files here ?

Comment 4

[Prateek Arora]

Attachment: BrowserToolbarTabletBase.java

Adding Reviewer

Comment 5

[Prateek Arora]

Attachment: bug1197441.patch

Cannot believe my own dumbness that I have been attaching raw java file till now. Sorry guys for spaming your mail folders. Just learnt to generate patch file. Attached the patch file. Thanks

Comment 6

[Prateek Arora]

Attachment: bug1197441.patch --Fixed Some errors in previous patch

Comment 7

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Prateek Arora from comment #1)
> To find its calls, I used grep, but couldn't find any.

For fast plain text search, you can try out mxr.mozilla.org (or dxr.mozilla.org but it doesn't do anything special for our code). Use the mozilla-central tree for roughly what you'd be working with locally.

However, we also support IDEs which can generate accurate call hierarchies – see [1] to set this up.

(In reply to Prateek Arora from comment #5)
> I have been attaching raw java file till
> now. Sorry guys for spaming your mail folders. Just learnt to generate patch
> file. Attached the patch file. Thanks

No worries! We all have to learn somehow. :)

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android/IDEs

Comment 8

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8654916 [details] [diff] [review]
bug1197441.patch --Fixed Some errors in previous patch

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

Code changes look good but unfortunately, the patch does not apply cleanly - can you pull the latest changes and rebase your patch on top?

Then address the changes below and flag me for re-review!

::: mobile/android/base/toolbar/BrowserToolbarTablet.java
@@ +197,5 @@
>          return super.cancelEdit();
>      }
>  
>      private void stopEditingNewTablet() {
>          // Undo the changes caused by calling setButtonEnabled in startEditing.

Update this comment too.

(for your edification, I found this through my plain-text search [1]).

[1]: https://mxr.mozilla.org/mozilla-central/search?string=setbuttonenabled&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 9

[Prateek Arora]

Attachment: bug1197441.patch

Updated the patch as per Michael's comments.

@Michael, I tried pushing this for review through MozReview but reviewboard-hg.mozilla.org kept kicking me out. Permission Denied (publickey). Will look into it. For now, submitting patch through here only

Comment 10

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8655539 [details] [diff] [review]
bug1197441.patch

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

Code looks good but this patch still doesn't apply cleanly – can you pull the latest changes from upstream and rebase your patch on top? If you need help doing this, please let me know.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Prateek Arora from comment #9)
> @Michael, I tried pushing this for review through MozReview but
> reviewboard-hg.mozilla.org kept kicking me out. Permission Denied
> (publickey).

In the past, we only allowed employees onto reviewboard because ldap was used for authentication. That may have been overturned w/ the latest Bugzilla API key implementation [1]. Have you checked that out yet?

[1]: http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install.html

Comment 12

[Prateek Arora]

(In reply to Michael Comella (:mcomella) from comment #10)
> Comment on attachment 8655539 [details] [diff] [review]
> bug1197441.patch
> 
> Review of attachment 8655539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Code looks good but this patch still doesn't apply cleanly – can you pull
> the latest changes from upstream and rebase your patch on top? If you need
> help doing this, please let me know.

Hi, I have couple of points here :
1) Is it possible that when I create patch, and by the time it is reviewed, certain commits are pushed to mozilla-central ? Because, last night, I did 'hg pull', made my changes and created patch. After 3-4 hours when it was reviewed, there were other commits pushed in meantime. Can this be a problem ?

2)I have 3 heads right now in local repository. 1 says tip and is the result of latest hg pull. I will study more about hg heads, but could this be the problem ? I made sure that I am patching on top of latest code.

I will get back to the thread after a bit more study on hg. BTW which timezone are you ? Would like to catch you up on IRC. Thanks alot for struggling with me. ^_^

Comment 13

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

By the way, you can use the "Need more information from" field to give a user a persistent notification so they'll be more likely to see your comments or questions. For example, I'll flag you (note that I actually don't need anything atm so you can just clear it). And to answer you questions...

(In reply to Prateek Arora from comment #12)
> 1) Is it possible that when I create patch, and by the time it is reviewed,
> certain commits are pushed to mozilla-central ?

Yes, this is possible.

However, in the attachment of your patch, it mentions the parent commit is 22c34579ae0720e7d3dc39a22b9d33f13bc0198b, which is from August 21st, making me think the rebase did not complete successfully.

> 2)I have 3 heads right now in local repository. 1 says tip and is the result
> of latest hg pull. I will study more about hg heads, but could this be the
> problem ? I made sure that I am patching on top of latest code.

Sounds like this could be the problem. I assume you're not using patch queues and are using bookmarks? `hg rebase -b <bookmark-to-move> -d <mozilla-central-commit>` should do the trick, but read `hg help rebase` to make sure it's the right thing.

> I will get back to the thread after a bit more study on hg. BTW which
> timezone are you ? Would like to catch you up on IRC.

I'm PST and yes, if you jump on irc, it should be easier for me to help you.

> Thanks alot for struggling with me. ^_^

No worries – that's what I'm here for. :P

Comment 14

[Prateek Arora]

Attachment: bug1197441.patch

Tried rebasing using bookmarks this time. Hope this works. fingers crossed.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8656107 [details] [diff] [review]
bug1197441.patch

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

Rebase was successful! Yay! Thanks!

I made a push to our try test servers (above).

Once the push goes green, you can add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Let me know if you need help reading the results.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 16

[Prateek Arora]

Attachment: bug1197441.patch -- Added my name in author instead of email only

adding checkin-needed keyword.

try server run status :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9ee747e4bb4

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e0beeafff1f9

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0beeafff1f9