Closed Bug 1171256 Opened 5 years ago Closed 5 years ago

Add an API similar to Chrome's webNavigation

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: billm, Assigned: billm)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Attached 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+
Attached 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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.