Closed Bug 1406594 Opened 4 years ago Closed 4 years ago

history.search and history.getVisits cannot find redirects and "manual_subframe" entries because they are "hidden" entries

Categories

(WebExtensions :: General, enhancement, P5)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox57 wontfix, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: emk, Assigned: emk)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1406594 - Fix history.search and history.getVisited so that they find hidden items correctly.
59 bytes, text/x-review-board-request
aswan
: review+
Details 
I found this when I was trying to port Link Status Redux to WE. Chrome does not have this problem.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment on attachment 8916196 [details]
Bug 1406594 - Fix history.search and history.getVisited so that they find hidden items correctly.

https://reviewboard.mozilla.org/r/187440/#review192790

Bob can you take a first pass at reviewing this?
Attachment #8916196 - Flags: review?(aswan) → review?(bob.silverberg)
status-firefox57: --- → wontfix
Priority: -- → P5
> Priority: -- → P5

Does it mean my review request will be postponed indefinitely? Quite disappointing.
Flags: needinfo?(amckay)
(In reply to Masatoshi Kimura [:emk] from comment #6)
> > Priority: -- → P5
> 
> Does it mean my review request will be postponed indefinitely? Quite
> disappointing.

No, it does not mean that. I will endeavour to do an initial review today.
Flags: needinfo?(amckay)
Comment on attachment 8916196 [details]
Bug 1406594 - Fix history.search and history.getVisited so that they find hidden items correctly.

This looks good to me, but I want to check with Marco to make sure there isn't a reason that we are currently ignoring hidden items for the history API.

Marco, do you think it's fine to include hidden items in history.search and history.getVisits?
Flags: needinfo?(mak77)
Comment on attachment 8916196 [details]
Bug 1406594 - Fix history.search and history.getVisited so that they find hidden items correctly.

https://reviewboard.mozilla.org/r/187440/#review193582

Thanks for the patch! The code looks good to me. I've asked Marco to confirm that this is something we want to do for the history API.
Attachment #8916196 - Flags: review?(bob.silverberg) → review+
Comment on attachment 8916196 [details]
Bug 1406594 - Fix history.search and history.getVisited so that they find hidden items correctly.

This will need a final review from aswan before landing, which should also wait for a response from Marco.
Attachment #8916196 - Flags: review?(aswan)
(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> This looks good to me, but I want to check with Marco to make sure there
> isn't a reason that we are currently ignoring hidden items for the history
> API.

The history results API by default exclude hidden visits for a couple reasons:
1. they are not much useful to the user, our history UI tries to avoid showing data that doesn't have value. And so far the searches were mostly used to populate the UI so the default was set for that.
2. fetching, returning and showing less entries is cheaper, in general.

I don't have a problem with an API returning these visits, they exist.

A separate question though, could getVisits use PlacesUtils.history.fetch instead, with the includeVisits option? That API is async and thus better, though it may be missing some of the properties you are looking for. In particular visitId and referringVisitId. We could expose that info, even if I was honestly trying to stop exposing visit ids. An API should never expose raw database details. And I don't see where these ids are used by the chrome.history API, I cannot find any method that takes a VisitItem or a visit id. A visit can be completely identified by url, transition and time.
Btw, it's a possibility we can evaluate.
Flags: needinfo?(mak77)
Comment on attachment 8916196 [details]
Bug 1406594 - Fix history.search and history.getVisited so that they find hidden items correctly.

https://reviewboard.mozilla.org/r/187440/#review193808

I don't know what "hidden history entries" are which makes this hard to evaluate.  In particular, would this change in behavior possibly break existing extensions?
For the patch itself, there are a few line-item comments below but more importantly, please don't test the history api inside the webnavigation test, add a new test case to test_ext_history.js or even a new test_ext_history_reload.js if necessary.

::: browser/components/extensions/test/xpcshell/test_ext_history.js:437
(Diff revision 3)
> +    browser.test.onMessage.addListener((msg, url) => {
> +      switch (msg) {
> +        case "search":
> +          browser.history.search({text: "", startTime: new Date(0)}).then(results => {
> +            browser.test.sendMessage("search-result", results);
> +          });
> +          break;
> +        case "get-visits":
> +          browser.history.getVisits({url}).then(results => {
> +            browser.test.sendMessage("get-visits-result", results);
> +          });
> +          break;
> +      }
> +    });

please make the handler async and use await instead of .then

::: browser/components/extensions/test/xpcshell/test_ext_history.js:459
(Diff revision 3)
> +
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: ["history"],
> +    },
> +    background: `(${background})()`,

just `background,` should be adequate here

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html:133
(Diff revision 3)
>      for (let event of EVENTS) {
>        listeners[event] = gotEvent.bind(null, event);
>        browser.webNavigation[event].addListener(listeners[event]);
>      }
>  
> +    browser.test.onMessage.addListener((msg, url) => {

same comment as above

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html:133
(Diff revision 3)
>      for (let event of EVENTS) {
>        listeners[event] = gotEvent.bind(null, event);
>        browser.webNavigation[event].addListener(listeners[event]);
>      }
>  
> +    browser.test.onMessage.addListener((msg, url) => {
Attachment #8916196 - Flags: review?(aswan) → review-
(In reply to Marco Bonardo [::mak] from comment #11)
> (In reply to Bob Silverberg [:bsilverberg] from comment #8)
> > This looks good to me, but I want to check with Marco to make sure there
> > isn't a reason that we are currently ignoring hidden items for the history
> > API.
> 
> The history results API by default exclude hidden visits for a couple
> reasons:
> 1. they are not much useful to the user, our history UI tries to avoid
> showing data that doesn't have value. And so far the searches were mostly
> used to populate the UI so the default was set for that.
> 2. fetching, returning and showing less entries is cheaper, in general.
> 
> I don't have a problem with an API returning these visits, they exist.
> 
> A separate question though, could getVisits use PlacesUtils.history.fetch
> instead, with the includeVisits option? That API is async and thus better,
> though it may be missing some of the properties you are looking for. In
> particular visitId and referringVisitId. 

Masatoshi, can you look into using PlacesUtils.history.fetch for getVisits as Marco suggests?

> We could expose that info, even if
> I was honestly trying to stop exposing visit ids. An API should never expose
> raw database details. And I don't see where these ids are used by the
> chrome.history API, I cannot find any method that takes a VisitItem or a
> visit id. A visit can be completely identified by url, transition and time.
> Btw, it's a possibility we can evaluate.

Not having looked at the code, I would guess we use them internally in some way, rather than deliver them to the consumer of our API.
Flags: needinfo?(VYV03354)
(In reply to Andrew Swan [:aswan] from comment #12)
> Comment on attachment 8916196 [details]
> Bug 1406594 - Fix history.search and history.getVisited so that they find
> hidden items correctly.
> 
> I don't know what "hidden history entries" are which makes this hard to
> evaluate.  In particular, would this change in behavior possibly break
> existing extensions?

I'm not really clear on what the value of these entries are either. Masatoshi, can you explain the use cases that require access to these hidden entries?

I assume that extensions can filter these out if they wish, but following on Andrew's second comment, might this change break some existing extensions until they have a chance to add code to filter the entries out?
to be clear, using fetch is not something we absolutely need for this bug, my question was more intended as a follow-up. I'm fine either way.
(In reply to Bob Silverberg [:bsilverberg] from comment #13)
> Masatoshi, can you look into using PlacesUtils.history.fetch for getVisits
> as Marco suggests?

visitId and referringVisitId are part of Chrome extension API[1].
We should consider more carefully before we introduce incompatibility with Chrome. Even if we decided to drop them, a follow-up would be better as Marco said.

(In reply to Bob Silverberg [:bsilverberg] from comment #14)
> I'm not really clear on what the value of these entries are either.
> Masatoshi, can you explain the use cases that require access to these hidden
> entries?

* Chrome extensions API chrome.history has no concept of "hidden entries". It does not differentiate redirects with other entries. We should not expose our internal details.
* The current behavior is inconsistent with the documentation. The documentation mentions "manual_subframe"[2], But the current implementation will never return "manual_subframe" because the TRANSITION_FRAMED_LINK transition type is "hidden".
* The current behavior is inconsistent with Chrome. It will surprise extension developers from Chrome.
* This behavior actually breaks my extension[3]. Even worse, I can not work around the problem because I can not fabricate non-existing search result. On the other hand, it is possible to filter out extra entries.

> I assume that extensions can filter these out if they wish, but following on
> Andrew's second comment, might this change break some existing extensions
> until they have a chance to add code to filter the entries out?

As I said above, the current behavior would rather break extension authors' expectation.

[1] https://developer.chrome.com/extensions/history#type-VisitItem
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/history/TransitionType
[3] https://addons.mozilla.org/ja/firefox/addon/link-status-we/
Flags: needinfo?(VYV03354)
Comment on attachment 8916196 [details]
Bug 1406594 - Fix history.search and history.getVisited so that they find hidden items correctly.

https://reviewboard.mozilla.org/r/187440/#review193808

> I don't know what "hidden history entries" are which makes this hard to evaluate.  In particular, would this change in behavior possibly break existing extensions?

See comment 16. We should not hesitate to fix a bug just because some extensions may depend on the bug, especially when it is incompatible with Chrome.

> For the patch itself, there are a few line-item comments below but more importantly, please don't test the history api inside the webnavigation test, add a new test case to test_ext_history.js or even a new test_ext_history_reload.js if necessary.

It is impossible because synthesized redirect entries are not "hidden", so the test will not test what it supposed to test.
I added a mochitest under browser/components/extensions/test/ instead.

> please make the handler async and use await instead of .then

Done.

> just `background,` should be adequate here

OK.

> same comment as above

Fixed and moved into a new test.
Thanks for responding to all of the questions and for the new patch, Masatoshi. As both you and Marco suggested, can you please open a follow-up bug, which depends on this bug, to investigate using PlacesUtils.history.fetch for getVisits?
Blocks: 1408364
(In reply to Bob Silverberg [:bsilverberg] from comment #19)
> Thanks for responding to all of the questions and for the new patch,
> Masatoshi. As both you and Marco suggested, can you please open a follow-up
> bug, which depends on this bug, to investigate using
> PlacesUtils.history.fetch for getVisits?

Filed bug 1408364.
Comment on attachment 8916196 [details]
Bug 1406594 - Fix history.search and history.getVisited so that they find hidden items correctly.

https://reviewboard.mozilla.org/r/187440/#review195114

r=me with comments addressed

::: browser/components/extensions/test/mochitest/test_ext_history_redirect.html:15
(Diff revision 5)
> +<body>
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +const REDIRECT = new URL("redirection.sjs", location).href;

please call this REDIRECT_URL for clarity

::: browser/components/extensions/test/mochitest/test_ext_history_redirect.html:67
(Diff revision 5)
> +  let win = window.open(REDIRECT);
> +  await waitForLoad(win);

can you just use BrowserTestUtils.withNewTab() for this?  you'll have to make it a browser test instead of a mochitest but I think that's fine.

::: browser/components/extensions/test/xpcshell/test_ext_history.js:413
(Diff revision 5)
> +    // EMBED visits will not go through the database.
> +    // ["auto_subframe", Ci.nsINavHistoryService.TRANSITION_EMBED],

I don't understand this comment.  If its about something that isn't relevant in Firefox, please just remove it.

::: browser/components/extensions/test/xpcshell/test_ext_history.js:431
(Diff revision 5)
> +  let visitDate = new Date(1999, 9, 9, 9, 9).getTime();
> +  for (let [, transitionType] of TRANSITIONS) {
> +    pageInfos.push({
> +      url: VISIT_URL_PREFIX + transitionType + "/",
> +      visits: [
> +        {transition: transitionType, date: new Date(visitDate -= 1000)},

nit: do you need to change the date here?  it isn't ever looked at, can you just use a fixed value?
Attachment #8916196 - Flags: review?(aswan) → review+
Comment on attachment 8916196 [details]
Bug 1406594 - Fix history.search and history.getVisited so that they find hidden items correctly.

https://reviewboard.mozilla.org/r/187440/#review195114

> please call this REDIRECT_URL for clarity

Fixed.

> can you just use BrowserTestUtils.withNewTab() for this?  you'll have to make it a browser test instead of a mochitest but I think that's fine.

I tried to port, but it did not fail even without includeHidden changes. So the browser test did not work correctly somehow.
I kept mochitest at the moment.

> I don't understand this comment.  If its about something that isn't relevant in Firefox, please just remove it.

Updated the comment.

> nit: do you need to change the date here?  it isn't ever looked at, can you just use a fixed value?

Needed because the search result will be sorted in descending chlonological order.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/f1146d5beca8
Fix history.search and history.getVisited so that they find hidden items correctly. r=aswan,bsilverberg
https://hg.mozilla.org/mozilla-central/rev/f1146d5beca8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(VYV03354)
Flags: needinfo?(VYV03354) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.