1260743 - Searching by the url doesn’t work

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Vasilica Tamas, QA [:vtamas]]

[Note]
This is a follow-up bug for Bug 1225743

[Affected versions]:
Firefox 48.0a1 (2016-03-29)
Firefox 47.0a2 (2016-03-29)

[Affected platforms]:
Windows 10 64-bit
Ubuntu 14.04 32-bit
Mac OS X 10.11.2

[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Create the xpinstall.signatures.dev-root pref in about:config and set it to true.
3.Install the following webextension:  https://addons.allizom.org/en-US/firefox/addon/testforica07/
4.Bookmark a webpage and copy the url.
5.Click on the webextension icon from toolbar.
6.Paste the url in the text field and click on “Search” button.

[Expected Results]:
The bookmarked page successfully appears as a result while searching by the entire url. 

[Actual Results]:
There is no result displayed while searching by the entire url.
See screenshot: http://i.imgur.com/tPNASxZ.jpg

[Additional notes]:
- The expected result is successfully displayed when the protocol (http://) and the sub-domaine (www.) are eliminated from the search keyword.
- See screenshot: http://i.imgur.com/YwtTczV.jpg
- I am marking Firefox 46 and Firefox 45 as unaffected because Bug 1225743 was implemented starting with Firefox 37.

Comment 1

[Bob Silverberg [:bsilverberg]]

This is being caused by the implementation of `queryBookmarks` in Bookmarks.jsm [1]. When it constructs a query using a string search term, it uses `MATCH_BOUNDARY` [2] for the `matchBehavior` which results in a non-match when passing in a full url. I found through experimentation that if we use `MATCH_ANYWHERE_UNMODIFIED` [3] instead of `MATCH_BOUNDARY` that it is able to find the bookmark when using the full url as the query parameter, but I assume that simply changing that in the Bookmarks.jsm code would break other things.

I'm not sure why the code needs to use `MATCH_BOUNDARY`, but I'm guessing there are valid reasons.

We can ensure that results are returned when searching using a full url if we specify that it is a url we are searching for, i.e., the query is not a string containing a url but rather an object with a `url` property, and our API supports that. But I can see how that would be inconvenient to a developer who doesn't know what a user might type into a search field. Ideally we should be able to return results for a bookmark via a string search even if the user types in the full url.

Marco, what is your take on this? Why does the current code use `MATCH_BOUNDARY`? Could we change that in `queryBookmarks` in Bookmarks.jsm? If not, what would you recommend?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#913
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/mozIPlacesAutoComplete.idl#36
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/mozIPlacesAutoComplete.idl#47

Comment 2

[Johann Hofmann [:johannh]]

(In reply to Bob Silverberg [:bsilverberg] from comment #1)
> This is being caused by the implementation of `queryBookmarks` in
> Bookmarks.jsm [1]. When it constructs a query using a string search term, it
> uses `MATCH_BOUNDARY` [2] for the `matchBehavior` which results in a
> non-match when passing in a full url. I found through experimentation that
> if we use `MATCH_ANYWHERE_UNMODIFIED` [3] instead of `MATCH_BOUNDARY` that
> it is able to find the bookmark when using the full url as the query
> parameter, but I assume that simply changing that in the Bookmarks.jsm code
> would break other things.
> 
> I'm not sure why the code needs to use `MATCH_BOUNDARY`, but I'm guessing
> there are valid reasons.
> 
> We can ensure that results are returned when searching using a full url if
> we specify that it is a url we are searching for, i.e., the query is not a
> string containing a url but rather an object with a `url` property, and our
> API supports that. But I can see how that would be inconvenient to a
> developer who doesn't know what a user might type into a search field.
> Ideally we should be able to return results for a bookmark via a string
> search even if the user types in the full url.
> 
> Marco, what is your take on this? Why does the current code use
> `MATCH_BOUNDARY`? Could we change that in `queryBookmarks` in Bookmarks.jsm?
> If not, what would you recommend?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> Bookmarks.jsm#913
> [2]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> mozIPlacesAutoComplete.idl#36
> [3]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> mozIPlacesAutoComplete.idl#47

Mmh, there was something wrong with MATCH_ANYWHERE but I don't remember what it was unfortunately. Anyway, I would claim that this function and the WebExtension side is actually pretty well covered with tests, so if all other tests and the one you're adding for this bug pass, you're good to go. This function was written only for WebExtensions, so you can safely assume that you're not breaking other consumers if the tests pass.

Comment 3

[Bob Silverberg [:bsilverberg]]

Oh, nice, that's good to know, Johann. Thanks for the info!  I'll try making the change and running *all* of the tests and see what happens.

Comment 4

[Bob Silverberg [:bsilverberg]]

I did some testing and all of our tests for the bookmarks API are fine with the change from MATCH_BOUNDARY to MATCH_ANYWHERE_UNMODIFIED, but one of the tests for Bookmarks.jsm fails. This test [1] does a string search for the string "a menu bookmark" and instead of returning one result (which is a bookmark that was created with the title "a menu bookmark"), which is currently expected, it returns two. It also returns a bookmark with the title "Bookmarks Menu". I think one could argue either case - either the single result or the two results could be considered to be accurate depending on how strict we expect the search to be.

Personally, I'm fine with making this change and allowing the search to be a bit more liberal. In that case I would simply update the test to expect the two bookmarks instead of just the one.

What do you think, Johann?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/bookmarks/test_bookmarks_search.js#49

Comment 5

[Marco Bonardo [:mak]]

The reason this happens is that the search function you are using was meant for the awesomebar, that ignores scheme and www to try to provide more results for the user, for example if the user typed http://www.test we could also return an https://test.

MATCH_ANYWHERE_UNMODIFIED makes so that we don't fixup the uri http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/SQLFunctions.cpp#227 and it's the bahavior used when searching in the Library or the history sidebar, for example.

I think it's fine to use match_anywhere_unmodified, but if you prefer the boundary behavior, it should be trivial to add a MATCH_BOUNDARY_UNMODIFIED

Comment 6

[Johann Hofmann [:johannh]]

Huh, checking that test against Chrome the MATCH_ANYWHERE_UNMODIFIED behavior actually seems more correct... If you create a bookmark and a folder with those titles it matches both.

So I think you can go ahead here.

Comment 7

[Bob Silverberg [:bsilverberg]]

Thanks for the feedback Johann and Marco. It sounds like we can just switch to MATCH_ANYWHERE_UNMODIFIED for now, so that's what I'm going to do.

Comment 8

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1260743 - Searching by the url doesn’t work, r?aswan r?mak

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

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8751417 [details]
MozReview Request: Bug 1260743 - Searching by the url doesn’t work, r?aswan r?mak

https://reviewboard.mozilla.org/r/52013/#review48945

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8751417 [details]
MozReview Request: Bug 1260743 - Searching by the url doesn’t work, r?aswan r?mak

https://reviewboard.mozilla.org/r/52013/#review49075

Comment 11

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=947672a3d5a1

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/133ef863766e

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/133ef863766e

Comment 14

[Vasilica Tamas, QA [:vtamas]]

Manually verified as fixed on Firefox 49.0a1 (2016-05-19) under Windows 10 64-bit, Ubuntu 13.10 64-bit and Mac OS X 10.11. The bookmarked page is successfully displayed while searching by the URL as a string.