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

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

* 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."

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 2

[swaroop.rao]

Attachment: Bug 1315980 - Changed ExtendedJSONObject to implement Cloneable and the clone() method to throw CloneNotSupportedException. Changed inner class in ResizablePathDrawable to implement Cloneable.

Review commit: https://reviewboard.mozilla.org/r/92418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92418/

Comment 3

[swaroop.rao]

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 4

[Sebastian Kaspari (:sebastian; :pocmo)]

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)?

Comment 5

[swaroop.rao]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92418/diff/1-2/

Comment 6

[swaroop.rao]

Sorry I missed that one. I've fixed ExtendedJSONObject and submitted it for review, as well.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 8

[swaroop.rao]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92418/diff/2-3/

Comment 9

[swaroop.rao]

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 10

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9fbfb6f55d5

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a34cadadc944

Comment 14

[areya]

https://hg.mozilla.org/mozilla-central/rev/a34cadadc944