Remove GeckoApp.showButtonToast()

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sebastian, Assigned: k.krish, Mentored)

Tracking

unspecified
Firefox 48
All
Android
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

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

Attachments

(1 attachment, 6 obsolete attachments)

Comment 1

3 years ago
Hello Sebastian!

Could you assign this to me?

Thanks!

Comment 2

3 years ago
Created attachment 8697301 [details] [diff] [review]
bug1231655.diff

I have removed the unnecessary code and I have attached a patch.
Attachment #8697301 - Flags: review?(s.kaspari)
Assignee: nobody → roliusz
(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

3 years ago
Created attachment 8697317 [details] [diff] [review]
All instances removed

Removed all instances of ButtonToast.
Attachment #8697317 - Flags: review?(alex_johnson24)

Comment 5

3 years ago
Created attachment 8697319 [details]
I have removed all specified instances of the the method.

I have attached a patch to remove everything mentioned in the previous comment.
Comment on attachment 8697317 [details] [diff] [review]
All instances removed

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

Thanks!  :)
Attachment #8697317 - Flags: review?(alex_johnson24) → review+
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?
Attachment #8697317 - Flags: review+ → review-
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

3 years ago
Created attachment 8697563 [details] [diff] [review]
bug1231655.diff

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

Thank you!
Attachment #8697563 - Flags: review?(alex_johnson24)
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.
Attachment #8697563 - Flags: review?(alex_johnson24) → review+
(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.
We'll actually do this as another bug.  We'll just focus on GeckoApp here (the title of the bug).
Blocks: 1231958

Comment 13

3 years ago
Created attachment 8697581 [details] [diff] [review]
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.
Attachment #8697581 - Flags: review?(alex_johnson24)
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.
Attachment #8697581 - Flags: review?(alex_johnson24) → review-
Mentor: alex_johnson24
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".
Attachment #8697563 - Flags: checkin+
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!  :)
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.
Flags: needinfo?(calowh)
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?
Attachment #8697581 - Flags: review- → review+
Flags: needinfo?(calowh)
Attachment #8697301 - Attachment is obsolete: true
Attachment #8697301 - Flags: review?(s.kaspari)
Attachment #8697317 - Attachment is obsolete: true
Attachment #8697319 - Attachment is obsolete: true

Updated

3 years ago
Keywords: checkin-needed

Comment 19

3 years ago
(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
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.
Keywords: checkin-needed
(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! :)
No longer blocks: 1231958
Duplicate of this bug: 1231958
@Roland: Are you still interested in working on this?
Flags: needinfo?(roliusz)
(Assignee)

Comment 24

3 years ago
I would like to take a look into this bug and fix it.
Can I move forward ?
Flags: needinfo?(s.kaspari)
(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. :)
Assignee: roliusz → nobody
Flags: needinfo?(s.kaspari)
Flags: needinfo?(roliusz)
Attachment #8697563 - Attachment is obsolete: true
Attachment #8697581 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8728379 [details] [diff] [review]
Bug1231655.diff

Please review the patch
Attachment #8728379 - Flags: review+
Attachment #8728379 - Flags: review+ → review?(s.kaspari)
Assignee: nobody → k.krish
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.
Attachment #8728379 - Flags: review?(s.kaspari) → feedback+
(Assignee)

Comment 28

3 years ago
Created attachment 8728439 [details] [diff] [review]
Bug1231655.diff

I have changed the patch as required. Please take a look. Thanks :)
Attachment #8728439 - Flags: review?(s.kaspari)
Comment on attachment 8728379 [details] [diff] [review]
Bug1231655.diff

Marking the old patch as obsolete.
Attachment #8728379 - Attachment is obsolete: true
Comment on attachment 8728439 [details] [diff] [review]
Bug1231655.diff

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

LGTM. Thank you! :)
Attachment #8728439 - Flags: review?(s.kaspari) → review+

Comment 32

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6009eb63149d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.