Closed Bug 767920 Opened 9 years ago Closed 9 years ago

Overlap tcp/ssl handshakes with typing in search box.

Categories

(SeaMonkey :: Search, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.13

People

(Reporter: philip.chee, Assigned: ananuti)

Details

(Whiteboard: [good first bug][mentor=Neil][lang=xml/js][level=apprentice])

Attachments

(1 file, 3 obsolete files)

From Firefox Bug 735543:
> When the user focuses on the search box we can try and TCP and/or SSL connect to 
> the search provider if there isn't an existing connection already. That allows 
> the overlap of the connection to happen with the typing - making the connection 
> overhead more or less free.
> 
> The average RTT is around 100ms (with a wide variation). That's 2 or 3 RTT saved 
> for SSL providers, 1 for TCP. So that's a significant latency improvement.

Our code is pretty similar. The SeaMonkey equivalent is somewhere here:
<http://hg.mozilla.org/comm-central/annotate/b3396b2cf032/suite/common/search/search.xml#l456>
<http://mxr.mozilla.org/comm-central/source/suite/common/search/search.xml>
Attached patch v1 (obsolete) — Splinter Review
Attachment #637367 - Flags: review?(neil)
Assignee: nobody → ananuti
Hi! Thanks for your patch. A couple of notes:

> +        var connector =
> +            Services.io.QueryInterface(Components.interfaces.nsISpeculativeConnect);

Suite wrapping style is something like:
var connector = Services.io
                        .QueryInterface(etc).

Also, Suite style is for .XBL not to rely on externally defined Services so you need to do something like:

var connector = Components.classes["@mozilla.org/network/io-service;1"]
                          .getService(Components.interfaces.nsIIOService)
                          .QueryInterface(Components.interfaces.nsISpeculativeConnect);

or perhaps:
var connector = Components.classes["@mozilla.org/network/io-service;1"]
                          .getService(Components.interfaces.nsISpeculativeConnect);
Attachment #637367 - Flags: review?(neil)
Attached patch v2 (obsolete) — Splinter Review
Attachment #637367 - Attachment is obsolete: true
Attachment #637450 - Flags: review?(philip.chee)
Attachment #637450 - Flags: review?(neil)
Comment on attachment 637450 [details] [diff] [review]
v2

Thanks! A couple of final nits:

> +      <handler event="focus">
> +      <![CDATA[
The formatting in this file is rather inconsistent but the other handlers seem to use:
      <handler event="focus"><![CDATA[
Which actually matches your closing:
> +      ]]></handler>

> +        // Speculatively connect to the current engine's search URI (and
> +        // suggest URI, if different) to reduce request latency
Period at the end of a sentence please.

Note to Neil: Should we have a preference to control this with the default being off?
Attachment #637450 - Flags: review?(philip.chee) → feedback+
Attached patch v3 (obsolete) — Splinter Review
Attachment #637450 - Attachment is obsolete: true
Attachment #637450 - Flags: review?(neil)
Attachment #637752 - Flags: review?(neil)
Attachment #637752 - Flags: feedback+
Comment on attachment 637752 [details] [diff] [review]
v3

>+        var connector = Components.classes["@mozilla.org/network/io-service;1"]
>+                                  .getService(Components.interfaces.nsIIOService)
>+                                  .QueryInterface(Components.interfaces.nsISpeculativeConnect);
Nit: No need to use both getService and QueryInterface, just
getService(Components.interfaces.nsISpeculativeConnect) will do.
Attachment #637752 - Flags: review?(neil) → review+
Attachment #637752 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed
Ekanan Ketunuti, Thank you for your patch!
> Created attachment 638071 [details] [diff] [review]
> patch for check-in, r=neil
This patch didn't apply because you edited it manually. I know it's tempting but please stick to hg qrefesh.
I've fixed this for you before pushing..

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/4b0c1a50b836
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(In reply to Philip Chee from comment #9)
> This patch didn't apply because you edited it manually. I know it's tempting
> but please stick to hg qrefesh.

Sorry about that.
Target Milestone: --- → seamonkey2.13
Comment on attachment 638071 [details] [diff] [review]
patch for check-in, r=neil

>@@ -475,16 +475,35 @@
-475: Indicates the position in the original file
,16: Indicates the number of lines showing from the original file
(in this case, 2x8 lines of context, as there are no deletions or holes)
+475: Indicates the position in the updated file
(always the same for the first hunk in that file)
,35: Indicates the number of lines showing from the updated file
But you actually removed a line, so there are only 34 lines showing now ;-)

Alternatively, editors exist that will update the patch lines for you.
wouldn't it be easier to just do hg qrefresh; hg export ?
I only use mq to fix up my tree after I lose a push race with multiple patches.
You need to log in before you can comment on or make changes to this bug.