Add an API similar to Chrome's webNavigation

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch patch (obsolete) — Splinter Review
This needs some tests, but I'd like to get some initial feedback.
Attachment #8614970 - Flags: review?(dtownsend)
Comment on attachment 8614970 [details] [diff] [review]
patch

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

Looking good so far

::: toolkit/modules/addons/WebNavigation.jsm
@@ +24,5 @@
> +  init() {
> +    Services.mm.addMessageListener("Extension:DOMContentLoaded", this);
> +    Services.mm.addMessageListener("Extension:StateChange", this);
> +    Services.mm.addMessageListener("Extension:LocationChange", this);
> +    Services.mm.loadFrameScript("resource://gre/modules/WebNavigationContent.js", true);

If you do this and a tab already has that framescript loaded does it get loaded a second time?

@@ +49,5 @@
> +
> +  removeListener(type, listener) {
> +    let listeners = this.listeners.get(type);
> +    listeners.delete(listener);
> +    if (listeners.size == 0) {

This will throw if an extension double-removes. Maybe that's ok though it's probably a bad exception for the developer to decipher.

@@ +181,5 @@
> +  },
> +  removeListener(listener) {
> +    return Manager.removeListener("onReferenceFragmentUpdated", listener);
> +  }
> +};

A lot of duplication here that you could probably simplify:

for (let event of ["onBeforeNavigate", ...]) {
  WebNavigation[event] = {
    addListener: Manager.addListener.bind(Manager, event),
    removeListener: Manager.removeListener.bind(Manager, event),
  }
}

@@ +205,5 @@
> +
> +  // Not sure how to implement these yet.
> +  //onCreatedNavigationTarget,
> +  //onHistoryStateUpdated,
> +};

Also note getFrame and getAllFrames
Attachment #8614970 - Flags: review?(dtownsend) → feedback+
Posted patch patch v2Splinter Review
OK, here's a new version with a test.
Attachment #8614970 - Attachment is obsolete: true
Attachment #8617685 - Flags: review?(dtownsend)
Comment on attachment 8617685 [details] [diff] [review]
patch v2

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

Looks good. The chrome docs talk about some filtering options for all this but I can't find any examples of it in the docs or examples. Do you know anything about it?
Attachment #8617685 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/7eca38a74be8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.