Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Bloodhound for URL bar suggestions & move suggestions to browser process #8824

Merged
merged 22 commits into from
May 18, 2017

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented May 10, 2017

This PR does several things:

  • Makes suggestions code independent of window renderer
  • Moves URL bar suggestion creating into the browser process
  • Creates a wrapper lib for bloodhound autocomplete results for bookmarks and history
  • Uses that wrapper lib instead of using indexOf for faster autocomplete results.
  • Moves several windowActions around url bar suggestions into app actions and stops naming them set*
  • Dispatcher with queryInfo of windowId specified will now always route to the browser process, before it would skip that if the window renderer was routing an app action to itself. The browser process in that case will not re-dispatch to the window process.
  • Various other perf and refactoring improvements.

Fix #7453
Fix #5878
Fix #8919

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Better to not rebase in this case, changes are very cleanly defined in various small refactors leading up to using bloodhound for URL suggestions in the browser process.

Test Plan:

  • Test around search options in preferences to make sure they work like they used to
  • Import lots of history and bookmarks and make sure it's still snappy to get suggestions
  • Make sure google.com shows up first when typing google.com
  • Make sure things work at least as good as before in terms of getting results you wanted when typing.
  • QA team: please do a full pass around URL bar suggestions, try to find things that don't work as expected but used to work before. Please post follow ups for found things within the context of the same .300 milestone. Please avoid posting things that have always worked that way in previous versions though.

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run indivually or as a suite ref

@bbondy bbondy force-pushed the bloodhound branch 7 times, most recently from 243f473 to e10c2ab Compare May 14, 2017 04:35
@bbondy bbondy force-pushed the bloodhound branch 8 times, most recently from 5a256f2 to 6a4d3f3 Compare May 16, 2017 20:59
@bbondy bbondy changed the title WIP: Bloodhound for URL bar suggestions Bloodhound for URL bar suggestions & move suggestions to browser process May 16, 2017
@bbondy bbondy force-pushed the bloodhound branch 5 times, most recently from 2010302 to abddb25 Compare May 17, 2017 20:08
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. When you arrow over suggestions which don't match the current URL format, their URL isn't put into the URL bar. After talking to you, this is expected... but it feels a little awkward. If you hit enter, it does work as expected though
    urlbar1
    edit: this was already captured with URL is not changing in Address bar #8306

  2. I think results should prefer the URL over the title / other criteria. Here's an example:

google com

None of the results are for www.google.com. However, if I type the protocol in:
sgoogle com
this looks as expected

/**
* Indicates that the urlbar text has changed, usually from user input
*
* @param {string} location - The text to set as the new navbar URL input
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs need updating to include the windowId / input fields

Copy link
Member

@bsclifton bsclifton May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derp- this is an individual commit, will go back to looking at the all files changed view 😛 Disregard (you had already fixed it!)

@bsclifton
Copy link
Member

bsclifton commented May 17, 2017

I think this PR fixes this issue: #5878

@bsclifton
Copy link
Member

bsclifton commented May 17, 2017

The weird behavior I noticed with TAB has been logged as a separate issue: #8919 (happens in master- not caused by this PR)

Auditors: @bsclifton

Fix #5878

Test comming in a follow up commit now, just pushing without so you can get a build going
Matches things better if the user puts a / in the input
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are some great improvements! Great job 😄

Definitely faster- the results are more relevant / matching, and you fixed some related bugs too

++++!

@bsclifton bsclifton added this to the 0.15.300 milestone May 18, 2017
@bsclifton bsclifton merged commit 940b064 into master May 18, 2017
@bsclifton bsclifton deleted the bloodhound branch May 18, 2017 17:29
bsclifton added a commit that referenced this pull request May 18, 2017
Bloodhound for URL bar suggestions & move suggestions to browser process
bsclifton added a commit that referenced this pull request May 18, 2017
Bloodhound for URL bar suggestions & move suggestions to browser process
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants