Closed Bug 558623 Opened 10 years ago Closed 6 years ago

e10s HTTP: combine PHttpChannel constructor with SendAsyncOpen/SendRedirect1Begin

Categories

(Core :: Networking: HTTP, defect)

Other Branch
x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)

References

Details

Attachments

(2 files, 1 obsolete file)

We could save an IPDL message per-channel by combining the PHttpChannel construction with AsyncOpen.  See HttpChannelChild::AsyncOpen.

Not a huge win, but low-hanging fruit.
Yoink.
Assignee: nobody → josh
This is not quite as simple as it appears at first.  I took a stab at it, then got bit by the whole nsHttp.h/chromium LOG macro mess again.  I don't really feel like trying to sort that out right now, so I'm removing this from my plate.
Assignee: josh → nobody
Depends on: 536321
This should still be possible.  Note that we'll want to combine both AsyncOpen (for child->parent construction) and SendRedirect1Begin (for parent-> child construction during redirects) into one constructor (since IPDL doesn't allow multiple constructors).   We can do this most easily by using an IPDL union that contains two structs, one the args that AsyncOpen needs, and the other than SendRedirect1Begin needs.  See the TestDataStructures example in ipc/ipdl/tests/cxx.

For info on how to deal with the chromium LOG issue, see bug 545995 and the top of nsHttp.h
Summary: e10s HTTP: combine IPDL constructor and SendAsyncOpen → e10s HTTP: combine PHttpChannel constructor with SendAsyncOpen/SendRedirect1Begin
I hit a higher amount of silly C++ macro/typedef/namespace/etc issues in this patch than any other I can remember writing.  This patch fixes one of them:  we'd been keeping our (kludgy) LOG() macro definitions in nsHttp.h, but the tortuous logic I implemented to ensure we don't conflict with the chromium LOG macro blew up once the IPDL parser needed to see nsHttp.h. (Also we don't want anything but files in /netwerk/protocol/http to see the LOG() macro since it's http specific).  So I split out the log stuff into its own file.  

Note I hg cloned nsHttp.h->HttpLog.h so even though it's a new file you may see deletions in it (not sure how smart splinter will be about that).
Assignee: nobody → jduell.mcbugs
Attachment #763933 - Flags: review?(josh)
Attached patch part2: main patch, v1 (obsolete) — Splinter Review
My favorite part is where I ran into some X11 header doing "#define Status int" (hello global namespace pollution!).  Glad I remembered my old pal "g++ -E -dD"
Attachment #763934 - Flags: review?(josh)
Needed to remove a MOZ_OVERRIDE.
Attachment #763934 - Attachment is obsolete: true
Attachment #763934 - Flags: review?(josh)
Attachment #763992 - Flags: review?(josh)
Comment on attachment 763933 [details] [diff] [review]
part1: split out HTTP LOG() stuff into separate file

Review of attachment 763933 [details] [diff] [review]:
-----------------------------------------------------------------

I recall hitting this same problem when I last attempted this and giving up in disgust. You are a better man than I.
Attachment #763933 - Flags: review?(josh) → review+
Last build seemed to be hitting bustage on inbound.  Here's one off of m-c:

https://tbpl.mozilla.org/?tree=Try&rev=aa32905a019c
Comment on attachment 763992 [details] [diff] [review]
part2: main patch, v2 (one line fix)

Review of attachment 763992 [details] [diff] [review]:
-----------------------------------------------------------------

This is a... special patch. It felt like the code was cleaner before, but fewer messages is a win. So it goes.

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +180,5 @@
>    SerializeInputStream(mUploadStream, uploadStream);
>  
> +  FTPChannelOpenArgs openArgs;
> +  // Gross: no access to FTPChannelOpenArgs members, but they each have a
> +  // function with the struct name that returns a ref.  Yuk.

This is the bed we have made; there's no particular need to grouse about it here.

@@ +187,5 @@
> +  openArgs.entityID() = mEntityID;
> +  openArgs.uploadStream() = uploadStream;
> +
> +  gNeckoChild->SendPFTPChannelConstructor(this, tabChild,
> +                IPC::SerializedLoadContext(this), openArgs);

Indent this to match.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1059,5 @@
>    }
>  
> +  HttpChannelOpenArgs openArgs;
> +  // Gross: no access to HttpChannelOpenArgs members, but they each have a
> +  // function with the struct name that returns a ref.

Preach it, brother.

::: netwerk/protocol/http/HttpChannelParent.h
@@ +55,5 @@
> +  // used to connect redirected-to channel in parent with just created
> +  // ChildChannel.  Used during redirects.
> +  virtual bool ConnectChannel(const uint32_t& channelId);
> +
> +  virtual bool DoAsyncOpen(  const URIParams&           uri,

These two don't need to be virtual.
Attachment #763992 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/312d1be1a95a
https://hg.mozilla.org/mozilla-central/rev/f22c958c6187
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.