Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124903003:
Record impressions/navigations only once per tile. (Closed)
|
Created:
4 years, 5 months ago by sfiera Modified:
4 years, 5 months ago Reviewers:
Marc Treib CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, pedrosimonetti+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord impressions/navigations only once per tile.
Track when we have logged an impression or navigation for a tile, then
ignore it if we are asked to log an additional one for the same tile.
If we detect that the user has returned to the NTP after navigating
away, reset the tracker (and has_emitted_) so that we can collect
stats again.
BUG=625163, 626681
Committed: https://crrev.com/61b6eba8e654d433215a148f03f7eeac708c73ab
Cr-Commit-Position: refs/heads/master@{#405751}
Patch Set 1 #Patch Set 2 : #
Total comments: 24
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 5
Patch Set 7 : #
Total comments: 9
Patch Set 8 : #
Total comments: 1
Patch Set 9 : #
Messages
Total messages: 49 (25 generated)
See the long comment in the header and decide if it matches what you think "expected" behavior is for these metrics.
sfiera@chromium.org changed reviewers: + treib@chromium.org
See the long comment in the header and decide if it matches what you think "expected" behavior is for these metrics.
On 2016/07/06 17:52:15, sfiera wrote: > See the long comment in the header and decide if it matches what you think > "expected" behavior is for these metrics. For impressions, this makes sense. (One small wrinkle: If the user first gets, say, 3 TopSites tiles, then logs in and gets 8 MostLikely tiles, then we'd log only the 5 additional tiles - would be nicer if we could log either all or nothing. But that should be rare, and I don't see what we could do about it with the current architecture.) For navigations, I'm not convinced: If a user opens an NTP, clicks a tile, navigates back, and clicks a tile again, then we'd only record a navigation if that was a different tile than the first time. That seems inconsistent - any reason we need the once-per-NTP behavior for navigations at all? But it seems to me that we should either have a one-navigation-per-NTP constraint, independent of tile, or not have a constraint at all. WDYT?
How exactly are navigations used? I thought the idea was to compare them against impressions to get a clickthrough rate. Suppose a user always navigates to tile #1, goes back, then navigates to tile #2, then back, then tile #1 again. We could… 1. Record 100% clickthrough for tile 1, and nothing for tile 2 (recording one navigation per NTP) 2. Record 200% clickthrough for tile 1, and 100% for tile 1 (recording one navigation per click) 3. Record 100% clickthrough for both tiles. (as here) 4. Record 66% clickthrough for tile 1, and 33% for tile 2 (which requires that the navigations behavior remain unchanged, but that the impressions reset on navigating back to the NTP) I can investigate #4 further, but I think #3 is better than #1 and #2.
On 2016/07/07 10:54:42, sfiera wrote: > How exactly are navigations used? I thought the idea was to compare them against > impressions to get a clickthrough rate. Suppose a user always navigates to tile > #1, goes back, then navigates to tile #2, then back, then tile #1 again. We > could… > > 1. Record 100% clickthrough for tile 1, and nothing for tile 2 (recording one > navigation per NTP) > 2. Record 200% clickthrough for tile 1, and 100% for tile 1 (recording one > navigation per click) > 3. Record 100% clickthrough for both tiles. (as here) > 4. Record 66% clickthrough for tile 1, and 33% for tile 2 (which requires that > the navigations behavior remain unchanged, but that the impressions reset on > navigating back to the NTP) > > I can investigate #4 further, but I think #3 is better than #1 and #2. We're mostly interested in the total per-NTP CTR, not so much in the per-tile one. In both #2 and #3, that would be > 100%. IMO the "correct" behavior would be to reset everything when navigating back to the NTP, i.e. just count it as a whole new NTP. Failing that, I'd kinda prefer either #1 or #2 over #3: #1 because it's at least consistent (everything is only ever recorded on the first impression), or #2 because it doesn't throw away navigations. #3 just seems kinda arbitrary to me - we're ignoring some navigations, but not others. The tiles might even have changed in the meantime, so tile index isn't actually all that meaningful. Anyway, the whole thing probably isn't frequent enough to fight over :) Should we maybe add some "NavigatedBackToNTP" metric to track how often that kind of thing happens? Then again, if we can detect that case easily, then we might as well just re-count the metrics...
On 2016/07/07 11:39:01, Marc Treib wrote: > On 2016/07/07 10:54:42, sfiera wrote: > > How exactly are navigations used? I thought the idea was to compare them > against > > impressions to get a clickthrough rate. Suppose a user always navigates to > tile > > #1, goes back, then navigates to tile #2, then back, then tile #1 again. We > > could… > > > > 1. Record 100% clickthrough for tile 1, and nothing for tile 2 (recording one > > navigation per NTP) > > 2. Record 200% clickthrough for tile 1, and 100% for tile 1 (recording one > > navigation per click) > > 3. Record 100% clickthrough for both tiles. (as here) > > 4. Record 66% clickthrough for tile 1, and 33% for tile 2 (which requires that > > the navigations behavior remain unchanged, but that the impressions reset on > > navigating back to the NTP) > > > > I can investigate #4 further, but I think #3 is better than #1 and #2. > > We're mostly interested in the total per-NTP CTR, not so much in the per-tile > one. In both #2 and #3, that would be > 100%. > IMO the "correct" behavior would be to reset everything when navigating back to > the NTP, i.e. just count it as a whole new NTP. Failing that, I'd kinda prefer > either #1 or #2 over #3: #1 because it's at least consistent (everything is only > ever recorded on the first impression), or #2 because it doesn't throw away > navigations. #3 just seems kinda arbitrary to me - we're ignoring some > navigations, but not others. The tiles might even have changed in the meantime, > so tile index isn't actually all that meaningful. > > Anyway, the whole thing probably isn't frequent enough to fight over :) Should > we maybe add some "NavigatedBackToNTP" metric to track how often that kind of > thing happens? Then again, if we can detect that case easily, then we might as > well just re-count the metrics... OK, then I'll switch to #1 for now and figure out if I can do #4 instead. Those options are best for per-NTP CTR, instead of per-tile. I'm not sure I'll be able to make any progress today, as my home setup is lacking :/
Description was changed from ========== Record impressions/navigations only once per tile. Track when we have logged an impression or navigation for a tile, then ignore it if we are asked to log an additional one for the same tile. BUG=625163 ========== to ========== Record impressions/navigations only once per tile. Track when we have logged an impression or navigation for a tile, then ignore it if we are asked to log an additional one for the same tile. BUG=625163,626681 ==========
#4 is now mostly implemented. We log stats when the user navigates away, and reset when they navigate back. In order to do this, I had to remove the code that flushes stats when the most visited sites change. That code was causing the NTP URL to be updated spuriously and prevented us from knowing when the user navigated to/from the NTP. The problem this introduces is that we may now be inflating stats, because a long-running NTP might receive stats multiple times. (there's probably problems on login, when tiles switch from client to server, but that's mostly out-of-scope) My proposal is to do the same thing for tiles that I'm doing here for impressions. When a tile's title/favicon/thumbnail loads, we mark a load time for (source, whatever). If we already have a load time for (source, whatever), we keep the old one, since we already managed to load a tile. When we log stats, we take the max for the source. We could still inflate stats when a new user starts with <8 tiles, has a long-running NTP tab, and builds up top sites in other tabs. An alternate proposal would be that we just expose a hook to JS saying "log stats when you've finished N loads". We'd change our NTP to use that, but lose stats for external ones.
The CQ bit was checked by sfiera@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/07/11 16:44:57, sfiera wrote: > #4 is now mostly implemented. We log stats when the user navigates away, and > reset when they navigate back. > > In order to do this, I had to remove the code that flushes stats when the most > visited sites change. That code was causing the NTP URL to be updated spuriously > and prevented us from knowing when the user navigated to/from the NTP. The > problem this introduces is that we may now be inflating stats, because a > long-running NTP might receive stats multiple times. I don't get this. The NTP will emit stats multiple times only when the user navigates away and back, right? > (there's probably problems on login, when tiles switch from client to server, > but that's mostly out-of-scope) Acknowledged. > My proposal is to do the same thing for tiles that I'm doing here for > impressions. When a tile's title/favicon/thumbnail loads, we mark a load time > for (source, whatever). If we already have a load time for (source, whatever), > we keep the old one, since we already managed to load a tile. When we log stats, > we take the max for the source. But that would require that the JS tells us what exactly was loaded, right? At the moment, we only get "something loaded", without title/thumbnail info, and without even an index. > We could still inflate stats when a new user starts with <8 tiles, has a > long-running NTP tab, and builds up top sites in other tabs. > > An alternate proposal would be that we just expose a hook to JS saying "log > stats when you've finished N loads". We'd change our NTP to use that, but lose > stats for external ones. While the remote NTP exists, I'd like to avoid changing the JS hooks. Versioning is a major hassle...
https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:95: // detecting when the user leaves or returns to the NTP. ...wait, what? It can change while the NTP is open?! https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:97: // TODO(sfiera): move to a world where the NTP URL is always chrome://newtab/. hahahahaha... wait, you're serious. Let me laugh even harder :D https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:141: if (position >= static_cast<int>(impression_was_logged_.size())) { Hm, might as well just resize this to kNumMostVisited once https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:184: // content::WebContentsObserver override Any reason for moving this up here? (Bad rebase? I recently moved it down, when I made it private) https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:188: if (load_details.previous_url.is_valid() We could keep the early-out if the previous URL isn't valid, and make things a bit easier to read. https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:191: EmitNtpStatistics(EmitReason::NAVIGATED_AWAY); Is there no "return" here on purpose? (So that a reload will both emit and reset stats?) If so, that might be worth a comment. https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:198: search::MatchesOriginAndPath(load_details.entry->GetURL(), ntp_url_)) { Fun fact: search::MatchesOriginAndPath isn't symmetric - it'll consider "http" equal to "https" in only one direction; I can't remember which. But do we even need this here? Couldn't we just compare the URLs for equality? https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:200: impression_was_logged_.clear(); So LoadTime and NumberOfTiles won't be logged again, because has_emitted_ is still true. Can we simply reset that too? Would be nice to be consistent. https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right): https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:30: // MUST be called only when the NTP is active. Can we DCHECK this? search::IsInstantNTP might be the right thing.
https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:95: // detecting when the user leaves or returns to the NTP. On 2016/07/12 08:17:46, Marc Treib wrote: > ...wait, what? It can change while the NTP is open?! Yep. Reproduction: 1. Start Chromium with a clean profile 2. Switch to the second tab ("Getting Started") 3. Go to chromium.org or wherever 4. Switch back to the first tab 5. Wait (1 to 20 seconds) 6. Ta-da! Tiles have changed. In fact, new tiles are sent every time TopSites/MostLikely changes, even if there isn't an NTP active in the tab, it's just that without the NTP nothing will be listening for them. (that's where http://crbug.com/626681 comes from)
https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:95: // detecting when the user leaves or returns to the NTP. On 2016/07/12 08:37:33, sfiera wrote: > On 2016/07/12 08:17:46, Marc Treib wrote: > > ...wait, what? It can change while the NTP is open?! > > Yep. Reproduction: > > 1. Start Chromium with a clean profile > 2. Switch to the second tab ("Getting Started") > 3. Go to http://chromium.org or wherever > 4. Switch back to the first tab > 5. Wait (1 to 20 seconds) > 6. Ta-da! Tiles have changed. The *tiles* can change, but the NTP URL? I guess it can happen when there's a redirect from remote to local NTP? (That happens when the default search provider changes, which happens a few seconds after startup - from google.com to google.de.) > In fact, new tiles are sent every time TopSites/MostLikely changes, even if > there isn't an NTP active in the tab, it's just that without the NTP nothing > will be listening for them. (that's where http://crbug.com/626681 comes from) Yup, but that should be fixed by this CL, right?
https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:95: // detecting when the user leaves or returns to the NTP. On 2016/07/12 08:58:47, Marc Treib wrote: > On 2016/07/12 08:37:33, sfiera wrote: > > On 2016/07/12 08:17:46, Marc Treib wrote: > > > ...wait, what? It can change while the NTP is open?! > > > > Yep. Reproduction: > > > > 1. Start Chromium with a clean profile > > 2. Switch to the second tab ("Getting Started") > > 3. Go to http://chromium.org or wherever > > 4. Switch back to the first tab > > 5. Wait (1 to 20 seconds) > > 6. Ta-da! Tiles have changed. > > The *tiles* can change, but the NTP URL? I guess it can happen when there's a > redirect from remote to local NTP? (That happens when the default search > provider changes, which happens a few seconds after startup - from http://google.com to > google.de.) Oh, misread that. But still yes. On first launch, I get: [89280:89280:0712/105122:VERBOSE1:ntp_user_data_logger.cc(101)] NTP URL changed from "https://www.google.com/_/chrome/newtab?espv=2&ie=UTF-8" to "chrome-search://local-ntp/local-ntp.html" I don't get it on subsequent runs. Weirdly, on changing the default search engine, and therefore the NTP, the tab remains open but ceases to be an NTP: the Bing NTP is redirected to bing.com; the Google NTP redirects to google.com. That's detected correctly as "leaving", here. > > In fact, new tiles are sent every time TopSites/MostLikely changes, even if > > there isn't an NTP active in the tab, it's just that without the NTP nothing > > will be listening for them. (that's where http://crbug.com/626681 comes from) > > Yup, but that should be fixed by this CL, right? Not fixed as far as I understand it. SearchTabHelper::MostVisitedItemsChanged() will still call ipc_router_.SendMostVisitedItems(), but there's nothing listening on the other end.
Still haven't started looking at comments, but a few notes on the new behavior: - Going back to the NTP or reloading it now generates a new set of statistics (load time, number of tiles, and impressions). Nice. We could break this out separately from the Startup/NewTab stats if we wanted. - Unfortunately, NewTabPage.LoadTime.Startup apparently broke. All load times are recorded as NewTab. I don't think I broke it but I'll do a bisect to figure out when and how it did. - If an NTP finishes loading, then gets new tiles later while open, we record additional impressions, but number of tiles is already recorded. I think this is probably working-as-intended. - Similarly, if the user logs in while the NTP is open, we get additional impressions but number of tiles remains. The new impressions are recorded as client, but that's an edge case that we don't really care about. - It also seems like the next NTP opened after logging also gets recorded as client. This sounds like there might be a more important underlying bug and I'll look at it further. - On first launch, which the redirection to the local NTP happens, we only record one set of statistics (as Web, not LocalNTP). - If I open the local NTP manually as chrome-search://local-ntp/local-ntp.html, I seem to get MostLikely tiles. They are recorded as client impressions, but with Web load times. Another edge case that's not worth spending time on.
On 2016/07/13 09:49:31, sfiera wrote: > Still haven't started looking at comments, but a few notes on the new behavior: > > - Going back to the NTP or reloading it now generates a new set of statistics > (load time, number of tiles, and impressions). Nice. We could break this out > separately from the Startup/NewTab stats if we wanted. Hm, maybe? I don't really see what the difference between "NewTab" and a hypothetical "Revisit" would be? > - Unfortunately, NewTabPage.LoadTime.Startup apparently broke. All load times > are recorded as NewTab. I don't think I broke it but I'll do a bisect to figure > out when and how it did. Check UMA first, I *think* this only ever worked on Windows. > - If an NTP finishes loading, then gets new tiles later while open, we record > additional impressions, but number of tiles is already recorded. I think this is > probably working-as-intended. That's only when *additional* tiles come in, right? Yes, that sounds fine (and rare, anyway). > - Similarly, if the user logs in while the NTP is open, we get additional > impressions but number of tiles remains. The new impressions are recorded as > client, but that's an edge case that we don't really care about. Ack. > - It also seems like the next NTP opened after logging also gets recorded as > client. This sounds like there might be a more important underlying bug and I'll > look at it further. Huh, that's interesting. Yup, warrants some investigation. > - On first launch, which the redirection to the local NTP happens, we only > record one set of statistics (as Web, not LocalNTP). OK, IMO that's acceptable. > - If I open the local NTP manually as chrome-search://local-ntp/local-ntp.html, > I seem to get MostLikely tiles. They are recorded as client impressions, but > with Web load times. Another edge case that's not worth spending time on. Oh wow. Pretty sure you don't get MostLikely, I don't see how? But the Web load times are something we should investigate - does the ntp_url_ thing get messed up again?
Description was changed from ========== Record impressions/navigations only once per tile. Track when we have logged an impression or navigation for a tile, then ignore it if we are asked to log an additional one for the same tile. BUG=625163,626681 ========== to ========== Record impressions/navigations only once per tile. Track when we have logged an impression or navigation for a tile, then ignore it if we are asked to log an additional one for the same tile. If we detect that the user has returned to the NTP after navigating away, reset the tracker (and has_emitted_) so that we can collect stats again. BUG=625163,626681 ==========
The CQ bit was checked by sfiera@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sfiera@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...
https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:97: // TODO(sfiera): move to a world where the NTP URL is always chrome://newtab/. On 2016/07/12 08:17:46, Marc Treib wrote: > hahahahaha... wait, you're serious. Let me laugh even harder :D Restricted the TODO somewhat--I expect it's a non-goal to move external NTPs to chrome://newtab/ anyway. https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:141: if (position >= static_cast<int>(impression_was_logged_.size())) { On 2016/07/12 08:17:46, Marc Treib wrote: > Hm, might as well just resize this to kNumMostVisited once Do we have a guarantee that we can't be called with >= 8? If an external NTP creates a 9th tile, could we get here? (external NTPs still log impressions, I think) https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:184: // content::WebContentsObserver override On 2016/07/12 08:17:46, Marc Treib wrote: > Any reason for moving this up here? (Bad rebase? I recently moved it down, when > I made it private) Yep, that. Down. https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:188: if (load_details.previous_url.is_valid() On 2016/07/12 08:17:46, Marc Treib wrote: > We could keep the early-out if the previous URL isn't valid, and make things a > bit easier to read. This conditional is now gone, because we don't emit stats here, so I left the other one untouched. https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:191: EmitNtpStatistics(EmitReason::NAVIGATED_AWAY); On 2016/07/12 08:17:46, Marc Treib wrote: > Is there no "return" here on purpose? (So that a reload will both emit and reset > stats?) If so, that might be worth a comment. Conditional is gone. (this did handle reload) https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:198: search::MatchesOriginAndPath(load_details.entry->GetURL(), ntp_url_)) { On 2016/07/12 08:17:46, Marc Treib wrote: > Fun fact: search::MatchesOriginAndPath isn't symmetric - it'll consider "http" > equal to "https" in only one direction; I can't remember which. > But do we even need this here? Couldn't we just compare the URLs for equality? Changed to ==. https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:200: impression_was_logged_.clear(); On 2016/07/12 08:17:46, Marc Treib wrote: > So LoadTime and NumberOfTiles won't be logged again, because has_emitted_ is > still true. Can we simply reset that too? Would be nice to be consistent. Reset has_emitted_. https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right): https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:30: // MUST be called only when the NTP is active. On 2016/07/12 08:17:46, Marc Treib wrote: > Can we DCHECK this? search::IsInstantNTP might be the right thing. DCHECK added.
https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:95: // detecting when the user leaves or returns to the NTP. On 2016/07/12 09:08:58, sfiera wrote: > On 2016/07/12 08:58:47, Marc Treib wrote: > > On 2016/07/12 08:37:33, sfiera wrote: > > > On 2016/07/12 08:17:46, Marc Treib wrote: > > > > ...wait, what? It can change while the NTP is open?! > > > > > > Yep. Reproduction: > > > > > > 1. Start Chromium with a clean profile > > > 2. Switch to the second tab ("Getting Started") > > > 3. Go to http://chromium.org or wherever > > > 4. Switch back to the first tab > > > 5. Wait (1 to 20 seconds) > > > 6. Ta-da! Tiles have changed. > > > > The *tiles* can change, but the NTP URL? I guess it can happen when there's a > > redirect from remote to local NTP? (That happens when the default search > > provider changes, which happens a few seconds after startup - from > http://google.com to > > google.de.) > > Oh, misread that. But still yes. On first launch, I get: > > [89280:89280:0712/105122:VERBOSE1:ntp_user_data_logger.cc(101)] NTP URL changed > from "https://www.google.com/_/chrome/newtab?espv=2&ie=UTF-8" to > "chrome-search://local-ntp/local-ntp.html" > > I don't get it on subsequent runs. On subsequent runs, the Google TLD doesn't change anymore :) The code that does the redirect is here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_instant_contro... > Weirdly, on changing the default search engine, and therefore the NTP, the tab > remains open but ceases to be an NTP: the Bing NTP is redirected to http://bing.com; > the Google NTP redirects to http://google.com. That's detected correctly as "leaving", > here. That's also expected: The same code I linked above reloads the tab; the remote NTP JS detects that the embeddedSearch API isn't available, and redirects to the home page. > > > In fact, new tiles are sent every time TopSites/MostLikely changes, even if > > > there isn't an NTP active in the tab, it's just that without the NTP nothing > > > will be listening for them. (that's where http://crbug.com/626681 comes > from) > > > > Yup, but that should be fixed by this CL, right? > > Not fixed as far as I understand it. SearchTabHelper::MostVisitedItemsChanged() > will still call ipc_router_.SendMostVisitedItems(), but there's nothing > listening on the other end. I meant that http://crbug.com/626681 (wrong NTP URL stored here) should be fixed. The MV items will still be sent which is kinda pointless, but shouldn't actually hurt, right? https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:97: // TODO(sfiera): move to a world where the NTP URL is always chrome://newtab/. On 2016/07/14 14:11:08, sfiera wrote: > On 2016/07/12 08:17:46, Marc Treib wrote: > > hahahahaha... wait, you're serious. Let me laugh even harder :D > > Restricted the TODO somewhat--I expect it's a non-goal to move external NTPs to > chrome://newtab/ anyway. External as in remote? No. We might conceivably move the local NTP to chrome://newtab, but that might be more trouble than it's worth - quite a lot of stuff builds on the chrome-search:// scheme. https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:141: if (position >= static_cast<int>(impression_was_logged_.size())) { On 2016/07/14 14:11:07, sfiera wrote: > On 2016/07/12 08:17:46, Marc Treib wrote: > > Hm, might as well just resize this to kNumMostVisited once > > Do we have a guarantee that we can't be called with >= 8? If an external NTP > creates a 9th tile, could we get here? (external NTPs still log impressions, I > think) No guarantee, but the histograms support only 8 bins anyway. So if the position is >= 8, we might just early-out. https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:221: number_of_tiles_ = 0; I was gonna say "Also reset has_[server|client]_side_suggestions_", but shouldn't they and number_of_tiles_ be in a clean state anyway? https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right): https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:18: using Samples = std::vector<base::HistogramBase::Sample>; nit: Also add "using base::HistogramBase::Sample" for consistency?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:141: if (position >= static_cast<int>(impression_was_logged_.size())) { On 2016/07/14 14:40:52, Marc Treib wrote: > On 2016/07/14 14:11:07, sfiera wrote: > > On 2016/07/12 08:17:46, Marc Treib wrote: > > > Hm, might as well just resize this to kNumMostVisited once > > > > Do we have a guarantee that we can't be called with >= 8? If an external NTP > > creates a 9th tile, could we get here? (external NTPs still log impressions, I > > think) > > No guarantee, but the histograms support only 8 bins anyway. So if the position > is >= 8, we might just early-out. Fixed to be 8 always. https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:221: number_of_tiles_ = 0; On 2016/07/14 14:40:52, Marc Treib wrote: > I was gonna say "Also reset has_[server|client]_side_suggestions_" Done. > but shouldn't they and number_of_tiles_ be in a clean state anyway? Nope. When we get redirected to the local NTP, we log stats for the remote NTP, then get a second set of logged items, but don't emit them (because of has_emitted_). If the user goes forward and back, they get 4 tiles the first time, held over from the local NTP's stats. https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right): https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:18: using Samples = std::vector<base::HistogramBase::Sample>; On 2016/07/14 14:40:52, Marc Treib wrote: > nit: Also add "using base::HistogramBase::Sample" for consistency? Done.
The CQ bit was checked by sfiera@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...
Final nits :) https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:221: number_of_tiles_ = 0; On 2016/07/15 09:55:10, sfiera wrote: > On 2016/07/14 14:40:52, Marc Treib wrote: > > I was gonna say "Also reset has_[server|client]_side_suggestions_" > > Done. > > > but shouldn't they and number_of_tiles_ be in a clean state anyway? > > Nope. When we get redirected to the local NTP, we log stats for the remote NTP, > then get a second set of logged items, but don't emit them (because of > has_emitted_). If the user goes forward and back, they get 4 tiles the first > time, held over from the local NTP's stats. Acknowledged. https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:219: has_client_side_suggestions_ = true; These should be set to false https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right): https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:11: #include "base/gtest_prod_util.h" nit: empty line between <> and "" includes https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:73: // sources, such as logging in (switching from client to server tiles), then nit: s/logging/signing (that's the usual nomenclature) https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:76: std::bitset<8> impression_was_logged_; Please use the kNumMostVisited constant instead of a magic number (promote kNumMostVisited to a private static member)
https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:219: has_client_side_suggestions_ = true; On 2016/07/15 10:04:46, Marc Treib wrote: > These should be set to false Hein, that's embarrassing. I had looked for a way to trigger NavigationEntryCommitted() from the test to check this behavior, but I didn't see any way to create load_details.entry. Found a workaround. (although it's still not properly tested, because there's no test on load times and I don't know the right test APIs for histograms to add it) https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right): https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:11: #include "base/gtest_prod_util.h" On 2016/07/15 10:04:46, Marc Treib wrote: > nit: empty line between <> and "" includes Done. https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:73: // sources, such as logging in (switching from client to server tiles), then On 2016/07/15 10:04:46, Marc Treib wrote: > nit: s/logging/signing (that's the usual nomenclature) Done. https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:76: std::bitset<8> impression_was_logged_; On 2016/07/15 10:04:46, Marc Treib wrote: > Please use the kNumMostVisited constant instead of a magic number (promote > kNumMostVisited to a private static member) Done.
The CQ bit was checked by sfiera@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.
LGTM, thanks! https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:219: has_client_side_suggestions_ = true; On 2016/07/15 10:26:00, sfiera wrote: > On 2016/07/15 10:04:46, Marc Treib wrote: > > These should be set to false > > Hein, that's embarrassing. > > I had looked for a way to trigger NavigationEntryCommitted() from the test to > check this behavior, but I didn't see any way to create load_details.entry. > Found a workaround. Nice, I actually find that easier to read than the previous code. > (although it's still not properly tested, because there's no test on load times > and I don't know the right test APIs for histograms to add it) There's some examples of timing-related tests here: https://cs.chromium.org/chromium/src/components/ntp_snippets/ntp_snippets_fet... You don't have to add any LoadTime tests in this CL though. https://codereview.chromium.org/2124903003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right): https://codereview.chromium.org/2124903003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:134: if ((position >= kNumMostVisited) || (impression_was_logged_[position])) { optional nit: no parens required around impression_was_logged_[position]
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2124903003/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Record impressions/navigations only once per tile. Track when we have logged an impression or navigation for a tile, then ignore it if we are asked to log an additional one for the same tile. If we detect that the user has returned to the NTP after navigating away, reset the tracker (and has_emitted_) so that we can collect stats again. BUG=625163,626681 ========== to ========== Record impressions/navigations only once per tile. Track when we have logged an impression or navigation for a tile, then ignore it if we are asked to log an additional one for the same tile. If we detect that the user has returned to the NTP after navigating away, reset the tracker (and has_emitted_) so that we can collect stats again. BUG=625163,626681 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Record impressions/navigations only once per tile. Track when we have logged an impression or navigation for a tile, then ignore it if we are asked to log an additional one for the same tile. If we detect that the user has returned to the NTP after navigating away, reset the tracker (and has_emitted_) so that we can collect stats again. BUG=625163,626681 ========== to ========== Record impressions/navigations only once per tile. Track when we have logged an impression or navigation for a tile, then ignore it if we are asked to log an additional one for the same tile. If we detect that the user has returned to the NTP after navigating away, reset the tracker (and has_emitted_) so that we can collect stats again. BUG=625163,626681 Committed: https://crrev.com/61b6eba8e654d433215a148f03f7eeac708c73ab Cr-Commit-Position: refs/heads/master@{#405751} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/61b6eba8e654d433215a148f03f7eeac708c73ab Cr-Commit-Position: refs/heads/master@{#405751} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
