I am in the process of removing nsIAtomService (bug 1392884) in order to deCOMtaminate nsIAtom (bug 1392883). There are a few uses in comm-central that need to be removed.

A number of OnItem*() methods have nsIAtom parameters. Some of these methods convert those nsIAtoms to strings and then check their value against another string. Others check their value against an atom obtained from nsIAtomService. This patch changes the latter methods to be like the former methods, thus removing numerous uses of nsIAtomService.

nsIDocShell.forcedCharset is an `ACString` attribute. viewlog.js currently assigns it an atom. This patch changes it to a string. (It's likely this code is unused, because it was clearly broken: mAtomService is not mentioned anywhere else in the code.)

nsIMsgFolderListener.itemEvent() and nsIMsgFolderNotificationService.notifyItemEvent() both have an `in nsISupports aData` argument that can take an nsIAtom. We're removing nsIAtom usage from scripts, so this patch gives them both an extra `in AUTF8String aString` argument that can be used instead to pass string data. This patch changes the two places that pass an atom (in nsMsgDBView::ApplyCommandToIndices() and SyntheticMessageSet.prototype.setJunk()) are changed to pass an equivalent string via the new argument.

As usual, I have checked this code compiles but not tested it. Given that it modifies a decent amount of JavaScript, I recommend a try push before landing :) Thank you.

Comment on attachment 8901049 [details] [diff] [review] (part 1) - Convert nsIAtom arguments to strings before checking their value Looks good upon visual inspection.

Comment on attachment 8901050 [details] [diff] [review] (part 2) - Fix forcedCharset assignment Good catch, looks like dead code to me.

Comment on attachment 8901052 [details] [diff] [review] (part 3) - Avoid atom usage for itemEvent/notifyItemEvent Looks very nice upon visual inspection. Do I see it correctly that I could try/land these patches now? Let me see how my last C-C push turned out, then I'll do a try. Please note, we've been thinking about removing all atom use from C-C C++ code, see bug 1334834. There's a (bit-rotten?) C++ patch that forgot to do the JS parts, doh! Looks like you have big plans for atoms (bug 1392883), so having identified the C++ usage might come in handy. BTW, were are you located, quite late in the Americas right now. And not to forget, thanks as always!!

> Do I see it correctly that I could try/land these patches now? Yes! > Please note, we've been thinking about removing all atom use from C-C C++ > code, see bug 1334834. There's a (bit-rotten?) C++ patch that forgot to do > the JS parts, doh! > > Looks like you have big plans for atoms (bug 1392883), so having identified > the C++ usage might come in handy. Oh, interesting. Yes, something like that patch will likely be necessary. > BTW, were are you located, quite late in the Americas right now. I'm in Melbourne, Australia. It's 5:14pm on Friday afternoon here :)

Comment on attachment 8901049 [details] [diff] [review] (part 1) - Convert nsIAtom arguments to strings before checking their value Review of attachment 8901049 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/db/gloda/modules/index_msg.js @@ +2802,5 @@ > * status. > */ > OnItemPropertyFlagChanged: function gloda_indexer_OnItemPropertyFlagChanged( > aMsgHdr, aProperty, aOldValue, aNewValue) { > + propertyString = aProperty.toString(); Some declaration missing here. I can fix that.

Comment on attachment 8901052 [details] [diff] [review] (part 3) - Avoid atom usage for itemEvent/notifyItemEvent Review of attachment 8901052 [details] [diff] [review]: ----------------------------------------------------------------- Is the new argument in NotifyItemEvent a temporary measure (maybe caused by the m-c parent definitions) until all users are converted?

No. The third argument was an nsISupports that received headers (in nsMsgMaildirStore.cpp) and atoms (messageModifier.js). Now we need two arguments to pass these different types of data.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4081763c440cedf0785da05a1e2fd65a399d1be6 with the fix to |let propertyString = aProperty.toString();|.

Looks like that caused some test failures: TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_messages_local.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_messages_local.js | test_filthy_moves_slash_move_from_unindexed_to_indexed - [test_filthy_moves_slash_move_from_unindexed_to_indexed : 1219] 9999 == 0 TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_junk_local.js | xpcshell return code: 0 I'll have a look before landing this.

Aceman, in case you want to try this, here is the fixed 3rd part.

BTW, mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_index_junk_local.js is the faster test to run of the two.

Comment on attachment 8901299 [details] [diff] [review] (part 3) - Avoid atom usage for itemEvent/notifyItemEvent - missing declaration fixed Sorry, I changed the part 1.

Interestingly enough the problem doesn't come from part 3, since it occurs already with part 1 (and 2). So instead of looking at part 3, which looks right, the focus should be on part 1 :-(

Here we have it, a cut&paste error: if (aProperty.toString() !== "FolderFlagAtom") :-(

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/ac14ca6da238 (part 1) - Convert nsIAtom arguments to strings before checking their value. r=jorgk https://hg.mozilla.org/comm-central/rev/40598e802849 (part 2) - Fix forcedCharset assignment. r=jorgk https://hg.mozilla.org/comm-central/rev/351c858c276c (part 3) - Avoid atom usage for itemEvent/notifyItemEvent. r=jorgk

Thank you for debugging and landing the patch. I'm actually heartened that you had to fix some problems, because it shows there is reasonable test coverage. > Please note, we've been thinking about removing all atom use from C-C C++ code, see bug 1334834. > There's a (bit-rotten?) C++ patch that forgot to do the JS parts, doh! What was the motivation there -- some kind of test failures?

As bug 1334834 mentions, this came from 1334558. We used to have static atoms and I switched to dynamic atoms as a bustage-fix. Back then, switching to strings was also considered. I'm working on bug 1334834 now.

Inspired by you we will now go ahead and remove use of nsIAtom in nsIFolderListener.idl and nsIMsgFolder.idl. I believe that will completely remove nsIAtom from comm-central (but I haven't checked exactly). That was on the books for a while, but given that you did the JS part for us here, it had become a little easier. So thanks again!

(In reply to Nicholas Nethercote [:njn] from comment #22) > > Please note, we've been thinking about removing all atom use from C-C C++ code, see bug 1334834. > > There's a (bit-rotten?) C++ patch that forgot to do the JS parts, doh! > > What was the motivation there -- some kind of test failures? You mean we could keep using nsIAtom inside C++ code (as it isn't going away completely)? But we still need to remove it from the public .idl interfaces (seen from JS). So how would that be possible without removing it also from the C++ methods implementing the interfaces?

Aceman, please ready bug 1392883 comment #0: The idea is remove the XPCOM interface nsIAtom and turn that into a C++ class, like has happened twice in recent bustage: bug 1393088, bug 1380413/bug 1380413. So we're well advised to remove nsIAtom. > So how would that be possible without removing it also from the C++ methods implementing the interfaces? There will be a different C++ interface for the new atoms (mark 2). So we might as well switch to strings now.

> You mean we could keep using nsIAtom inside C++ code (as it isn't going away > completely)? But we still need to remove it from the public .idl interfaces > (seen from JS). Correct. nsIAtom is used a huge amount in C++ code but only a tiny amount in script code. My goal is to remove nsIAtom.idl and convert nsIAtom to a vanilla C++-only type. > So how would that be possible without removing it also from > the C++ methods implementing the interfaces? Bug 1393642 provides one possible model: - Change those IDL methods to take an AString instead of an nsIAtom. - For each existing IDL method, add a new C++ function of the same name that takes an nsIAtom parameter. The existing C++ function would then just be a thin wrapper function that atomizes the argument and then calls the new method.

Sorry, pasted that badly: bug 1380413/bug 1381011.

OK, so in that case we do not need to make those wrapper functions to accept both strings from JS and nsIAtom from C++ if we don't really need atoms internally. I don't know what are atoms good for but if we can convert to plain strings (or nsCString) is that better than keep atoms?

> I don't know what are atoms good for An atom is a guaranteed-unique representation of a particular string value. It lets you do pointer-comparison for equality instead of full string equality. They're often called "interned strings", see https://en.wikipedia.org/wiki/String_interning for more.

Thanks, so it is a performance feature for often used strings.