1265847 - Implement browser.history.onVisitRemoved

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

As per https://developer.chrome.com/extensions/history#event-onVisitRemoved

Comment 1

[Bob Silverberg [:bsilverberg]]

Combining this with the bug for onVisited.

Comment 2

[Bob Silverberg [:bsilverberg]]

I'm splitting these two methods into two bugs again, as one is dependent on some native code changes.

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1265847 - Implement browser.history.onVisitRemoved,

Review commit: https://reviewboard.mozilla.org/r/56306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56306/

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8757941 [details]
Bug 1265847 - Implement browser.history.onVisitRemoved,

https://reviewboard.mozilla.org/r/56306/#review53606

::: browser/components/extensions/ext-history.js:61
(Diff revision 1)
> +
> +// WeakMap[Extension -> observer]
> +var observerMap = new WeakMap();
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("startup", (type, extension) => {

This creates an observer for every webextension whether or not the extension cares about history events?  Why not create this on demand only when there is a listener?

::: browser/components/extensions/ext-history.js:135
(Diff revision 1)
>          return Promise.resolve(results);
>        },
> +
> +      onVisitRemoved: new EventManager(context, "history.onVisitRemoved", fire => {
> +        let listener = (event, data) => {
> +          fire(data, true);

I think you want `this.context.runSafe(fire, data)`
also what is the second argument?

::: browser/components/extensions/test/browser/browser_ext_history.js:113
(Diff revision 1)
> +  for (let index of [0, 5, 6]) {
> +    let url = visits[index].uri.spec;
> +    ok(removedUrls.includes(url), `${url} received by onVisitRemoved`);
> +  }

Seems like you should test the exact contents of removedUrls, if the event was triggered too many times for instance, you would have extra entries here but the test would pass.
Also, there should be multiple entries for the base URL, once for each of the deleteRange calls, it would be good to verify that.

Comment 5

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8757941 [details]
Bug 1265847 - Implement browser.history.onVisitRemoved,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56306/diff/1-2/

Comment 6

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/56306/#review53606

> I think you want `this.context.runSafe(fire, data)`
> also what is the second argument?

I wrote this to be similar to what's done in ext-notifications.js [1]. It looks like the implemementation of `fire` in `EventManager` does just what you suggest [2], so it seems like just calling `fire` would do the same.

As for what the second argument is, I have no idea. I must have brought that across from ext-notifications.js. I don't think it's used for anything so I will remove it.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-notifications.js#134
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#624

> Seems like you should test the exact contents of removedUrls, if the event was triggered too many times for instance, you would have extra entries here but the test would pass.
> Also, there should be multiple entries for the base URL, once for each of the deleteRange calls, it would be good to verify that.

I don't think the order of the items in `removedUrls` can be guaranteed, so we probably shouldn't check each entry by index. I did add a check that `removedUrls` is of the length we expect, and I think that combined with checking that each URL we expect to be in there is in fact in there is proof that `onVisitRemoved` was called exactly as expected.

There will not be multiple entries for any URL because `onVisitRemoved` isn't called for each visit, it's only called when all visits for a particular URL are removed. I know this is conterintuitive based on the name (which should really be called `onUrlRemoved`), but it is how it works in Chrome. This was discussed quite a bit in bug 1265845 (comments 8 - 11) [1] if you want to take a look.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1265845#c8

Comment 7

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/56306/#review53606

> I wrote this to be similar to what's done in ext-notifications.js [1]. It looks like the implemementation of `fire` in `EventManager` does just what you suggest [2], so it seems like just calling `fire` would do the same.
> 
> As for what the second argument is, I have no idea. I must have brought that across from ext-notifications.js. I don't think it's used for anything so I will remove it.
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-notifications.js#134
> [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#624

Ah, I overlooked that you're not using SingletonEventManager, can you switch to it (from EventManager)

> I don't think the order of the items in `removedUrls` can be guaranteed, so we probably shouldn't check each entry by index. I did add a check that `removedUrls` is of the length we expect, and I think that combined with checking that each URL we expect to be in there is in fact in there is proof that `onVisitRemoved` was called exactly as expected.
> 
> There will not be multiple entries for any URL because `onVisitRemoved` isn't called for each visit, it's only called when all visits for a particular URL are removed. I know this is conterintuitive based on the name (which should really be called `onUrlRemoved`), but it is how it works in Chrome. This was discussed quite a bit in bug 1265845 (comments 8 - 11) [1] if you want to take a look.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1265845#c8

Oh, how confusing.  Then would it make sense to test that onVisitRemoved doesn't get invoked on your first deleteRange() call (since there are still some visits for that url)?

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8757941 [details]
Bug 1265847 - Implement browser.history.onVisitRemoved,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56306/diff/2-3/

Comment 9

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/56306/#review53606

> Ah, I overlooked that you're not using SingletonEventManager, can you switch to it (from EventManager)

Done!

> Oh, how confusing.  Then would it make sense to test that onVisitRemoved doesn't get invoked on your first deleteRange() call (since there are still some visits for that url)?

Ok, I did that, but I don't think there's a guarantee that it will prove anything. I don't think there is a risk of a failure, but it could be a false positive. That's because I am returning the contents of `removedUrls` after the call to `delete-range` finishes, but there's no guarantee that the `onVisitRemoved` handler would have finished by then, if it were to be called. I'm not really too concerned about that though.

Comment 10

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=548abf3f1888

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8757941 [details]
Bug 1265847 - Implement browser.history.onVisitRemoved,

https://reviewboard.mozilla.org/r/56306/#review54498

looks good, just minor feedback.

::: browser/components/extensions/ext-history.js:57
(Diff revision 3)
> +function getExtensionObserver(extension) {
> +  if (!observerMap.has(extension)) {
> +    let observer = {
> +      onDeleteURI: function(uri, guid, reason) {
> +        this.emit("visitRemoved", {allHistory: false, urls: [uri.spec]});
> +      },
> +      onVisit: function() {},
> +      onBeginUpdateBatch: function() {},
> +      onEndUpdateBatch: function() {},
> +      onTitleChanged: function() {},
> +      onClearHistory: function() {
> +        this.emit("visitRemoved", {allHistory: true});
> +      },
> +      onPageChanged: function() {},
> +      onFrecencyChanged: function() {},
> +      onManyFrecenciesChanged: function() {},
> +      onDeleteVisits: function(uri, time, guid, reason) {
> +        this.emit("visitRemoved", {allHistory: false, urls: [uri.spec]});
> +      },
> +    };
> +    EventEmitter.decorate(observer);
> +    PlacesUtils.history.addObserver(observer, false);
> +    observerMap.set(extension, observer);
> +  }
> +  return observerMap.get(extension);
> +}

I think my original comment wasn't very clear, this will create a separate observer per extension if there are multiple extensions listening for events.  There's nothing extension-specific in the observer though, a single observer could serve all the extensions.
However, I don't think this is a big deal and not worth changing, just wanted to make sure that idea got communicated clearly...

::: browser/components/extensions/test/browser/browser_ext_history.js:17
(Diff revision 3)
> +
> +    browser.history.onVisitRemoved.addListener(data => {
> +      if (data.allHistory) {
> +        historyClearedCount++;
> +      } else {
> +        removedUrls.push(data.urls[0]);

this seems to assume that the length of urls is exactly 1.  either add a test that that's true or use concat()

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8757941 [details]
Bug 1265847 - Implement browser.history.onVisitRemoved,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56306/diff/3-4/

Comment 13

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/56306/#review54498

> I think my original comment wasn't very clear, this will create a separate observer per extension if there are multiple extensions listening for events.  There's nothing extension-specific in the observer though, a single observer could serve all the extensions.
> However, I don't think this is a big deal and not worth changing, just wanted to make sure that idea got communicated clearly...

Yeah, I guess I didn't understand what you were getting at. As long as I needed to update the patch, I figured I might as well make this change too, so I did.

Comment 14

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=691efb04f5ed

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/3445f2c6868e
Implement browser.history.onVisitRemoved. r=aswan

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3445f2c6868e

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3445f2c6868e