795136 - DOM API glue for the network indication API

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

+++ This bug was initially created as a clone of Bug #777445 +++

This bug will take care of writing the DOM glue on top of bug 777445.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - New API without permission checks

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Permissions

More work will have to be done when bug 758269 will land.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 665893 [details] [diff] [review]
Part 1 - New API without permission checks

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

::: dom/base/nsGlobalWindow.cpp
@@ +9049,5 @@
> +    bool dummy;
> +    return DispatchEvent(event, &dummy);
> +  }
> +
> +  if (!nsCRT::strcmp(aTopic, NS_NETWORK_ACTIVITY_BLIP_DOWNLOAD_TOPIC)) {

Nit: I would probably have merged these two if-branches and branched the first InitEvent argument. Up to you though.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 665893 [details] [diff] [review]
Part 1 - New API without permission checks

All the code should be inside some
#ifdef B2G.
We must not expose these onfoo handlers on desktop FF.
(I wonder if you need a new interface to add onfoo handlers to window)


>+void
>+nsGlobalWindow::EnableNetworkEvent(uint32_t aType)
>+{
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+  if (!os) {
>+    NS_ERROR("ObserverService should be available!");
>+    return;
>+  }
>+
>+  switch (aType) {
>+    case NS_NETWORK_UPLOAD_EVENT:
>+      os->AddObserver(mObserver, NS_NETWORK_ACTIVITY_BLIP_UPLOAD_TOPIC, false);
>+      break;
>+    case NS_NETWORK_DOWNLOAD_EVENT:
>+      os->AddObserver(mObserver, NS_NETWORK_ACTIVITY_BLIP_DOWNLOAD_TOPIC, false);
>+      break;
>+  }
>+}
I think you want to pass true here in both cases as the last parameter, so that observer service doesn't
end up keeping window alive.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, given that bug 758269 has landed now. Can you add the new permission to PermissionsTable.jsm rather than just permissions.txt. The permission should be ALLOW_ACTION for certified and DENY_ACTION for everything else.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #4)
Ah, mObserver is passed to AddObserver. Then one needs to call RemoveObserver

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - New API without permission checks

Updated patch that prevent an assert (see bug 795703) and run-time leak.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Tests can't actually work...

Unfortunately, because of bug 795711, tests can't work. I forgot about that because I had a workaround locally to be able to work on the patch.

Comment 9

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37a2a580d614
https://hg.mozilla.org/integration/mozilla-inbound/rev/789babb33800
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b627472f76c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37a2a580d614
https://hg.mozilla.org/mozilla-central/rev/789babb33800
https://hg.mozilla.org/mozilla-central/rev/1b627472f76c