Closed Bug 467117 Opened 13 years ago Closed 13 years ago

Convert nsSpotlightIntegration and nsWinSearchIntegration to JS modules

Categories

(Thunderbird :: General, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: rain1, Assigned: rain1)

References

Details

Attachments

(2 files, 13 obsolete files)

65.82 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
19.95 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → sid1337
Status: NEW → ASSIGNED
Attached patch WIP, part 1, whitespace ignored (obsolete) — Splinter Review
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #351564 - Attachment is obsolete: true
Attached patch patch v1, whitespace ignored (obsolete) — Splinter Review
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.
Attachment #351567 - Attachment is obsolete: true
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.
Attachment #351601 - Attachment is obsolete: true
Attachment #351603 - Attachment is obsolete: true
Attachment #351794 - Flags: review?(bienvenu)
No changes, but fixes bitrot.
Attachment #351793 - Attachment is obsolete: true
Blocks: 468471
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.
Attachment #351888 - Attachment is obsolete: true
Attached patch v1.13: two small bugfixes (obsolete) — Splinter Review
One classic =/== mistake, and remnants of an old function. Sigh. David, please try this patch.
Attachment #352183 - Attachment is obsolete: true
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.
Attachment #352193 - Attachment is obsolete: true
(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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
sorry, sid says there's more to come.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #352310 - Attachment description: v1.15: even more bugfixes → v1.15: even more bugfixes - checked in, with addition of temp->text
Attachment #351794 - Flags: review?(bienvenu) → review-
Attachment #352310 - Flags: review+
Thanks David for catching all those problems.

This should be very straightforward compared to the last one.
Attachment #351794 - Attachment is obsolete: true
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.
Attachment #352333 - Flags: review?(bienvenu)
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.
Attachment #352333 - Flags: review?(bienvenu)
Attached patch The real part 2 (obsolete) — Splinter Review
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.
Attachment #352333 - Attachment is obsolete: true
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.
Attachment #352413 - Flags: review?(bienvenu)
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.
Attachment #352413 - Attachment is obsolete: true
Attachment #352413 - Flags: review?(bienvenu)
According to Standard8, there currently isn't a packages-static for Mac, so we're clear there.
Attachment #352557 - Attachment is obsolete: true
Attachment #352931 - Flags: review?(bienvenu)
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.
Attachment #352931 - Flags: review?(bienvenu) → review+
I think that should be it as far as this bug is concerned. Thanks David!
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/b7d9726d941b

Please mark fixed if appropriate.
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 3.0b2
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.