Last Comment Bug 767920 - Overlap tcp/ssl handshakes with typing in search box.
: Overlap tcp/ssl handshakes with typing in search box.
Status: RESOLVED FIXED
[good first bug][mentor=Neil][lang=xm...
:
Product: SeaMonkey
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.13
Assigned To: Ekanan Ketunuti
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-25 03:43 PDT by Philip Chee
Modified: 2012-11-17 11:15 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (1.80 KB, patch)
2012-06-27 20:38 PDT, Ekanan Ketunuti
no flags Details | Diff | Splinter Review
v2 (1.94 KB, patch)
2012-06-28 03:39 PDT, Ekanan Ketunuti
philip.chee: feedback+
Details | Diff | Splinter Review
v3 (1.94 KB, patch)
2012-06-28 18:36 PDT, Ekanan Ketunuti
neil: review+
ananuti: feedback+
Details | Diff | Splinter Review
patch for check-in, r=neil (1.81 KB, patch)
2012-06-29 18:55 PDT, Ekanan Ketunuti
no flags Details | Diff | Splinter Review

Description Philip Chee 2012-06-25 03:43:30 PDT
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>
Comment 1 Ekanan Ketunuti 2012-06-27 20:38:02 PDT
Created attachment 637367 [details] [diff] [review]
v1
Comment 2 Philip Chee 2012-06-28 03:01:28 PDT
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);
Comment 3 Ekanan Ketunuti 2012-06-28 03:39:32 PDT
Created attachment 637450 [details] [diff] [review]
v2
Comment 4 Philip Chee 2012-06-28 09:01:48 PDT
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?
Comment 5 Ekanan Ketunuti 2012-06-28 18:36:47 PDT
Created attachment 637752 [details] [diff] [review]
v3
Comment 6 neil@parkwaycc.co.uk 2012-06-29 13:08:20 PDT
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.
Comment 7 Ekanan Ketunuti 2012-06-29 18:55:06 PDT
Created attachment 638071 [details] [diff] [review]
patch for check-in, r=neil
Comment 8 Philip Chee 2012-06-29 20:30:21 PDT
Ekanan Ketunuti, Thank you for your patch!
Comment 9 Philip Chee 2012-06-29 20:52:33 PDT
> 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
Comment 10 Ekanan Ketunuti 2012-06-29 22:48:04 PDT
(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.
Comment 11 neil@parkwaycc.co.uk 2012-07-02 09:25:20 PDT
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.
Comment 12 Philip Chee 2012-07-02 21:28:07 PDT
wouldn't it be easier to just do hg qrefresh; hg export ?
Comment 13 neil@parkwaycc.co.uk 2012-11-17 11:15:00 PST
I only use mq to fix up my tree after I lose a push race with multiple patches.

Note You need to log in before you can comment on or make changes to this bug.