Closed Bug 1315980 Opened 4 years ago Closed 4 years ago

[findbugs] [CN] Class defines clone() but doesn't implement Cloneable

Categories

(Firefox for Android :: General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: sebastian, Assigned: swaroop.rao, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

* org.mozilla.gecko.sync.ExtendedJSONObject defines clone() but doesn't implement Cloneable
* org.mozilla.gecko.widget.ResizablePathDrawable$NonScaledPathShape defines clone() but doesn't implement Cloneable

"This class defines a clone() method but the class doesn't implement Cloneable. There are some situations in which this is OK (e.g., you want to control how subclasses can clone themselves), but just make sure that this is what you intended."
To start, set up a build environment - you can see the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Then, you'll need to upload a patch - see:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "sebastian" and you can find me and other helpful folks in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC
I have submitted a patch for review using MozReview. After looking at the code, I believe that the class should implement the Cloneable interface. Since the class is an inner class, it can be more complex and unintuitive to override the clone() method in a subclass of the parent class, so I feel that the original author intended to implement it as a Cloneable but just forgot. Please let me know if there is anything more to be done for this bug.
Comment on attachment 8809948 [details]
Bug 1315980 - Changed ExtendedJSONObject to implement Cloneable and the clone() method to throw CloneNotSupportedException. Changed inner class in ResizablePathDrawable to implement Cloneable.

https://reviewboard.mozilla.org/r/92418/#review92942

This is looking good. Can you look at "ExtendedJSONObject" too (See comment 0)?
Attachment #8809948 - Flags: review?(s.kaspari)
Assignee: nobody → swaroop.rao
Sorry I missed that one. I've fixed ExtendedJSONObject and submitted it for review, as well.
Comment on attachment 8809948 [details]
Bug 1315980 - Changed ExtendedJSONObject to implement Cloneable and the clone() method to throw CloneNotSupportedException. Changed inner class in ResizablePathDrawable to implement Cloneable.

https://reviewboard.mozilla.org/r/92418/#review93700

This seems to have overriden the previous change. Make sure to either add both changes to the commit or push two commits to reviewboard.
Attachment #8809948 - Flags: review?(s.kaspari)
I couldn't figure out a way to add two changesets to a single review request in MozReview, so I backed out both commits, created a fresh commit with both changes and submitted that changeset for review. Hope that's OK.
Comment on attachment 8809948 [details]
Bug 1315980 - Changed ExtendedJSONObject to implement Cloneable and the clone() method to throw CloneNotSupportedException. Changed inner class in ResizablePathDrawable to implement Cloneable.

https://reviewboard.mozilla.org/r/92418/#review94074
Attachment #8809948 - Flags: review?(s.kaspari) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a34cadadc944
Changed ExtendedJSONObject to implement Cloneable and the clone() method to throw CloneNotSupportedException. Changed inner class in ResizablePathDrawable to implement Cloneable. r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a34cadadc944
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.