Remove unused method hasOtherSyncClients

REOPENED
Unassigned

Status

()

REOPENED
3 years ago
3 years ago

People

(Reporter: mcomella, Unassigned, Mentored)

Tracking

(Depends on: 2 bugs)

unspecified
Firefox 45
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 unaffected)

Details

(Whiteboard: [lang=java])

Attachments

(3 attachments)

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 the unused method ActivityChooserModel.hasOtherSyncClients [1].

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/widget/ActivityChooserModel.java?rev=35336ca96de2#1298

Comment 1

3 years ago
I have setup the the build environment. I am able to run
./mach build
./mach package
./mach install
and run the browser in my device.

I can see "private boolean hasOtherSyncClients()" method in ActivityChooserModel.java class.
Removing the method "hasOtherSyncClients()" from the "ActivityChooserModel.java" is the fix??
(In reply to mails2bibin from comment #1)
> Removing the method "hasOtherSyncClients()" from the
> "ActivityChooserModel.java" is the fix??

That's correct! We'll assign you to the bug once you upload a patch. :)

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.
Created attachment 8682398 [details] [diff] [review]
Remove unused method hasOtherSyncClients

I deleted the hasOtherSyncClients function.
Attachment #8682398 - Flags: review?(michael.l.comella)
Comment on attachment 8682398 [details] [diff] [review]
Remove unused method hasOtherSyncClients

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

Looks good to me!

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 #8682398 - Flags: review?(michael.l.comella) → review+
Assignee: nobody → mrjohnsonalex
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0ba952c2819
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
re backout: see bug 1140048 comment 83. Since we backed out the bug 1140048 that added this bug, this does not affect any versions until bug 1140048 lands again.
Status: RESOLVED → REOPENED
status-firefox45: fixed → unaffected
Resolution: FIXED → ---
Created attachment 8689629 [details]
MozReview Request: Bug 1216307 - Backed out. r?mcomella

Bug 1216307 - Backed out. r?mcomella
Attachment #8689629 - Flags: review?(michael.l.comella)
Comment on attachment 8689629 [details]
MozReview Request: Bug 1216307 - Backed out. r?mcomella

https://reviewboard.mozilla.org/r/25679/#review23123

Backouts don't need reviews and I already backed it out.

Thanks for the patch though!
Attachment #8689629 - Flags: review?(michael.l.comella)

Comment 12

3 years ago
I am computer science student , i would like to fix this bug , is that okay ? :)
(In reply to hassani2 from comment #12)
> I am computer science student , i would like to fix this bug , is that okay
> ? :)

We must wait till bug 1140048 relands before we land this bug.  See comment 9.
Assignee: alex_johnson24 → hassani2
I don't think bug 1140048 will reland soon – we need to complete bug 1217174 first, which is a bit of work and prioritized.

hassani2, would you like to talk a look at bug 1164307?
Assignee: hassani2 → nobody

Comment 15

3 years ago
Same for this bug, we should just address removing unused code in the same patch that removes the callsites.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → INVALID
I'd like to leave this open so we don't forget to do it as part of bug 1140048.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

Comment 17

3 years ago
Created attachment 8706468 [details] [diff] [review]
bug-1216307-fix.patch

Deleted method hasOtherSyncClients() as well as its callsite.
Attachment #8706468 - Flags: review?(michael.l.comella)
Comment on attachment 8706468 [details] [diff] [review]
bug-1216307-fix.patch

Removing this depends on the completion of bug 1140048 first (or more likely bug 1217174).
Attachment #8706468 - Flags: review?(michael.l.comella) → review-

Comment 19

3 years ago
Hi Michael, I am Java certified and also a newbie here.I want to resolve this bug.May be I can help you.
Flags: needinfo?(michael.l.comella)
(In reply to Swarnim from comment #19)
> Hi Michael, I am Java certified and also a newbie here.I want to resolve
> this bug.May be I can help you.

Hi Swarnim – this bug requires bug 1140048 to be completed first. Perhaps you'd like to take a look at bug 1042201?
Flags: needinfo?(michael.l.comella)

Comment 21

3 years ago
Yes Michael, I have gone through Bug 1042201 - Search provider suggestions widget for error pages.I read all the comments and attachments but still having some doubts related to that bug.Perhaps I have to talk with the Mentor:Margaret Leibovic.

Thankyou Michael

Comment 22

3 years ago
Hello,

I'am really interesting to try to fix a bug (I am beginner).
I install intelliJ,and import mozilla center, but i don't know with witch bug i can start.
Anybody can advise to me ? :)

Thank you in advance for your sugestions
Hey Alex! Welcome to Bugzilla.

Unfortunately, this isn't a good bug to start because it requires bug 1140048 to be completed first. Perhaps you can take a look at bug 1242629?

By the way, you can find us on mozilla irc in the channel #mobile if you have further questions!

Comment 24

3 years ago
Hi Michael,
Thanks for your answer !
I will see the bug 1242629 :)
(In reply to Michael Comella (:mcomella) from comment #23)
> Hey Alex! Welcome to Bugzilla.
> 
> Unfortunately, this isn't a good bug to start because it requires bug
> 1140048 to be completed first. Perhaps you can take a look at bug 1242629?

Wouldn't we want to remove the "good first bug" whiteboard tag then?
Flags: needinfo?(michael.l.comella)
(In reply to Andreas Wagner [:TheOne] from comment #25)
> > Unfortunately, this isn't a good bug to start because it requires bug
> > 1140048 to be completed first. Perhaps you can take a look at bug 1242629?
> 
> Wouldn't we want to remove the "good first bug" whiteboard tag then?

From a correctness point of view, I don't think so: bugs that have unfixed dependencies can still be [good first bug]s (when they're ready to be worked on).

However, ideally we wouldn't broadcast [good first bug]s that have unfixed dependencies to avoid issues where someone comes in to work on a bug but doesn't see the dependencies. Unfortunately, it's too much bookkeeping to only add the [good first bug] flags once the dependencies are fixed.

I think the ideal solution here would be to update the [good first bug] tools to only display [good first bug]s that don't have dependencies.

That being said, I don't think bug 1140048 will get fixed any time soon so I'm going to remove the [good first bug] tag. Thanks for pointing it out.
Flags: needinfo?(michael.l.comella)
Whiteboard: [lang=java][good first bug] → [lang=java]
You need to log in before you can comment on or make changes to this bug.