513393 - Add Necko IPDL initialization code

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jason Duell]

Attachment: Patch that bootstraps necko IPDL

Here's a patch that bootstraps a necko IPDL protocol.

The protocol just has a couple of dummy placeholder "ping/pong" methods.

Boris: the only thing that I'd especially like your feedback on is whether you think I call InitNeckoChildProtocol() in the right place.  I was going to do it in nsIOService::Init(), but that's too early for IPDL.  So instead I call it the first time either of the HTTP or ftp protocols is requested.  Is that OK?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why is it too early?  What does IPC expect to be around that isn't at that point?

Do we need to bootstrap the IPC stuff before calling GetService on the protocol handlers, or could those protocol handlers' Init() methods bootstrap it as needed?

Comment 2

[Jason Duell]

Attachment: Same version, with comment and a couple nits fixed.

So cjones and I poked around long enough to find out that getting Necko IPC to be bootstrapped within nsIOService::Init() is a fair amount of work.  So for now punt and use my original hack of doing the init the first time http/ftp is used.

Added TODO comment in the code, and fixed some makefile changes that weren't needed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

What about my last question in comment 1?

Comment 4

[Benjamin Smedberg]

Comment on attachment 398015 [details] [diff] [review]
Same version, with comment and a couple nits fixed.

I'm no necko peer, but I'd kinda prefer the mozilla::network or mozilla::necko namespace to mozilla::netwerk.

ContentProcessParent should use 2-space indentation, please don't use 4-space in new code (this was decided recently in the newsgroups).

I'm a little concerned about including chromium-config.mk *after* rules.mk, although if it works maybe that's good enough.

As for initialization, why not just put a method nsIOService::GetIPCChild which is called from the HTTP channel constructors or somewhere equivalent? It can initialize gNeckoChild if it's not already initialized.

* don't use __ suffixes on the include guards: those are technically reserved for the compiler

* do use the full include name for the include-guard, e.g.

#ifndef mozilla_netwerk_NeckoParent_h
#define mozilla_netwerk_NeckoParent_h

I don't think neckoipc_s should link into libxul directly: link it into necko instead (netwerk/build/Makefile.in)

Comment 5

[Jason Duell]

> Do we need to bootstrap the IPC stuff before calling GetService on the protocol handlers, or could those protocol handlers' Init() methods bootstrap it as needed?

I pinged cjones one more time to see if the above idea unlocked some possibility he hadn't thought of before.  He tells me there's no inherent reason we couldn't get earlier IPC init to work, but it would involve fiddling with chromium's init.  But there's no particular reason why we actually need necko initialized so early, and I don't want to waste cjones's cycles on early init unless we actually need it.  And we don't.  So I still suggest we just lazily init at protocol load time.  

> why not just put a method nsIOService::GetIPCChild which
is called from the HTTP channel constructors or somewhere equivalent? It can
initialize gNeckoChild if it's not already initialized.

Yes, we could do that.  I don't see why it's any better than doing it at protocol load time, though, and it's slightly less efficient (lots of needless "if" statement hits).

> I'd kinda prefer the mozilla::network or mozilla::necko
namespace to mozilla::netwerk

After IRC chat, we're going with "mozilla::net"

Other suggestions sound fine, and will be implemented.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> He tells me there's no inherent reason we couldn't get earlier IPC init to work,

I'm talking about _later_ IPC init here, not earlier.  You're doing IPC init in GetProtocolHandler.  I'm suggesting doing it in nsHTTPProtocolHandler::Init and nsFTPProtocolHandler::Init (or the constructors if this is infallible and there are no init methods and you don't want to add any).  That avoids polluting GetProtocolHandler with protocol specifics and lets other protocols do the same thing as needed.

Comment 7

[Benjamin Smedberg]

Is there any reason to do it when we get the protocol handler, even? We don't actually need IPC until we start loading channels, right?

Anyway, I fully agree it should be in the protocol handler initializer if not later, and not in GetProtocolHandler.

Comment 8

[Jason Duell]

Attachment: Necko e10s init patch, with fixes and PHttpChannel subprotocol

New patch:

(Boris:  again, all I really need from you is sign-off on where I've moved the Necko protocol init to.)

- init Necko IPDL in HTTP Protocol manager ::Init(), as bz suggested.

- bsmedberg's comments incorporated.

- Added 'PHttpChannel' sub-protocol of Necko protocol. 

- Added/corrected modelines for emacs/vim in all dom/ipc/* files, as per https://developer.mozilla.org/En/Developer_Guide/Coding_Style.   Since 95% of the code in the existing ContentProcess files uses 4-space indents rather than 2, I put "4" in the modelines for now.  We should convert these files to 2-space indenting.  New files in this patch are using 2-space indents.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 399888 [details] [diff] [review]
Necko e10s init patch, with fixes and PHttpChannel subprotocol

Necko init part looks fine.

Comment 10

[Benjamin Smedberg]

Comment on attachment 399888 [details] [diff] [review]
Necko e10s init patch, with fixes and PHttpChannel subprotocol

>diff --git a/dom/ipc/ContentProcessChild.cpp b/dom/ipc/ContentProcessChild.cpp

>-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>-/* vim: sw=4 ts=4 et : */
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* vim: set sw=4 ts=8 et tw=80 : */
>+

Here and everywhere else: please leave tab-width: 8 so that it's easier to catch when tabs have crept into the file. And skip the extra line between the modelines and the license header.

>diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp

>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* vim: set sw=4 ts=8 et tw=80 : */

You added an extra modeline here... probably a merge conflict, but please fix.

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp

duplicate modelines in this file as well

Comment 11

[Jason Duell]

Attachment: patch with modeline fixes, and pre-commited Makefile tier changes removed [Checkin: Comment 13]

Benjamin,

Here's a patch with the dupe modelines and the extra spaces removed.  I also removed the makefile tier stuff that we committed the other day.

One thing I'm confused about:

>-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>-/* vim: sw=4 ts=4 et : */
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* vim: set sw=4 ts=8 et tw=80 : */
>+

>Here and everywhere else: please leave tab-width: 8 so that it's easier to
>catch when tabs have crept into the file. 

The code above changes the tab-width's to 8 from 4.  So I'm not sure what you want here.  The style guide says to use 8, and that's what's still in the patch.

I assume you can sign off on r+sr at this point, since bz already did his 'r' for the last patch and nothing has really changed.

Comment 12

[Benjamin Smedberg]

Comment on attachment 400822 [details] [diff] [review]
patch with modeline fixes, and pre-commited Makefile tier changes removed
[Checkin: Comment 13]

I didn't need to re-review, but sure!

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/83d438dfacf9 on behalf of jduell

Comment 14

[Serge Gautherie (:sgautherie)]

Where else do you want this to land?

Comment 15

[Jason Duell]

This only needed to land in electrolysis-land.