We need to forward online and offline events from chrome to content.

Doug, please point out the relevant code files/functions where this happens, and I'll try to get someone on this ASAP.

I'm starting to be very familiar with forwarding stuff over IPC... I can tackle this if you want.

+1

hehe... I don't exactly know what your "+1" refers to, but in Comment 2, I meant I could tackle this bug, once you're finished pointing out the relevant code files/functions where it happens... just wanted to make sure we understand each other, thanks.

Frederic, I mentioned this to Jae-Seong as a possible bug for him to work on, but if you have cycles free, you could take it. Perhaps you can work that out between yourselves. I'm sure there's enough necko work to go around...

Assigning to Frederic--I'm giving bug 555514 to Jae-Seong. Dougt: please point us at the function/file where online/offline events are currently send.

Comment on attachment 435935 [details] initial planning Looks fine. I think we might have to get rid of the two notifications: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#154 I am worried that they will have synchronization issues. The listeners (in C++) are few and they probably can just use this new event.

(In reply to comment #8) >I think we might have to get rid of the two notifications: > > I am worried that they will have synchronization issues. The listeners (in > C++) are few and they probably can just use this new event. atm, we've got : - 2 NS_IOSERVICE_GOING_OFFLINE_TOPIC listeners (nsDownloadManager and nsXPInstallManager). - 2 NS_IOSERVICE_OFFLINE_STATUS_TOPIC listeners (nsDownloadManager and nsGlobalWindow that fires DOM online/offline events) I'm not sure I understand your sync concern here... Do you mean you're concerned about chrome and content offline states somehow getting out of sync ?

after thinking about it a bit more, it isn't a worry. Those consumers are in the chrome process and can continue doing what they are doing. for e10s, there will only be one event which broadcasts offline state information to content. My worry was that if we had to propagate the "i am about to go offline" event to content, content really couldn't do anything since by the time it got the event, chrome probably would already be offline.

Comment on attachment 435935 [details] initial planning agreed! So will patch soon and r? somebody... Jduell ?

Have both dougt and I review. I don't know this code at all, so I'm mainly useful for general e10s review.

Comment on attachment 436253 [details] [diff] [review] patch v.1 >diff --git a/dom/ipc/ContentProcessChild.cpp b/dom/ipc/ContentProcessChild.cpp >--- a/dom/ipc/ContentProcessChild.cpp >+++ b/dom/ipc/ContentProcessChild.cpp >@@ -152,6 +152,17 @@ ContentProcessChild::RecvRegisterChrome( > return true; > } > >+bool >+ContentProcessChild::RecvSetOffline(const PRBool& offline) >+{ >+ nsCOMPtr<nsIIOService> io (do_GetIOService()); >+ if (io) { >+ io->SetOffline(offline); >+ } Lose the curly braces. NS_ASSERTION(io, "IO Service can not be null"); I am not sure you really need to test for |io|. >diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp >--- a/dom/ipc/ContentProcessParent.cpp >+++ b/dom/ipc/ContentProcessParent.cpp >@@ -61,6 +61,8 @@ PRBool gSingletonDied = PR_FALSE; > namespace mozilla { > namespace dom { > >+#define NS_IOSERVICE_SET_OFFLINE_TOPIC "history-notify-visited" This is probably the wrong string for this topic. why not "offline-topic-changed" or something like that? This is also defined below in a header? >+ >+#ifdef MOZ_IPC >+ if (XRE_GetProcessType() == GeckoProcessType_Default) { >+ nsCOMPtr<nsIObserverService> observerService = >+ do_GetService(NS_OBSERVERSERVICE_CONTRACTID); >+ if (observerService) { >+ (void)observerService->NotifyObservers(aURI, >+ HISTORY_NOTIFY_VISITED_TOPIC, >+ offline ? >+ NS_LITERAL_STRING("true").get() : >+ NS_LITERAL_STRING("false").get()); not sure about the rest of the file, but, in general, you should line up parameters to this method call. Also, add an NS_ASSERTION() We should always have the observer service.

Patch corrected per review comments

Comment on attachment 437034 [details] [diff] [review] patch v.2 drop the whitespace at the end of: + parent, NS_IOSERVICE_SET_OFFLINE_TOPIC, PR_FALSE); + NS_IOSERVICE_SET_OFFLINE_TOPIC, offline ? I tested this in Fennec on device using: http://starkravingfinkle.org/projects/offline/todo.html And did not see the expected result. Any ideas?

> I tested this in Fennec on device using: > > http://starkravingfinkle.org/projects/offline/todo.html > > And did not see the expected result. Any ideas? What were you looking for as an expected behavior for this on Fennec ? I'm afraid me don't currently have any offline/online support on the fennec chrome, right?

hey fred23, basically I disconnect the phone from the connected network. this (on non-e10s builds) fires an offline notification which content can register for: addEventListener("online", function...

Comment on attachment 437034 [details] [diff] [review] patch v.2 okay... the problem here is that the #defines do not match: +#define NS_IOSERVICE_SET_OFFLINE_TOPIC "network:set-offline" + +#define NS_IOSERVICE_SET_OFFLINE_TOPIC "ioservice-set-offline" + If they do match, things work as expected. Given that you want to make this a private event between the for e10s purposes, how about: #define NS_E10S_IOSERVICE_SET_OFFLINE_TOPIC "e10s:network:set-offline" Fred, can you fix this up and post a new patch?

fwiw, to test on the desktop, just change one of the ui handlers (like the forward button), to do something like: let (ios = Cc["@mozilla.org/network/io-service;1"] .getService(Ci.nsIIOService2)) { let a = ios.offline; ios.manageOfflineStatus = !a; ios.offline = !a; }

I'll push as soon as trees are open, since it's working on desktop.

Comment on attachment 444153 [details] [diff] [review] patch - with the #define change all new instances of NS_IOSERVICE_SET_OFFLINE_TOPIC should be NS_E10S_IOSERVICE_SET_OFFLINE_TOPIC

patch with last comments from Doug addressed.

Pushed: http://hg.mozilla.org/projects/electrolysis/rev/26c6fe18490a

(In reply to comment #19) > #define NS_E10S_IOSERVICE_SET_OFFLINE_TOPIC "e10s:network:set-offline" Left field comment, but why introduce "e10s" into the source code? Isn't this "ipc" ?

Good catch from the left field : Makes sense. Doug ?

yeah mfinkle is right. don't introduce project names. frederic, can you push a change?

Pushed this quick fix : http://hg.mozilla.org/projects/electrolysis/rev/31d2d548061a