561692 - e10s HTTP: Implement nsIProgressEventSink: part1

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

Attachment: WIP

Part 1: have HttpChannelParent implement nsIProgressEventSink (the attached WIP patch does the infrastructure for you).  Have its OnProgress and OnStatus functions send IPDL msgs to the content process, and call OnProgress/OnStatus on mProgressSink, as nsHttpChannel::OnTransportStatus does.

This will be correct, but I think we can wind up coalescing these two msgs with SendOnDataAvail, which will be part 2.  But let's get a functionally correct version first.

This depends on the patch for bug 536292 to get started.

Comment 1

[Jae-Seong Lee-Russo]

Attachment: patch, 1

I can't include a test here because I don't know what & how to test.  Any suggestions, please.

Comment 2

[Jason Duell]

I'm looking over the patch, but am not done reviewing.  Looks good so far.

In the meantime, to test, you should be able to write an xpcshell test that creates a JS object that implements  nsIProgressEventSink (i.e. onProgress + onStatus).  Pass that object to a channel via SetNotificationCallbacks, and it should receive OnProgress and onStatus events. 

Keep a running sum of the "progress" args as OnProgress is called. Make sure they add up to progressMax each time.  In your checkRequest function (or whatever you pass in to your ChannelListener, to be called at OnStopRequest time), make sure that the httpbody.length == last progressMax seen == your running tally of "progress" args.

Make sure OnStatus is called before each OnProgress.  I'm not sure there's much else to do there.  You could check the "status arg" to make sure it matches whatever nsHttpChannel hands out for it (i.e. NS_ConvertUTF8toUTF16(host).get())

Comment 3

[Jae-Seong Lee-Russo]

Attachment: patch 2 w/ test

Comment 4

[Jae-Seong Lee-Russo]

Attachment: patch, 3

Comment 5

[Jason Duell]

Comment on attachment 443166 [details] [diff] [review]
patch, 3

>diff --git a/netwerk/protocol/http/src/HttpChannelParent.cpp b/netwerk/protocol/http/src/HttpChannelParent.cpp
>+NS_IMETHODIMP
>+HttpChannelParent::OnProgress(nsIRequest *aRequest, 
>+                              nsISupports *aContext, 
>+                              PRUint64 aProgress, 
>+                              PRUint64 aProgressMax)
>+{
>+  printf("@@@@@@@@@@: HttpChannelParent::OnProgress: progress=%ld of %ld\n",
>+      (long)aProgress, (long)aProgressMax);

Remove printf.  And you might as well use (unsigned long) FYI

>+// TODO: Remove
>+#include <nsISocketTransport.h>
>+#if 0
>+NS_NET_STATUS_RESOLVING_HOST nsISocketTransport::STATUS_RESOLVING
>+NS_NET_STATUS_CONNECTED_TO   nsISocketTransport::STATUS_CONNECTED_TO
>+NS_NET_STATUS_SENDING_TO     nsISocketTransport::STATUS_SENDING_TO
>+NS_NET_STATUS_RECEIVING_FROM nsISocketTransport::STATUS_RECEIVING_FROM
>+NS_NET_STATUS_CONNECTING_TO  nsISocketTransport::STATUS_CONNECTING_TO
>+NS_NET_STATUS_WAITING_FOR    nsISocketTransport::STATUS_WAITING_FOR
>+#endif
>+#define STATUS_CASE(s) \
>+  case (s): return #s
>+
>+static const char *
>+statusString(nsresult status)
>+{
>+  switch(status) {
>+    STATUS_CASE(nsISocketTransport::STATUS_RESOLVING); //      = 0x804b0003;
>+    STATUS_CASE(nsISocketTransport::STATUS_CONNECTING_TO); //  = 0x804b0007;
>+    STATUS_CASE(nsISocketTransport::STATUS_CONNECTED_TO); //   = 0x804b0004;
>+    STATUS_CASE(nsISocketTransport::STATUS_SENDING_TO); //     = 0x804b0005;
>+    STATUS_CASE(nsISocketTransport::STATUS_WAITING_FOR); //    = 0x804b000a;
>+    STATUS_CASE(nsISocketTransport::STATUS_RECEIVING_FROM); // = 0x804b0006;
>+    STATUS_CASE(nsITransport::STATUS_READING); // 0x804b0008;
>+    STATUS_CASE(nsITransport::STATUS_WRITING); // 0x804b0009;
>+    default:
>+      return "unknown!";
>+  }
>+}

Remove all this too.

>+
>+NS_IMETHODIMP
>+HttpChannelParent::OnStatus(nsIRequest *aRequest, 
>+                            nsISupports *aContext, 
>+                            nsresult aStatus, 
>+                            const PRUnichar *aStatusArg)
>+{
>+  printf("@@@@@@@@@@: HttpChannelParent::OnStatus: %lu (%s): %s\n",
>+         (unsigned long)aStatus, statusString(aStatus),
>+         NS_ConvertUTF16toUTF8(aStatusArg).get());

Remove

>diff --git a/netwerk/test/unit/test_progress.js b/netwerk/test/unit/test_progress.js
>new file mode 100644
>+
>+do_load_httpd_js();
>+
>+var httpserver = new nsHttpServer();
>+var testpath = "/simple";
>+var httpbody = "0123456789";
>+
>+var tally = 0, max = 0, previous = 0;
>+
>+const STATUS_RECEIVING_FROM = 0x804b0006;
>+const LOOPS = 50000;
>+
>+var progressCallback = {
>+  QueryInterface: function (iid) {
>+    if (iid.equals(Ci.nsISupports) || iid.equals(Ci.nsIProgressEventSink))
>+	return this;
>+    throw Cr.NS_ERROR_NO_INTERFACE;
>+  },
>+
>+  getInterface: function (iid) {
>+    if (iid.equals(Ci.nsIProgressEventSink))
>+	return this;
>+    throw Cr.NS_ERROR_NO_INTERFACE;
>+  },
>+
>+  onProgress: function (request, context, progress, progressMax) {
>+    do_check_eq(mStatus, STATUS_RECEIVING_FROM);
>+    tally += (progress - previous);
>+    previous = progress;
>+    max = progressMax;
>+  },

Hmm.  It looks like tally isn't really serving such a useful purpose, in that even if we wind up missing an OnProgress msg, we wouldn't really notice in this test (we'd just see a larger delta between tally and previous).  So I'm not sure we really need anything other than a check that the last progress/progressMax seen both == length*LOOPS.  Let's remove tally, just so it makes reading the test simpler.

+r with those changes.

Comment 6

[Jae-Seong Lee-Russo]

Attachment: patch 4, comment 5

Comment 7

[Jae-Seong Lee-Russo]

Attachment: patch 5, tests were missing in 4

Comment 8

[Jason Duell]

Comment on attachment 444822 [details] [diff] [review]
patch 5, tests were missing in 4

Looks good!

I'm going to fix up a couple of minor things before I check in (so this is just FYI--no need to submit another patch);

>+  if (mProgressSink && NS_SUCCEEDED(mStatus) && mIsPending && !(mLoadFlags & LOAD_BACKGROUND)) {

80 char max length, so break into two lines. 


>+NS_IMETHODIMP
>+HttpChannelParent::OnProgress(nsIRequest *aRequest, 
>+                              nsISupports *aContext, 
>+                              PRUint64 aProgress, 
>+                              PRUint64 aProgressMax)
>+{
>+  SendOnProgress(aProgress, aProgressMax);

I'll add a guard that checks mProtocolValid before sending, as per bug 565223, which I'm going to check in at the same time.

>+NS_IMETHODIMP
>+HttpChannelParent::OnStatus(nsIRequest *aRequest, 
>+                            nsISupports *aContext, 
>+                            nsresult aStatus, 
>+                            const PRUnichar *aStatusArg)
>+{
>+  SendOnStatus(aStatus, nsString(aStatusArg));

Ditto.

Nice work!  Note that part 2 of this bug is less priority than anything hanging off of bug 516730, so get to it only when you've got stream serialization for POSTs working.

Comment 9

[Jason Duell]

http://hg.mozilla.org/projects/electrolysis/rev/93fb3e66d7b9