Closed Bug 555942 Opened 12 years ago Closed 11 years ago

Name of AddonsManager.getAddons() is misleading/inconsistent

Categories

(Toolkit :: Add-ons Manager, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5

People

(Reporter: Unfocused, Assigned: mossop)

References

Details

(Whiteboard: [rewrite])

Attachments

(3 files, 1 obsolete file)

Given that AddonsManager has getAddonsByTypes() and getAddonsWithPendingOperations(), the name of getAddons() is a bit misleading. It implies that it will get all addons, not restricted by any criteria - when what is really wanted is getAddonsByTypes(null, ...).

I'd like to see this renamed to getAddonsByIDs().

As an alternate (or in addition to!) solution, getAddons() could support a null first-argument similar to getAddonsByTypes().
Changing getAddons to getAddonsByID and getAddon to getAddonByID seems to make things consistent. Do you agree with this Rob?
Priority: -- → P1
I kind of prefer getAddonsByIDs over getAddonsByID to be consistent with the names of similar functions. Other than that I agree.
If this is to be done then getAddon should probably be renamed to getAddonByID. With the ID being the default property to identify an add-on I don't have a serious issue with the name being getAddons but I do agree with Blair that getAddonsByIDs is clearing just as getAddonByID is also clearer.
So I think to be consistent we should switch everything to the following methods:

getAddonByID
getAddonsByIDs
getAddonsByTypes
getAddonsWithPendingOperationsByTypes (urk!)
getInstallsByTypes

I'm also tempted to add an explicit:

getAllAddons
getAllInstalls

How does this sound?
Sounds good. My initial impression of getAddonsWithPendingOperationsByTypes is that it could be renamed to getAddonsWithOperationsByTypes. Pernding is redundant since if it has an operation to perform the operation is a pending operation.
Assignee: nobody → dtownsend
Attached patch style changes (obsolete) — Splinter Review
Pure style changes to add prefixes to all arguments and fix spacing in javadoc comments. Rob says this does not require review.
Attachment #439321 - Flags: review+
Comment on attachment 439321 [details] [diff] [review]
style changes

This is wrong
Attachment #439321 - Attachment is obsolete: true
Attached patch style changesSplinter Review
Attachment #439427 - Flags: review+
These are just the naming changes, not sure if you need to do much of a review of the patch.
Attachment #439432 - Flags: review?(robert.bugzilla)
Comment on attachment 439432 [details] [diff] [review]
naming changes rev 1

looks good!
Attachment #439432 - Flags: review?(robert.bugzilla) → review+
Landed:

http://hg.mozilla.org/projects/addonsmgr/rev/aeae9f36e2d2
http://hg.mozilla.org/projects/addonsmgr/rev/847ffd15dbdb

All the APIs are covered by automated tests.
Status: NEW → ASSIGNED
Flags: in-testsuite+
Flags: in-litmus-
Whiteboard: [rewrite] → [rewrite][fixed-in-addonsmgr][needs-landing]
Attached patch follow-upSplinter Review
Missed a couple of uses in the plugins tests
Attachment #441558 - Flags: review?(robert.bugzilla)
Attachment #441558 - Flags: review?(robert.bugzilla) → review+
http://hg.mozilla.org/mozilla-central/rev/adfa8cb1c5ec
http://hg.mozilla.org/mozilla-central/rev/f63f946b292f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [rewrite][fixed-in-addonsmgr][needs-landing] → [rewrite]
Target Milestone: --- → mozilla1.9.3a5
Blocks: 556200
Verified fixed by hg log.
Version: unspecified → Trunk
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.