Closed Bug 1197441 Opened 9 years ago Closed 9 years ago

Remove BrowserToolbarBase.setButtonEnabled

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: mcomella, Assigned: an0nym0usdroid42, Mentored)

References

Details

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

Attachments

(1 file, 7 obsolete files)

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
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
(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 ?
Attached patch BrowserToolbarTabletBase.java (obsolete) — Splinter Review
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 ?
Attachment #8654585 - Attachment is obsolete: true
Attached patch BrowserToolbarTabletBase.java (obsolete) — Splinter Review
Adding Reviewer
Attachment #8654586 - Attachment is obsolete: true
Attachment #8654622 - Flags: review?(michael.l.comella)
Attached patch bug1197441.patch (obsolete) — Splinter Review
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
Attachment #8654622 - Attachment is obsolete: true
Attachment #8654622 - Flags: review?(michael.l.comella)
Attachment #8654629 - Flags: review?(michael.l.comella)
Assignee: nobody → an0nym0usdroid42
Attachment #8654629 - Attachment is obsolete: true
Attachment #8654629 - Flags: review?(michael.l.comella)
Attachment #8654916 - Flags: review?(michael.l.comella)
(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 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
Attachment #8654916 - Flags: review?(michael.l.comella) → feedback+
Attached patch bug1197441.patch (obsolete) — Splinter Review
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
Attachment #8654916 - Attachment is obsolete: true
Attachment #8655539 - Flags: review?(michael.l.comella)
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.
Attachment #8655539 - Flags: review?(michael.l.comella) → feedback+
(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
(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. ^_^
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
Flags: needinfo?(an0nym0usdroid42)
Flags: needinfo?(an0nym0usdroid42)
Attached patch bug1197441.patch (obsolete) — Splinter Review
Tried rebasing using bookmarks this time. Hope this works. fingers crossed.
Attachment #8655539 - Attachment is obsolete: true
Attachment #8656107 - Flags: review?(michael.l.comella)
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
Attachment #8656107 - Flags: review?(michael.l.comella) → review+
adding checkin-needed keyword.

try server run status :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9ee747e4bb4
Attachment #8656107 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0beeafff1f9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.