40248 - command-line-argument-handlers has entry/value args reversed

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Bruce Mitchener]

rv = catman->AddCategoryEntry(NS_HTTP_STARTUP_CATEGORY, cidString, "Http Cookie
Notify",

catman.addCategoryEntry("command-line-argument-handlers",CHATZILLASERVICE_PROGID,
"chatzilla command line handler", true, true);

catman.addCategoryEntry("command-line-argument-handlers",
       JSCONSOLEHANDLER_PROGID, "jsconsole command line handler",
       true, true);

These are all reversing the entry/value arguments.  It should be in the order:
category name, entry name, progid.

Comment 1

[(not reading, please use [e-mail] instead)]

looking at comments in nsICategoryManager.idl, yes, you are right.

but then nsCategoryManager::EnumerateCategory or
nsCategoryManager::AddCategoryEntry() is broken.

currently, when we add (aEntry,aValue), we enumerate all the aEntry values.

scc / shaver, please let me know what you want to do with nsICategoryManager.idl
/ nsCategoryManager.cpp

once that is settled, I'll fix all the callers.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'm not sure I agree.

If you
  AddCategoryEntry("category", "entry1", "value1")
  AddCategoryEntry("category", "entry2", "value2")
  AddCategoryEntry("category", "entry3", "value3")
then EnumerateCategoryEntries("category") should give you back
  EnumeratorOf("entry1", "entry2", "entry3")

If you want the values, you should be doing:
  foreach entry in EnumeratorOf("entry1", "entry2", "entry3")
    value = GetCategoryEntry("category", entry)
or something.  (Note: EnumerateCategoryEntries, not EnumerateCategoryValues.)

Make sense?

Comment 3

[(not reading, please use [e-mail] instead)]

ah, that makes sense.

I need to use GetCategoryEntry(), I was being stupid.

I better go make sure the entry values I've been using are unique.

I should have a fix in hand today.

Comment 4

[(not reading, please use [e-mail] instead)]

ok, fix in hand.  thanks again to bruce for pointing out the problem.

I've fixed the command line handler code and the NS_HTTP_STARTUP_CATEGORY code.

the category code under mozilla/mailnews/import looks ok, but I'll send mail to
tony, and let him take a look.

I'll try to check this stuff in today.  bad usage of the category manager will
just get copied and pasted around, so I want to fix this stuff asap.

Comment 5

[(not reading, please use [e-mail] instead)]

shaver, take a look at your code in mozilla/layout/base/src/nsContentPolicy.cpp

you need to use GetCategoryEntry(), to get the progid from the category entry.

(I don't see where anyone adds anything to the NS_CONTENTPOLICY_CATEGORY
category, so it's not critical.)

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The content-policy category uses progid for entry and value, actually, so it
doesn't matter.  It's not so much a parameterized category as a simple list.  (I
much prefer parameterization over simple lists, where that's sane, but I can't
think of a good way to parameterize the content-policy stuff.  Any suggestions?)

For clarity, though, maybe I should encourage people to use a human-meaningful
name for the entry, and the progid for the value.  But wait, progids are
supposed to be human-meaningful themselves.

So I need something unique and meaningful as the entry name.  Why not use the
progid, which people should already be working to make be both of those things?

In the command-line argument case, why not use the command-line switch as the
category entry?  So you'd
  AddCategoryEntry("command-line-argument-handlers", "chat", CHATZILLA_PROGID);
  AddCategoryEntry("command-line-argument-handlers", "irc",  CHATZILLA_PROGID);
to make it get called for both -chat and -irc.

We obviously need some best practices guidance here.

Comment 7

[(not reading, please use [e-mail] instead)]

shaver, using the command line for the entry is a good idea, and I've got
another bug for that.

marking nsbeta2, I've got the fix for this problem in hand, and I'd like to
check it in, to avoid future copy-n-paste errors.

(someone is bound to copy the way I've used the category manager)

Comment 8

[(not reading, please use [e-mail] instead)]

checked in the fix.

Comment 9

[Bruce Mitchener]

Yay. This looks good.  I'll file a bug to do the same fix for the mailnewsimport 
stuff against [e-mail]

Comment 10

[Martijn Pieters]

Heh, I indeed did the copy-n-paste =) That's why the JS console command line
handler had the same problem. I fixed my Zope Studio component as well.