They're supposed to be singletons that live the life of the application. They're perfect candidates to be converted to modules. Marking as blocking bug 467116, as this will make life easier there.

This won't apply on trunk, some other patches need to get in first.

The idea behind this patch is to convert the component functions into objects, so that we can simply export the object as a symbol later. I'll ask for review once the other patches that this one is applied on top of get into trunk.

This patch is ready for review. What's been done: - coalesced everything into an object, "SearchIntegration". SearchIntegration extends "SearchSupport", which is the sum total of everything in searchCommon.js. (These changes have brought in an extra level of indentation, which is why I'm also attaching a -w patch) - The individual stream listeners now extend SearchSupport._streamListenerBase, into which more common code has been moved. QueueMessageToGetIndexed is now _streamListenerBase.startStreaming. - The idle/message displayed observers are all now part of SearchSupport/SearchIntegration. - A lot of the initialization that used to happen in Init...Integration() and elsewhere has been moved to individual getters and setters. This seems to help in code readability. - Several comments added. This patch will make it tremendously easier to later export the objects as module symbols. Part 2 is to export SearchSupport as a module, and part 3 to export the two desktop search integration components as modules.

No changes, but fixes bitrot.

very neat, a few things from the original code that could be cleaned up while all this code is changing: we don't need a temp var for msgHdr here: + let msgHdr = aMsgs.queryElementAt(i, Ci.nsIMsgDBHdr); + let file = SearchIntegration._getSupportFile(msgHdr); or here: + let msg = aSrcMsgs.queryElementAt(i, Ci.nsIMsgDBHdr); + let srcFile = SearchIntegration._getSupportFile(msg); - var messageId = msgHdr.messageId; + let messageId = msgHdr.messageId; messageId = encodeURIComponent(messageId); should be: let messageId = encodeURIComponent(msgHdr.messageId); similarly, here: (and out of curiosity, is escapeString the wrong thing here?) - var messageId = msgHdr.messageId; - // for the trunk, this should work - // var netUtils = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsINetUtil); - // messageId = netUtils.escapeString(messageId, netUtils.ESCAPE_URL_PATH); + let messageId = msgHdr.messageId; messageId = encodeURIComponent(messageId); can this: + let utf8String = this._unicodeConverter.ConvertFromUnicode(string); + utf8String += this._unicodeConverter.Finish(); + return this._xmlEscapeString(utf8String); be just this? : return this._xmlEscapeString(this._unicodeConverter.ConvertFromUnicode(string) + this._unicodeConverter.Finish()); If you can attach a patch with the nits addressed, where appropriate, I can apply the patch and give it a quick test and then mark it r+.

(In reply to comment #8) > similarly, here: (and out of curiosity, is escapeString the wrong thing here?) > - var messageId = msgHdr.messageId; > - // for the trunk, this should work > - // var netUtils = > Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsINetUtil); > - // messageId = netUtils.escapeString(messageId, > netUtils.ESCAPE_URL_PATH); > + let messageId = msgHdr.messageId; > messageId = encodeURIComponent(messageId); I don't know -- what I do know is that encodeURIComponent is the right thing to do here. :) I don't like to fix what isn't broken. I'll attach an updated patch in a moment.

(In reply to comment #8) > can this: > > + let utf8String = this._unicodeConverter.ConvertFromUnicode(string); > + utf8String += this._unicodeConverter.Finish(); > + return this._xmlEscapeString(utf8String); > > be just this? : > > return this._xmlEscapeString(this._unicodeConverter.ConvertFromUnicode(string) > + this._unicodeConverter.Finish()); I'm not sure of operator evaluation order in JS, and if it's right to left then Finish() would get called before ConvertFromUnicode(). I'm leaving this as it is.

Thanks humph for pointing out the duplicate null-check.

One classic =/== mistake, and remnants of an old function. Sigh. David, please try this patch.

js evaluates left to right, but discretion is probably the better part of valor here.

I'm running with this patch - I'm getting this error: Error: this._queueMessage is not a function Source File: file:///Users/davidbienvenu/tbirdhg/obj-i386-apple-darwin9.5.0/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/nsSpotlightIntegration.js Line: 309 I'll see if I can fix it locally and see if indexing works after that.

(In reply to comment #14) > I'm running with this patch - I'm getting this error: > Error: this._queueMessage is not a function > Source File: > file:///Users/davidbienvenu/tbirdhg/obj-i386-apple-darwin9.5.0/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/nsSpotlightIntegration.js > Line: 309 > > I'll see if I can fix it locally and see if indexing works after that. Gah, sorry, this went into the next patch. MQ can be frustrating sometimes... The fix is in msgAdded -- that should read SearchIntegration._queueMessage.

Hopefully there's nothing else that causes trouble.

(In reply to comment #16) > > Hopefully there's nothing else that causes trouble. There is -- the GenerateSupportFile in _onDoneStreaming should become this.startStreaming. What happened was that by mistake I committed these fixes into the next patch in my patch queue. I'll upload the next patch once bienvenu's pointed out any more gaffes in the code.

fix checked in, along with one small additional fix, temp-> text in log call.

sorry, sid says there's more to come.

Thanks David for catching all those problems. This should be very straightforward compared to the last one.

Comment on attachment 352333 [details] [diff] [review] Part 2: turn searchCommon.js into a module, SearchSupport.js The license comment styles have been changed as they were the only thing now making us use the preprocessor.

Comment on attachment 352333 [details] [diff] [review] Part 2: turn searchCommon.js into a module, SearchSupport.js cancelling review -- there seems to be a problem with module scoping and the timer.

This should be better. I have to store a ref to the downlevel object, because "SearchIntegration" now isn't available in SearchSupport's scope. This also means that we aren't tied to a particular object name, as we can always access SearchSupport._downlevel.

Comment on attachment 352413 [details] [diff] [review] The real part 2 The patch is the combination of a global search-and-replace, licence block changes and makefile changes.

So, over IRC, the opinion was that the previous patch was too big a hack. The idea of this patch is to just convert the components into modules. (I also kept the fixed up comments from the previous patch) Tested, works fine on both Vista and Mac.

According to Standard8, there currently isn't a packages-static for Mac, so we're clear there.

why the searchOverlay.js? Is that the get the spotlight/winsearch integration into the jar? Or is it needed for the search integration dialog? other than that question, this looks OK - I'll give it a quick run on my mac...

(In reply to comment #28) > why the searchOverlay.js? > Now that it's a module, we can't use the category manager, so we need to load the module somehow. searchOverlay.<js/xul> does that.

oh, right, it's not a component, it's a module.

I think that should be it as far as this bug is concerned. Thanks David!

Checked in: http://hg.mozilla.org/comm-central/rev/b7d9726d941b Please mark fixed if appropriate.