Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846673006:
Omnibox: Make InMemoryURLIndex respect hidden URLRows (Closed)
DescriptionOmnibox: Make InMemoryURLIndex respect hidden URLRows
When HistoryBackend records a new page visit, it creates a new URLRow,
which is marked "hidden" if it has only been visited by subframes.
URLDatabase records these "hidden" URLRows, but explicitly excludes
them from the results of URLDatabase::AutocompleteForPrefix.
InMemoryURLIndex, however, ignores the "hidden" flag. This causes
HistoryQuickProvider to return autocompletions that have never been
visited in the main frame before.
This CL prevents "hidden" URLRows from entering the InMemoryURLIndex's
index. It prevents HistoryQuickProvider from returning completions
that have never been visited in the main frame.
This fix works for both on-the-fly visits, as well as restoring from
on-disk URLDatabase on startup.
One thing to note is that even "hidden" (i.e. subframe) visits are
counted as part of the visit count for URLRows. This seems wrong, since
those visits didn't affect the URL of the Omnibox, and seem more like
internal page state. However, changing that is beyond the scope of this
CL.
BUG=711772
Review-Url: https://codereview.chromium.org/2846673006
Cr-Commit-Position: refs/heads/master@{#470019}
Committed: https://chromium.googlesource.com/chromium/src/+/121980770b3b9c81971b1277180e303c64905c1c
Patch Set 1 #
Total comments: 2
Patch Set 2 : address nit #Patch Set 3 : change approach #Patch Set 4 : update comment #Patch Set 5 : fix #Patch Set 6 : change approach #
Total comments: 8
Patch Set 7 : address mpearson comments #
Total comments: 2
Messages
Total messages: 61 (31 generated)
tommycli@chromium.org changed reviewers: + pkasting@chromium.org
pkasting: PTAL This CL seems to fix it, but I'm not completely sure, since the 'after' screenshot still shows the HistoryURLProvider pushing a never-visited never-typed URL into inline-autocomplete. However, the new default autocomplete is at least to an origin that has been visited and typed before.
On 2017/04/28 00:05:02, tommycli wrote: > pkasting: PTAL > > This CL seems to fix it, but I'm not completely sure, since the 'after' > screenshot still shows the HistoryURLProvider pushing a never-visited > never-typed URL into inline-autocomplete. > > However, the new default autocomplete is at least to an origin that has been > visited and typed before. screenshots of before and after in the bug btw
On 2017/04/28 00:05:09, tommycli wrote: > On 2017/04/28 00:05:02, tommycli wrote: > > pkasting: PTAL > > > > This CL seems to fix it, but I'm not completely sure, since the 'after' > > screenshot still shows the HistoryURLProvider pushing a never-visited > > never-typed URL into inline-autocomplete. > > > > However, the new default autocomplete is at least to an origin that has been > > visited and typed before. > > screenshots of before and after in the bug btw The test btw, fails without patch, passes after.
LGTM. I expect Mark will want to weigh in on this, but it matches what the HUP does, and it matches the conceptual principles of the omnibox (particularly "past behavior predicts future behavior"). The inline autocompletion could be helpful, but it's most likely just surprising. I think it is still worth looking into deeper fixes here, like disambiguating a "manual subframe" navigation that's triggered by a user gesture from one that isn't, and not treating the latter as a "visit", in the same way we don't treat auto subframe navigations as visits. Doing this correctly might be complicated. https://codereview.chromium.org/2846673006/diff/1/components/omnibox/browser/... File components/omnibox/browser/history_quick_provider_unittest.cc (right): https://codereview.chromium.org/2846673006/diff/1/components/omnibox/browser/... components/omnibox/browser/history_quick_provider_unittest.cc:534: expected_urls.push_back("http://nevertyped.com/"); Nit: Just init at the definition with = {...} instead of define + push_back.
tommycli@chromium.org changed reviewers: + mpearson@chromium.org
mpearson: PTAL, as pkasting suggested you'd want to weigh in... thanks! https://codereview.chromium.org/2846673006/diff/1/components/omnibox/browser/... File components/omnibox/browser/history_quick_provider_unittest.cc (right): https://codereview.chromium.org/2846673006/diff/1/components/omnibox/browser/... components/omnibox/browser/history_quick_provider_unittest.cc:534: expected_urls.push_back("http://nevertyped.com/"); On 2017/04/28 06:02:13, Peter Kasting (OOO thru May 3) wrote: > Nit: Just init at the definition with = {...} instead of define + push_back. Done.
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
not lgtm This change is problematic as written. By marking this match as not allowed to be a default match here, it trump another provider saying that it should be a default match. See crbug.com/542780. For example, if I type "walmart.c", I get an inline autocompletion from the server for walmart.com even if I haven't visited walmart. With this change, if I'd visited walmart.com yet never typed it, and HistoryQuick provider happens to score it higher than the score the server provides that result, the result will not be allowed to be inline autocompleted. A comment about metrics will be sent via e-mail. --mark
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/28 17:03:15, Mark P wrote: > not lgtm > > This change is problematic as written. By marking this match as not allowed to > be a default match here, it trump another provider saying that it should be a > default match. See crbug.com/542780. For example, if I type "walmart.c", I get > an inline autocompletion from the server for http://walmart.com even if I haven't > visited walmart. With this change, if I'd visited http://walmart.com yet never typed > it, and HistoryQuick provider happens to score it higher than the score the > server provides that result, the result will not be allowed to be inline > autocompleted. > > A comment about metrics will be sent via e-mail. > > --mark Looping back to the bug -- two things we are considering: tommycli: Perhaps the right thing to do is to prevent subframe navigations without user gesture from entering the index in the first place? mpearson: Exclude any URLs that have never appeared in the Omnibox (so basically all subframe navs)
On 2017/04/28 18:42:26, tommycli wrote: > On 2017/04/28 17:03:15, Mark P wrote: > > not lgtm > > > > This change is problematic as written. By marking this match as not allowed > to > > be a default match here, it trump another provider saying that it should be a > > default match. See crbug.com/542780. For example, if I type "walmart.c", I > get > > an inline autocompletion from the server for http://walmart.com even if I > haven't > > visited walmart. With this change, if I'd visited http://walmart.com yet > never typed > > it, and HistoryQuick provider happens to score it higher than the score the > > server provides that result, the result will not be allowed to be inline > > autocompleted. > > > > A comment about metrics will be sent via e-mail. > > > > --mark > > Looping back to the bug -- two things we are considering: > > tommycli: Perhaps the right thing to do is to prevent subframe navigations > without user gesture from entering the index in the first place? > > mpearson: Exclude any URLs that have never appeared in the Omnibox (so basically > all subframe navs) I tend to agree... it's odd to me that subframe navigations appear as Omnibox suggestions at all. They don't appear in the explicit history list in chrome://history
On 2017/04/28 18:43:11, tommycli wrote: > > mpearson: Exclude any URLs that have never appeared in the Omnibox (so > basically > > all subframe navs) "All subframe navs" sounds like the simpler solution, earning it my vote, unless there's some specific argument for "some subframe navs".
Description was changed from ========== Omnibox: Stop HistoryQuickProvider inline-completing never-typed sites. Currently, there's no restriction on HistoryQuickProvider to prevent it from providing an inline-autocomplete that has never been typed by the user. An attack could abuse this, by making repeated calls to history.pushState to control what is inline autocompleted when the user starts typing a popular URL. This CL closes that above hole. BUG=711772 ========== to ========== Omnibox: Stop HistoryQuickProvider autocompleting subframe visits Currently, HistoryQuickProvider can return subframe URL visits and keyword-generated URL visits. Neither type of visit is visible in chrome://history, and can never be returned by HistoryURLProvider. This CL brings HistoryQuickProvider into line with HistoryURLProvider and prevents it from returning these non-visible visits. BUG=711772 ==========
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Omnibox: Stop HistoryQuickProvider autocompleting subframe visits Currently, HistoryQuickProvider can return subframe URL visits and keyword-generated URL visits. Neither type of visit is visible in chrome://history, and can never be returned by HistoryURLProvider. This CL brings HistoryQuickProvider into line with HistoryURLProvider and prevents it from returning these non-visible visits. BUG=711772 ========== to ========== Omnibox: Stop HistoryQuickProvider autocompleting subframe visits This CL makes HistoryQuickProvider ignore subframe and keyword-generated visits. These visits have URLs that have never appeared in the Omnibox, and so should not be suggested as an auto-completion. BUG=711772 ==========
mpearson: PTAL again. This one filters out subframe and keyword generated visits (which have never appeared in the Omnibox). The filter is inspired by the one in VisitDatabase: https://cs.chromium.org/chromium/src/components/history/core/browser/visit_da... I think we should probably also filter out redirect-visits like VisitDatabase does too, but let's save that for a separate CL...
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/28 22:59:13, tommycli wrote: > mpearson: PTAL again. This one filters out subframe and keyword generated visits > (which have never appeared in the Omnibox). > > The filter is inspired by the one in VisitDatabase: > https://cs.chromium.org/chromium/src/components/history/core/browser/visit_da... > > I think we should probably also filter out redirect-visits like VisitDatabase > does too, but let's save that for a separate CL... I would think code like this should go here. https://cs.chromium.org/chromium/src/components/history/core/browser/url_data... The place you've picked to put the code is inappropriate. It's only for adding a new visit. For example, when rebuilding the url index private data from history upon startup, we end up here: https://cs.chromium.org/chromium/src/components/omnibox/browser/url_index_pri... which won't have your filter at all. I'm inclined to agree that things not visible on the history page shouldn't be indexed. But do you have a sense of how these pages are visited, i.e., how much are we excluding? Can you find an UMA that records transition types? (Reply off this public bug thread.) I'm initially reluctant to immediately exclude ui::PAGE_TRANSITION_KEYWORD_GENERATED after reading about it. I think we might get some benefit from it by boosting the domain name of commonly-used keyword search engines. --mark
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Omnibox: Stop HistoryQuickProvider autocompleting subframe visits This CL makes HistoryQuickProvider ignore subframe and keyword-generated visits. These visits have URLs that have never appeared in the Omnibox, and so should not be suggested as an auto-completion. BUG=711772 ========== to ========== Omnibox: Make InMemoryURLIndex respect hidden URLRows When HistoryBackend records a new page visit, it creates a new URLRow, which is marked "hidden" if it has only been visited by subframes. URLDatabase records these "hidden" URLRows, but explicitly excludes them from the results of URLDatabase::AutocompleteForPrefix. InMemoryURLIndex, however, ignores the "hidden" flag. This causes HistoryQuickProvider to return autocompletions that have never been visited in the main frame before. This CL prevents "hidden" URLRows from entering the InMemoryURLIndex's index. It prevents HistoryQuickProvider from returning completions that have never been visited in the main frame. This fix works for both on-the-fly visits, as well as restoring from on-disk URLDatabase on startup. BUG=711772 ==========
On 2017/05/01 19:38:29, Mark P wrote: > On 2017/04/28 22:59:13, tommycli wrote: > > mpearson: PTAL again. This one filters out subframe and keyword generated > visits > > (which have never appeared in the Omnibox). > > > > The filter is inspired by the one in VisitDatabase: > > > https://cs.chromium.org/chromium/src/components/history/core/browser/visit_da... > > > > I think we should probably also filter out redirect-visits like VisitDatabase > > does too, but let's save that for a separate CL... > > I would think code like this should go here. > https://cs.chromium.org/chromium/src/components/history/core/browser/url_data... > > The place you've picked to put the code is inappropriate. It's only for adding > a new visit. For example, when rebuilding the url index private data from > history upon startup, we end up here: > https://cs.chromium.org/chromium/src/components/omnibox/browser/url_index_pri... > which won't have your filter at all. > > I'm inclined to agree that things not visible on the history page shouldn't be > indexed. But do you have a sense of how these pages are visited, i.e., how much > are we excluding? Can you find an UMA that records transition types? (Reply > off this public bug thread.) > > I'm initially reluctant to immediately exclude > ui::PAGE_TRANSITION_KEYWORD_GENERATED after reading about it. I think we might > get some benefit from it by boosting the domain name of commonly-used keyword > search engines. > > --mark mpearson: PTAL again. Hey Mark, Based on your feedback, I got a better understanding of what's going on, and have changed my approach. I'm pretty confident this new one is "the one". I explain my new approach in the updated CL description. Thanks, Tommy
side note: https://codereview.chromium.org/2846673006/diff/100001/components/history/cor... File components/history/core/browser/url_database.cc (right): https://codereview.chromium.org/2846673006/diff/100001/components/history/cor... components/history/core/browser/url_database.cc:643: return false; A side note, this is also used in HistoryURLProvider::DoAutocomplete. However, it's harmless, since hidden rows are already excluded in the SQL query there.
mpearson: ping, thanks!
https://codereview.chromium.org/2846673006/diff/100001/components/history/cor... File components/history/core/browser/url_database.cc (right): https://codereview.chromium.org/2846673006/diff/100001/components/history/cor... components/history/core/browser/url_database.cc:640: bool RowQualifiesAsSignificant(const URLRow& row, While you're here, can you revise the .h comment for this function by replacing |time_cache| with |threshold|? thanks! https://codereview.chromium.org/2846673006/diff/100001/components/history/cor... components/history/core/browser/url_database.cc:643: return false; On 2017/05/02 18:09:34, tommycli wrote: > A side note, this is also used in HistoryURLProvider::DoAutocomplete. However, > it's harmless, since hidden rows are already excluded in the SQL query there. Acknowledged. https://codereview.chromium.org/2846673006/diff/100001/components/omnibox/bro... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2846673006/diff/100001/components/omnibox/bro... components/omnibox/browser/in_memory_url_index_unittest.cc:485: TEST_F(InMemoryURLIndexTest, HiddenURLRowsAreIgnored) { Please confirm that this test fails before your fix. (It's not obvious to me that you're setting everything that needs to be set to get the URL back in the first place.) https://codereview.chromium.org/2846673006/diff/100001/components/omnibox/bro... components/omnibox/browser/in_memory_url_index_unittest.cc:488: history::URLRow(GURL("http://hidden.com"), new_row_id++); nit: add trailing slash to make proper URL :-)
I like this approach a lot better. I have two questions remaining: * Did you manage to find UMA data about the relative number of hidden versus non-hidden navigations, hidden versus non-hidden pages in the database, or hidden versus non-hidden omnibox uses? * For a page that was visited once in a non-hidden manner, it appears we count all times it was visited, and count all visits (even hidden onces) in UpdateRecentVisits. Does that sound right to you? Please think it through and argue one way or the other (and put the text on the changelist description for future reference). thanks, mark
Description was changed from ========== Omnibox: Make InMemoryURLIndex respect hidden URLRows When HistoryBackend records a new page visit, it creates a new URLRow, which is marked "hidden" if it has only been visited by subframes. URLDatabase records these "hidden" URLRows, but explicitly excludes them from the results of URLDatabase::AutocompleteForPrefix. InMemoryURLIndex, however, ignores the "hidden" flag. This causes HistoryQuickProvider to return autocompletions that have never been visited in the main frame before. This CL prevents "hidden" URLRows from entering the InMemoryURLIndex's index. It prevents HistoryQuickProvider from returning completions that have never been visited in the main frame. This fix works for both on-the-fly visits, as well as restoring from on-disk URLDatabase on startup. BUG=711772 ========== to ========== Omnibox: Make InMemoryURLIndex respect hidden URLRows When HistoryBackend records a new page visit, it creates a new URLRow, which is marked "hidden" if it has only been visited by subframes. URLDatabase records these "hidden" URLRows, but explicitly excludes them from the results of URLDatabase::AutocompleteForPrefix. InMemoryURLIndex, however, ignores the "hidden" flag. This causes HistoryQuickProvider to return autocompletions that have never been visited in the main frame before. This CL prevents "hidden" URLRows from entering the InMemoryURLIndex's index. It prevents HistoryQuickProvider from returning completions that have never been visited in the main frame. This fix works for both on-the-fly visits, as well as restoring from on-disk URLDatabase on startup. One thing to note is that even "hidden" (i.e. subframe) visits are counted as part of the visit count for URLRows. This seems wrong, since those visits didn't affect the URL of the Omnibox, and seem more like internal page state. However, changing that is beyond the scope of this CL. BUG=711772 ==========
Description was changed from ========== Omnibox: Make InMemoryURLIndex respect hidden URLRows When HistoryBackend records a new page visit, it creates a new URLRow, which is marked "hidden" if it has only been visited by subframes. URLDatabase records these "hidden" URLRows, but explicitly excludes them from the results of URLDatabase::AutocompleteForPrefix. InMemoryURLIndex, however, ignores the "hidden" flag. This causes HistoryQuickProvider to return autocompletions that have never been visited in the main frame before. This CL prevents "hidden" URLRows from entering the InMemoryURLIndex's index. It prevents HistoryQuickProvider from returning completions that have never been visited in the main frame. This fix works for both on-the-fly visits, as well as restoring from on-disk URLDatabase on startup. One thing to note is that even "hidden" (i.e. subframe) visits are counted as part of the visit count for URLRows. This seems wrong, since those visits didn't affect the URL of the Omnibox, and seem more like internal page state. However, changing that is beyond the scope of this CL. BUG=711772 ========== to ========== Omnibox: Make InMemoryURLIndex respect hidden URLRows When HistoryBackend records a new page visit, it creates a new URLRow, which is marked "hidden" if it has only been visited by subframes. URLDatabase records these "hidden" URLRows, but explicitly excludes them from the results of URLDatabase::AutocompleteForPrefix. InMemoryURLIndex, however, ignores the "hidden" flag. This causes HistoryQuickProvider to return autocompletions that have never been visited in the main frame before. This CL prevents "hidden" URLRows from entering the InMemoryURLIndex's index. It prevents HistoryQuickProvider from returning completions that have never been visited in the main frame. This fix works for both on-the-fly visits, as well as restoring from on-disk URLDatabase on startup. One thing to note is that even "hidden" (i.e. subframe) visits are counted as part of the visit count for URLRows. This seems wrong, since those visits didn't affect the URL of the Omnibox, and seem more like internal page state. However, changing that is beyond the scope of this CL. BUG=711772 ==========
Patchset #8 (id:140001) has been deleted
Thanks! I'll send another email with replies to your general comments. https://codereview.chromium.org/2846673006/diff/100001/components/history/cor... File components/history/core/browser/url_database.cc (right): https://codereview.chromium.org/2846673006/diff/100001/components/history/cor... components/history/core/browser/url_database.cc:640: bool RowQualifiesAsSignificant(const URLRow& row, On 2017/05/03 19:04:49, Mark P wrote: > While you're here, can you revise the .h comment for this function by replacing > |time_cache| with |threshold|? > thanks! Done. https://codereview.chromium.org/2846673006/diff/100001/components/omnibox/bro... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2846673006/diff/100001/components/omnibox/bro... components/omnibox/browser/in_memory_url_index_unittest.cc:485: TEST_F(InMemoryURLIndexTest, HiddenURLRowsAreIgnored) { On 2017/05/03 19:04:49, Mark P wrote: > Please confirm that this test fails before your fix. (It's not obvious to me > that you're setting everything that needs to be set to get the URL back in the > first place.) Done. I can confirm this test fails before the fix, and succeeds after. It sets the same fields as the HugeResultSet test, so if that test works, so should this one. :) https://codereview.chromium.org/2846673006/diff/100001/components/omnibox/bro... components/omnibox/browser/in_memory_url_index_unittest.cc:488: history::URLRow(GURL("http://hidden.com"), new_row_id++); On 2017/05/03 19:04:49, Mark P wrote: > nit: add trailing slash to make proper URL :-) Done.
On 2017/05/03 19:04:56, Mark P wrote: > I like this approach a lot better. > > I have two questions remaining: > > * Did you manage to find UMA data about the relative number of hidden versus > non-hidden navigations, hidden versus non-hidden pages in the database, or > hidden versus non-hidden omnibox uses? > > * For a page that was visited once in a non-hidden manner, it appears we count > all times it was visited, and count all visits (even hidden onces) in > UpdateRecentVisits. Does that sound right to you? Please think it through and > argue one way or the other (and put the text on the changelist description for > future reference). > > thanks, > mark Regarding the UMAs. I didn't find any good UMA metrics for that. Closest one would be DataUse.PageTransition.UserTraffic, but it doesn't seem to show any hits for SUBFRAME data, so it's probably not useful. Regarding subframe visits incrementing the visit count - it does seem wrong, and I added a paragraph at the end of the CL description arguing as such. However, I also think it's beyond the scope of this CL to update, and we can change that in a followup (if you also think it's wrong).
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm I agree with you on the visit_count issue. Let's leave it to Peter, when he returns, to see if he thinks it's worth worrying about (i.e., filing a bug) or whether he thinks not many pages have both hidden and not-hidden visits and thus isn't worth the bother. --mark
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/03 19:49:20, Mark P wrote: > lgtm > > I agree with you on the visit_count issue. Let's leave it to Peter, when he > returns, to see if he thinks it's worth worrying about (i.e., filing a bug) or > whether he thinks not many pages have both hidden and not-hidden visits and thus > isn't worth the bother. > > --mark thank you sir, sending it in!
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2846673006/#ps120001 (title: "address mpearson comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tommycli@chromium.org changed reviewers: + sdefresne@chromium.org
sdefresne: PTAL OWNERS stamp needed, thanks!
lgtm
LGTM Considering whether "hidden visits" should not be counted anywhere for omnibox scoring: the main thing I'd think this might affect is stuff like youtube, maps, etc. that gets embedded into other sites a lot. It might be nice to (not inline) autocomplete an embedded YT video in the same way we can autocomplete a link-clicked site (but, again, we won't inline-autocomplete it). So my ideal solution would probably be one that does not exclude these sorts of visits, but rather counts them differently, in the same way we already distinguish between typed and untyped visits. I also think it's important that we follow up on Mark's comment that there's significant HQP position 0 use for untyped URLs, especially since our instinct was that this wasn't (and shouldn't be) happening. I am worried that even if inline AC for untyped URLs is good on net in this case, we may still want to remove it because the downside of surprising, annoying behavior is psychologically greater than the upside of helpful bootstrapping of completions. But in any case, I would be sad if that got dropped on the floor without further unvestigation. https://codereview.chromium.org/2846673006/diff/120001/components/omnibox/bro... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2846673006/diff/120001/components/omnibox/bro... components/omnibox/browser/in_memory_url_index_unittest.cc:493: EXPECT_EQ(0U, url_index_ Nit: EXPECT_TRUE(...empty())?
thank you all! https://codereview.chromium.org/2846673006/diff/120001/components/omnibox/bro... File components/omnibox/browser/in_memory_url_index_unittest.cc (right): https://codereview.chromium.org/2846673006/diff/120001/components/omnibox/bro... components/omnibox/browser/in_memory_url_index_unittest.cc:493: EXPECT_EQ(0U, url_index_ On 2017/05/04 19:17:36, Peter Kasting wrote: > Nit: EXPECT_TRUE(...empty())? Well... I didn't do that because everywhere else in the file uses EXPECT_EQ(0u. I think it's because we if the result is not zero, it's nice to be able to see the actual number. (If it's 1, 2, 3, etc. matches). We can fix in followup if you feel strongly about this point. Sending it in.
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494258290831720,
"parent_rev": "ec927595eabb77e499744d00f3b4114182a20546", "commit_rev":
"121980770b3b9c81971b1277180e303c64905c1c"}
Message was sent while issue was closed.
Description was changed from ========== Omnibox: Make InMemoryURLIndex respect hidden URLRows When HistoryBackend records a new page visit, it creates a new URLRow, which is marked "hidden" if it has only been visited by subframes. URLDatabase records these "hidden" URLRows, but explicitly excludes them from the results of URLDatabase::AutocompleteForPrefix. InMemoryURLIndex, however, ignores the "hidden" flag. This causes HistoryQuickProvider to return autocompletions that have never been visited in the main frame before. This CL prevents "hidden" URLRows from entering the InMemoryURLIndex's index. It prevents HistoryQuickProvider from returning completions that have never been visited in the main frame. This fix works for both on-the-fly visits, as well as restoring from on-disk URLDatabase on startup. One thing to note is that even "hidden" (i.e. subframe) visits are counted as part of the visit count for URLRows. This seems wrong, since those visits didn't affect the URL of the Omnibox, and seem more like internal page state. However, changing that is beyond the scope of this CL. BUG=711772 ========== to ========== Omnibox: Make InMemoryURLIndex respect hidden URLRows When HistoryBackend records a new page visit, it creates a new URLRow, which is marked "hidden" if it has only been visited by subframes. URLDatabase records these "hidden" URLRows, but explicitly excludes them from the results of URLDatabase::AutocompleteForPrefix. InMemoryURLIndex, however, ignores the "hidden" flag. This causes HistoryQuickProvider to return autocompletions that have never been visited in the main frame before. This CL prevents "hidden" URLRows from entering the InMemoryURLIndex's index. It prevents HistoryQuickProvider from returning completions that have never been visited in the main frame. This fix works for both on-the-fly visits, as well as restoring from on-disk URLDatabase on startup. One thing to note is that even "hidden" (i.e. subframe) visits are counted as part of the visit count for URLRows. This seems wrong, since those visits didn't affect the URL of the Omnibox, and seem more like internal page state. However, changing that is beyond the scope of this CL. BUG=711772 Review-Url: https://codereview.chromium.org/2846673006 Cr-Commit-Position: refs/heads/master@{#470019} Committed: https://chromium.googlesource.com/chromium/src/+/121980770b3b9c81971b1277180e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/121980770b3b9c81971b1277180e...
Message was sent while issue was closed.
On 2017/05/04 19:17:36, Peter Kasting wrote: > LGTM > > Considering whether "hidden visits" should not be counted anywhere for omnibox > scoring: the main thing I'd think this might affect is stuff like youtube, maps, > etc. that gets embedded into other sites a lot. It might be nice to (not > inline) autocomplete an embedded YT video in the same way we can autocomplete a > link-clicked site (but, again, we won't inline-autocomplete it). So my ideal > solution would probably be one that does not exclude these sorts of visits, but > rather counts them differently, in the same way we already distinguish between > typed and untyped visits. This doesn't answer the question I tried to ask. I guess I wasn't clear. (For the record, I disagree with your statement here. I think it's appropriate to never suggest a URL and title the user will not recognize. If the URL never appeared in the omnibox and the title never appeared as a tab strip title, displaying one of these anywhere in the dropdown is likely to be surprising.) My question was about pages that have both hidden and not-hidden visits. For example, if a user sees an embedded youtube video, then clicks on a link to the full youtube.com page for that video. For these pages, should we be scoring them counting all their visits or only all their non-hidden visits? I'm fine with the current state that results from this change (all visits are counted), but if you feel strongly please file a bug so we can remember to look into it at some point. > I also think it's important that we follow up on Mark's comment that there's > significant HQP position 0 use for untyped URLs, especially since our instinct > was that this wasn't (and shouldn't be) happening. I am worried that even if > inline AC for untyped URLs is good on net in this case, we may still want to > remove it because the downside of surprising, annoying behavior is > psychologically greater than the upside of helpful bootstrapping of completions. > But in any case, I would be sad if that got dropped on the floor without > further investigation. I think this behavior (inlining things with typed_count = 0) is likely reasonable, and probably not surprising else we would've heard about it sometime. (Recall that we still only inline-autocomplete prefix matches, even if they come from the HQP.) If you feel strongly, you can file a bug to investigate it. I think I know what metrics to use, though I don't want to share them on a public bug. That said, I would not prioritize this at this point. I think we probably would've heard at least one anecdotal complaint if inline autocompletion for URLs was too aggressive. I'd also ignore it for a while since HQP scoring is so much in flux right now anyway. --mark
Message was sent while issue was closed.
On 2017/05/08 18:10:36, Mark P wrote: > On 2017/05/04 19:17:36, Peter Kasting wrote: > > Considering whether "hidden visits" should not be counted anywhere for omnibox > > scoring: the main thing I'd think this might affect is stuff like youtube, > maps, > > etc. that gets embedded into other sites a lot. It might be nice to (not > > inline) autocomplete an embedded YT video in the same way we can autocomplete > a > > link-clicked site (but, again, we won't inline-autocomplete it). So my ideal > > solution would probably be one that does not exclude these sorts of visits, > but > > rather counts them differently, in the same way we already distinguish between > > typed and untyped visits. > > (For the record, I disagree with your statement here. I think it's appropriate > to never suggest a URL and title the user will not recognize. If the URL never > appeared in the omnibox and the title never appeared as a tab strip title, > displaying one of these anywhere in the dropdown is likely to be surprising.) My assumption was that the URL and title might be familiar despite not appearing in those places. For example, if I watch an embedded video about XYZ on site ABC, then "XYZ" could be familiar despite me having seen only "ABC" as a tab title. > My question was about pages that have both hidden and not-hidden visits. > For example, if a user sees an embedded youtube video, then clicks on a link > to the full http://youtube.com page for that video. For these pages, should we be > scoring them counting all their visits or only all their non-hidden visits? Right, I was trying to answer that question. I'm sorry my answer was not clearly an answer to that question :). I think they should be counted, but not counted the same way as a normal visit. So basically, neither of your answers. I think if they counted separately and could be weighted separately, we could eliminate the need for "filter sites that only have hidden visits" because such sites would be more low-ranked (and not inline-autocompleting) by default. Even a naive method of e.g. treating each "hidden visit" as, say, 1/3rd of a normal visit could help. But I don't feel confident enough about this to file a bug or care how we pursue it. > > I also think it's important that we follow up on Mark's comment that there's > > significant HQP position 0 use for untyped URLs, especially since our instinct > > was that this wasn't (and shouldn't be) happening. I am worried that even if > > inline AC for untyped URLs is good on net in this case, we may still want to > > remove it because the downside of surprising, annoying behavior is > > psychologically greater than the upside of helpful bootstrapping of > completions. > > But in any case, I would be sad if that got dropped on the floor without > > further investigation. > > I think this behavior (inlining things with typed_count = 0) is likely > reasonable, > and probably not surprising else we would've heard about it sometime. > (Recall that we still only inline-autocomplete prefix matches, even if they > come from the HQP.) > > If you feel strongly, you can file a bug to investigate it. This one I do feel strongly about. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=719657 . > I think we probably would've heard at least > one > anecdotal complaint if inline autocompletion for URLs was too aggressive. I am suspicious of this reasoning. We hear lots of anecdotal complaints that people want to turn off inline autocompletion in general; I got one just this morning, for example. I don't think users' mental granularity is sufficient to complain more precisely than that. > I'd also ignore it for a while since HQP scoring is so much in flux right now > anyway. That is fair. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
