Closed Bug 795136 Opened 8 years ago Closed 8 years ago

DOM API glue for the network indication API

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: mounir, Assigned: mounir)

References

(Depends on 1 open bug)

Details

(Keywords: feature, Whiteboard: [LOE:S][WebAPI:])

Attachments

(3 files, 1 obsolete file)

+++ 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.
Whiteboard: [LOE:L][WebAPI:P] → [LOE:L][WebAPI:]
Whiteboard: [LOE:L][WebAPI:] → [LOE:S][WebAPI:]
Attachment #665893 - Flags: superreview?(jonas)
Attachment #665893 - Flags: review?(bugs)
More work will have to be done when bug 758269 will land.
Attachment #665898 - Flags: review?(jonas)
Whiteboard: [LOE:S][WebAPI:] → [LOE:S][WebAPI:][needs review]
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.
Attachment #665893 - Flags: superreview?(jonas) → superreview+
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.
Attachment #665893 - Flags: review?(bugs) → review+
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.
(In reply to Olli Pettay [:smaug] from comment #4)
Ah, mObserver is passed to AddObserver. Then one needs to call RemoveObserver
Updated patch that prevent an assert (see bug 795703) and run-time leak.
Attachment #665893 - Attachment is obsolete: true
Attachment #666317 - Flags: review?(bugs)
Attachment #666317 - Flags: review?(bugs) → review+
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.
Attachment #666325 - Flags: review?(bzbarsky)
Attachment #666325 - Flags: review?(bzbarsky)
Depends on: 795715
Depends on: 795716
Depends on: 795725
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.