Overlap tcp/ssl handshakes with typing in search box.

RESOLVED FIXED in seamonkey2.13

Status

SeaMonkey
Search
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Philip Chee, Assigned: Ekanan Ketunuti)

Tracking

Trunk
seamonkey2.13

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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>
(Assignee)

Comment 1

5 years ago
Created attachment 637367 [details] [diff] [review]
v1
Attachment #637367 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Assignee: nobody → ananuti
(Reporter)

Comment 2

5 years ago
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);
(Assignee)

Updated

5 years ago
Attachment #637367 - Flags: review?(neil)
(Assignee)

Comment 3

5 years ago
Created attachment 637450 [details] [diff] [review]
v2
Attachment #637367 - Attachment is obsolete: true
Attachment #637450 - Flags: review?(philip.chee)
(Assignee)

Updated

5 years ago
Attachment #637450 - Flags: review?(neil)
(Reporter)

Comment 4

5 years ago
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+
(Assignee)

Comment 5

5 years ago
Created attachment 637752 [details] [diff] [review]
v3
Attachment #637450 - Attachment is obsolete: true
Attachment #637450 - Flags: review?(neil)
Attachment #637752 - Flags: review?(neil)
Attachment #637752 - Flags: feedback+

Comment 6

5 years ago
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+
(Assignee)

Comment 7

5 years ago
Created attachment 638071 [details] [diff] [review]
patch for check-in, r=neil
Attachment #637752 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Reporter)

Comment 8

5 years ago
Ekanan Ketunuti, Thank you for your patch!
(Reporter)

Comment 9

5 years ago
> 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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
(Assignee)

Comment 10

5 years ago
(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.
(Reporter)

Updated

5 years ago
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.
(Reporter)

Comment 12

5 years ago
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.