New issue
Advanced search Search tips

Issue 846459 link

Starred by 4 users

Issue metadata

Status: WontFix
Owner:
Closed: Nov 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

History.TopHostsVisitsByRank no longer recorded

Project Member Reported by dewittj@chromium.org, May 24 2018

Issue description

HistoryService::TopHosts is no longer called anywhere, because the last usage was in ResourcePrefetchPredictor and removed in revision b5ea1fe654f7e0e71704dc278ccd0b5c5cab7a8a

Is this UMA considered obsolete? Can it be removed? Or does there need to be another home for the TopHosts computation?

 

Comment 2 by bengr@chromium.org, Jun 1 2018

Cc: sclit...@chromium.org
Owner: dougarnett@chromium.org
Status: Assigned (was: Untriaged)
 I think we can remove this UMA, unless this is relevant to your work, Doug.
Cc: sophiechang@chromium.org dougarnett@chromium.org
Labels: -Pri-3 M-71 Pri-2
Owner: jegray@chromium.org
This is useful again with evaluating the effectiveness of requesting hints for top hosts. I'm going to look into re-enabling this for M-71.
Issue 684606 has been merged into this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7fbc699c08b42e562ecbdab2a4f632be7fffc685

commit 7fbc699c08b42e562ecbdab2a4f632be7fffc685
Author: Jered Gray <jegray@google.com>
Date: Mon Oct 08 23:36:43 2018

Fix counting bug in HistoryDatabase::TopHosts

Previously, HistoryDatabase::TopHosts was retrieving all visits within the past
month but adding total visit count for the URL to the host's visit count for
each visit.

What this means is that if a host had 10 visits, then the visit count would come
back as 100 (10 rows returned. all with a visit count of 10). If a host had 20
visits, then the visit count would come back as 400 (20 rows returned, all with
a visit count of 20).

This has been modified so that each visit simply increments the visit count of
the host. Furthermore, reload visits are now culled from the visits retrieved.
Previously, they were being retrieved HistoryDatabase::TopHosts() but were not
incrementing the visit count in HistoryBackend::AddPageVisit() when they were
added, so were not intended to impact the total visit count.

Additionally, the results are now sorted so that most recent visits are returned
first. This guarantees that the most recently visited hosts will be included in
the results in the case where |kMaxHostsInMemory| is reached.

Lastly, the logic for |kMaxHostsInMemory| has been modified so that it
blocks new hosts from being added to the hash map, but does not prevent the
visit counts of already existing hosts from being incremented.

A unittest has been added verifying that TopHosts works as expected and some
deprecated function cleanup has been done in HistoryServiceTest.

Bug:  846459 
Change-Id: Icf02305f672bc434da6a5b8dae0d3b2690ad2f3e
Reviewed-on: https://chromium-review.googlesource.com/c/1263571
Commit-Queue: Jered Gray <jegray@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597729}
[modify] https://crrev.com/7fbc699c08b42e562ecbdab2a4f632be7fffc685/components/history/core/browser/history_database.cc
[modify] https://crrev.com/7fbc699c08b42e562ecbdab2a4f632be7fffc685/components/history/core/browser/history_service_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/60dd4494fc65dba35bb5a6269735d3dab52c1ae6

commit 60dd4494fc65dba35bb5a6269735d3dab52c1ae6
Author: Jered Gray <jegray@google.com>
Date: Fri Oct 19 02:36:48 2018

Use same filters as TopHosts when calling RecordTopHostsMetrics

Previously, page visits were recorded in RecordTopHostMetrics() for
several transition types that were filtered out when retrieving
TopHosts, such as reloads. This has been changed so that the same
filters are used for both, which should make the UMA histogram more
accurate.

HistoryServiceTests have been updated to test the top hosts histogram
and an additional test has been added to verify that the page
transitions recorded as visits by RecordTopHostMetrics() and those
retrieved from the db when TopHosts() is called remain in sync.

Additionally, PAGE_TRANSITION_KEYWORD_GENERATED is no longer culled
from those incrementing the visit count.

Lastly, histograms.xml has been updated to reflect the current state
of History.TopHostVisitsByRank. This histogram hasn't been in active
use since September of 2017, although it'll soon become active again.

Bug:  846459 
Change-Id: I3fb153fab5c4e8db06a980d60ba869a2b25af903
Reviewed-on: https://chromium-review.googlesource.com/c/1270183
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Jered Gray <jegray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601032}
[modify] https://crrev.com/60dd4494fc65dba35bb5a6269735d3dab52c1ae6/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/60dd4494fc65dba35bb5a6269735d3dab52c1ae6/components/history/core/browser/history_database.cc
[modify] https://crrev.com/60dd4494fc65dba35bb5a6269735d3dab52c1ae6/components/history/core/browser/history_service_unittest.cc
[modify] https://crrev.com/60dd4494fc65dba35bb5a6269735d3dab52c1ae6/tools/metrics/histograms/histograms.xml
[modify] https://crrev.com/60dd4494fc65dba35bb5a6269735d3dab52c1ae6/ui/base/page_transition_types.h

Status: WontFix (was: Assigned)
History.TopHostsVisitsByRank has been removed from the codebase.
https://chromium-review.googlesource.com/c/1313050

Sign in to add a comment