1231655 - Remove GeckoApp.showButtonToast()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

GeckoApp.showButtonToast() is not used anymore and we can remove it now:

https://dxr.mozilla.org/mozilla-central/rev/b40ba117fa757861c9caa660ae989122718b494b/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#901

Comment 1

[Roland Demeter]

Hello Sebastian!

Could you assign this to me?

Thanks!

Comment 2

[Roland Demeter]

Attachment: bug1231655.diff

I have removed the unnecessary code and I have attached a patch.

Comment 3

[Alex Johnson (:alex_johnson)]

(In reply to Roland Demeter from comment #2)
> Created attachment 8697301 [details] [diff] [review]
> bug1231655.diff
> 
> I have removed the unnecessary code and I have attached a patch.

You could probably also remove the import for ButtonToast, getButtonToast() and the ButtonToast.java file.

Comment 4

[Roland Demeter]

Attachment: All instances removed

Removed all instances of ButtonToast.

Comment 5

[Hannah Calow]

Attachment: I have removed all specified instances of the the method.

I have attached a patch to remove everything mentioned in the previous comment.

Comment 6

[Alex Johnson (:alex_johnson)]

Comment on attachment 8697317 [details] [diff] [review]
All instances removed

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

Thanks!  :)

Comment 7

[Alex Johnson (:alex_johnson)]

Comment on attachment 8697317 [details] [diff] [review]
All instances removed

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

Actually, could you rebase this on top of Bug 1107811?

Comment 8

[Alex Johnson (:alex_johnson)]

To do this, first do hg log and copy the changeset id for your commit, then:
hg pull
hg rebase -s your-changeset-id -d tip

Comment 9

[Roland Demeter]

Attachment: bug1231655.diff

I have made the changes you requested, I rant into a few errors but I think I got there eventually!

Thank you!

Comment 10

[Alex Johnson (:alex_johnson)]

Comment on attachment 8697563 [details] [diff] [review]
bug1231655.diff

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

Look good to me!  But, it looks like the removal of ButtonToast.java is missing from this patch.  If you could remove it and attach it as a separate patch, I could r+ it too.

Comment 11

[Alex Johnson (:alex_johnson)]

(In reply to Alex Johnson(:alex_johnson) from comment #10)
> Comment on attachment 8697563 [details] [diff] [review]
> bug1231655.diff
> 
> Review of attachment 8697563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Look good to me!  But, it looks like the removal of ButtonToast.java is
> missing from this patch.  If you could remove it and attach it as a separate
> patch, I could r+ it too.

You might also need to remove ButtonToast from moz.build and BrowserApp.java.  Here are all the instances of ButtonToast in the project: https://dxr.mozilla.org/mozilla-central/search?q=buttontoast&redirect=false&case=false.  You have removed the one's from GeckoApp, we'll also need to get rid of the rest.

Comment 12

[Alex Johnson (:alex_johnson)]

We'll actually do this as another bug.  We'll just focus on GeckoApp here (the title of the bug).

Comment 13

[Roland Demeter]

Attachment: Bug1231655.diff

Hello Alex!

I have removed all instances and I have attached a new patch. I couldn't find any ButtonToast instances in my GeckoApp.jave file.

Comment 14

[Alex Johnson (:alex_johnson)]

Comment on attachment 8697581 [details] [diff] [review]
Bug1231655.diff

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

Could you submit this to Bug 1231958? We'll just focus on GeckoApp here.

Comment 15

[Alex Johnson (:alex_johnson)]

Comment on attachment 8697563 [details] [diff] [review]
bug1231655.diff

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

Roland, go ahead and set the keyword to "checkin-needed".

Comment 16

[Alex Johnson (:alex_johnson)]

Comment on attachment 8697563 [details] [diff] [review]
bug1231655.diff

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

Set the "checkin-needed" after this succeeds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1375117708cb  Let me know if you need help interpreting the results.  Thanks for the help!  :)

Comment 17

[Alex Johnson (:alex_johnson)]

Hannah, sorry we couldn't use your patch.  Roland was already assigned to the bug. But here's another bug you can work on if you're interested: Bug 1229105.

Comment 18

[Alex Johnson (:alex_johnson)]

Comment on attachment 8697581 [details] [diff] [review]
Bug1231655.diff

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

Actually, build fails if we don't remove the instances of ButtonToast from BrowserApp.  So, we're gonna have to land them both together.  Sorry!
Could you also delete mobile/android/base/java/org/mozilla/gecko/widget/ButtonToast.java?

Comment 19

[Roland Demeter]

(In reply to Alex Johnson (:alex_johnson) from comment #18)
> Comment on attachment 8697581 [details] [diff] [review]
> Bug1231655.diff
> 
> Review of attachment 8697581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, build fails if we don't remove the instances of ButtonToast from
> BrowserApp.  So, we're gonna have to land them both together.  Sorry!
> Could you also delete
> mobile/android/base/java/org/mozilla/gecko/widget/ButtonToast.java?

Hello Alex!

In my library the file does not exist. Is there a way to combine them all together as a patch again, so I could submit a whole patch with all the instances removed?

Thanks,
Roland

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Removing "checkin-needed". It seems like this series of patches is not ready yet?

Having a single patch for all changes here is a good idea. I usually use "hg rebase --collapse" for that. histedit is a mercurial extension that can do that too. Please flag me for review of the final patch.

Comment 21

[Alex Johnson (:alex_johnson)]

(In reply to Roland Demeter from comment #19)
> (In reply to Alex Johnson (:alex_johnson) from comment #18)
> > Comment on attachment 8697581 [details] [diff] [review]
> > Bug1231655.diff
> > 
> > Review of attachment 8697581 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Actually, build fails if we don't remove the instances of ButtonToast from
> > BrowserApp.  So, we're gonna have to land them both together.  Sorry!
> > Could you also delete
> > mobile/android/base/java/org/mozilla/gecko/widget/ButtonToast.java?
> 
> Hello Alex!
> 
> In my library the file does not exist. Is there a way to combine them all
> together as a patch again, so I could submit a whole patch with all the
> instances removed?
> 
> Thanks,
> Roland

You could try and find the commit where you removed that file and rebase it on top of the latest patch you want to submit. And then collapse them into one commit like sebastian mentioned. Also, don't forget to flag him for the final review. Thanks for your help! :)

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

@Roland: Are you still interested in working on this?

Comment 24

[k.krish]

I would like to take a look into this bug and fix it.
Can I move forward ?

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to k.krish from comment #24)
> I would like to take a look into this bug and fix it.
> Can I move forward ?

Okay. As soon as you'll attach a first patch I'll assign the bug to you. :)

Comment 26

[k.krish]

Attachment: Bug1231655.diff

Please review the patch

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8728379 [details] [diff] [review]
Bug1231655.diff

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

This patch is looking good. Let's just remove all those unnecessary import changes.

If you are using patch queues then you can update your patch with "hg qrefresh". If you use bookmarks or just plain commits then you can use "hg commit --amend" to update your last commit. After that upload the new patch and flag me for review (select review ? and then add me as reviewer).

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +62,5 @@
> +import android.widget.Toast;
> +
> +import org.json.JSONArray;
> +import org.json.JSONException;
> +import org.json.JSONObject;

There are a lot of import changes here (probably your IDE tried to be "helpful"). Please update your patch to not include all those changes.

Comment 28

[k.krish]

Attachment: Bug1231655.diff

I have changed the patch as required. Please take a look. Thanks :)

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8728379 [details] [diff] [review]
Bug1231655.diff

Marking the old patch as obsolete.

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8728439 [details] [diff] [review]
Bug1231655.diff

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

LGTM. Thank you! :)

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/6009eb63149d0dcacfa659b6d84a6f1eaa898c06
Bug 1231655 - Remove GeckoApp.showButtonToast(). r=sebastian

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6009eb63149d