Field trial removing tiles from NTP if URL is already open - for 1993 clients

Adding support for 1993 clients to part B of NTP Most Visited Tile Placement Experiment.

Part B of the experiment removes a tile from the NTP if the suggested URL is already open in one of the current browser's tabs. Also adding UMA stats to parts A (shuffle tile placement) and B of the experiment.

BUG=237911
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213947

[annark1, 2013-06-14 21:11:11]

Hi Philippe,

This is the experiment with 1993 code plus some UMA stats added. PTAL.

Thanks!
Anna

[beaudoin, 2013-06-20 21:55:44]

LGTM with a few nits.

For the big proposed clean-up, please check with the owner to see what he
thinks.

https://codereview.chromium.org/17114002/diff/1/chrome/browser/history/top_si...
File chrome/browser/history/top_sites.cc (right):

https://codereview.chromium.org/17114002/diff/1/chrome/browser/history/top_si...
chrome/browser/history/top_sites.cc:117: // would cause the number of Most
Visited recommendations to drop below eight.
We do not use comments describing >1 function. Put the comment on one of the
function, preferably in the .h, and ensure the other 2 refer to it.

https://codereview.chromium.org/17114002/diff/1/chrome/browser/history/top_si...
chrome/browser/history/top_sites.cc:180: }
These two functions are really quite similar. We could probably use some
Callback<> and base::Bind() to simplify this. (See doc in
http://src.chromium.org/viewvc/chrome/trunk/src/base/callback.h?content-type=...
)

I'm thinking of something like:

namespace {

const std::string& GetUrlFromVector(std::vector<InstantMostVisitedItem>* items,
size_t i) {
  return (*items)[i].url.spec();
}

const std::string& GetUrlFromPageValue(base::ListValue* pages_value, size_t i) {
  base::DictionaryValue* page_value;
  std::string url;
  if (!pages_value->GetDictionary(i, &page_value) ||
      !page_value->GetString("url", &url))
    return std::string();
  return url;
}

void RemoveUrlFromVector(base::ListValue* pages_value, size_t i) { ... }

void RemoveUrlFromPageValue(base::ListValue* pages_value, size_t i) { ... }


void RemoveUrlsMatchingOpenTabs(
    const std::set<std::string>& open_urls,
    size_t num_urls,
    const base::Callback<const std::string&(size_t)>& getUrl,
    const base::Callback<void(size_t)>& removeUrl) {

  // Your big method here, invoking the callbacks with Run();

}

}  // namespace

void TopSites::RemoveItemsMatchingOpenTabs(
    const std::set<std::string>& open_urls,
    std::vector<InstantMostVisitedItem>* items) {
  RemoveUrlsMatchingOpenTabs(open_urls, items->size(),
                             base::bind(&GetUrlFromVector, open_urls),
                             base::bind(&RemoveUrlFromVector, open_urls));
}

void TopSites::RemovePageValuesMatchingOpenTabs(...) { ... }


Not entirely sure it's worth it. You may want to see what the owner thinks but
if you have some free cycles it's a nice exercise. :)

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.h:60: TabStripModel*
tab_strip_model() const;
Can you return a const TabStripModel* ?

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.h:446: // in the Browser.
Rephrase: Removes recommended URLs ... if the relevant experiment is enabled.

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/webui/ntp/m...
File chrome/browser/ui/webui/ntp/most_visited_handler.h (right):

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/webui/ntp/m...
chrome/browser/ui/webui/ntp/most_visited_handler.h:94: // in the Browser.
Same here.

https://codereview.chromium.org/17114002/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/17114002/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:7184: Histogram of actions carried out
by Most Visited Tile Placement experiment.
This could be a bit more detailed. As such, it's unclear what this is for.
Explain that we want to identify the cases where the experiment could not
operate as expected and the reason for it.

https://codereview.chromium.org/17114002/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:15889: The types of actions performed by
the Most Visited Tile Placement experiment
Same here, a bit more details.

[beaudoin, 2013-07-03 18:33:05]

On 2013/06/20 21:55:44, beaudoin wrote:
> LGTM with a few nits.
> 
> For the big proposed clean-up, please check with the owner to see what he
> thinks.
> 
>
https://codereview.chromium.org/17114002/diff/1/chrome/browser/history/top_si...
> File chrome/browser/history/top_sites.cc (right):
> 
>
https://codereview.chromium.org/17114002/diff/1/chrome/browser/history/top_si...
> chrome/browser/history/top_sites.cc:117: // would cause the number of Most
> Visited recommendations to drop below eight.
> We do not use comments describing >1 function. Put the comment on one of the
> function, preferably in the .h, and ensure the other 2 refer to it.
> 
>
https://codereview.chromium.org/17114002/diff/1/chrome/browser/history/top_si...
> chrome/browser/history/top_sites.cc:180: }
> These two functions are really quite similar. We could probably use some
> Callback<> and base::Bind() to simplify this. (See doc in
>
http://src.chromium.org/viewvc/chrome/trunk/src/base/callback.h?content-type=...
> )
> 
> I'm thinking of something like:
> 
> namespace {
> 
> const std::string& GetUrlFromVector(std::vector<InstantMostVisitedItem>*
items,
> size_t i) {
>   return (*items)[i].url.spec();
> }
> 
> const std::string& GetUrlFromPageValue(base::ListValue* pages_value, size_t i)
{
>   base::DictionaryValue* page_value;
>   std::string url;
>   if (!pages_value->GetDictionary(i, &page_value) ||
>       !page_value->GetString("url", &url))
>     return std::string();
>   return url;
> }
> 
> void RemoveUrlFromVector(base::ListValue* pages_value, size_t i) { ... }
> 
> void RemoveUrlFromPageValue(base::ListValue* pages_value, size_t i) { ... }
> 
> 
> void RemoveUrlsMatchingOpenTabs(
>     const std::set<std::string>& open_urls,
>     size_t num_urls,
>     const base::Callback<const std::string&(size_t)>& getUrl,
>     const base::Callback<void(size_t)>& removeUrl) {
> 
>   // Your big method here, invoking the callbacks with Run();
> 
> }
> 
> }  // namespace
> 
> void TopSites::RemoveItemsMatchingOpenTabs(
>     const std::set<std::string>& open_urls,
>     std::vector<InstantMostVisitedItem>* items) {
>   RemoveUrlsMatchingOpenTabs(open_urls, items->size(),
>                              base::bind(&GetUrlFromVector, open_urls),
>                              base::bind(&RemoveUrlFromVector, open_urls));
> }
> 
> void TopSites::RemovePageValuesMatchingOpenTabs(...) { ... }
> 
> 
> Not entirely sure it's worth it. You may want to see what the owner thinks but
> if you have some free cycles it's a nice exercise. :)
> 
>
https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/browser_ins...
> File chrome/browser/ui/browser_instant_controller.h (right):
> 
>
https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/browser_ins...
> chrome/browser/ui/browser_instant_controller.h:60: TabStripModel*
> tab_strip_model() const;
> Can you return a const TabStripModel* ?
> 
>
https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/search/inst...
> File chrome/browser/ui/search/instant_controller.h (right):
> 
>
https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/search/inst...
> chrome/browser/ui/search/instant_controller.h:446: // in the Browser.
> Rephrase: Removes recommended URLs ... if the relevant experiment is enabled.
> 
>
https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/webui/ntp/m...
> File chrome/browser/ui/webui/ntp/most_visited_handler.h (right):
> 
>
https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/webui/ntp/m...
> chrome/browser/ui/webui/ntp/most_visited_handler.h:94: // in the Browser.
> Same here.
> 
>
https://codereview.chromium.org/17114002/diff/1/tools/metrics/histograms/hist...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/17114002/diff/1/tools/metrics/histograms/hist...
> tools/metrics/histograms/histograms.xml:7184: Histogram of actions carried out
> by Most Visited Tile Placement experiment.
> This could be a bit more detailed. As such, it's unclear what this is for.
> Explain that we want to identify the cases where the experiment could not
> operate as expected and the reason for it.
> 
>
https://codereview.chromium.org/17114002/diff/1/tools/metrics/histograms/hist...
> tools/metrics/histograms/histograms.xml:15889: The types of actions performed
by
> the Most Visited Tile Placement experiment
> Same here, a bit more details.

Anna: Is this still happening? If so, please also update the issue title and
description. Right now it's unclear precisely what you're doing here.

[annark1, 2013-07-04 18:29:14]

Adding owners:
estade@ for most_visited_handler.cc/.h
asvitkine@ for histograms.xml
brettw@ for everything else

PTAL. 

brettw, Philippe and I have discussed and we are wondering whether it is even
worth doing the Callback/Bind suggestion in top_sites.cc. We would appreciate
any thoughts you have on this.

Thanks for your reviews.

https://codereview.chromium.org/17114002/diff/1/chrome/browser/history/top_si...
File chrome/browser/history/top_sites.cc (right):

https://codereview.chromium.org/17114002/diff/1/chrome/browser/history/top_si...
chrome/browser/history/top_sites.cc:117: // would cause the number of Most
Visited recommendations to drop below eight.
On 2013/06/20 21:55:44, beaudoin wrote:
> We do not use comments describing >1 function. Put the comment on one of the
> function, preferably in the .h, and ensure the other 2 refer to it.

Done.

https://codereview.chromium.org/17114002/diff/1/chrome/browser/history/top_si...
chrome/browser/history/top_sites.cc:180: }
brettw, do you have a preference on how this should be done? 


On 2013/06/20 21:55:44, beaudoin wrote:
> These two functions are really quite similar. We could probably use some
> Callback<> and base::Bind() to simplify this. (See doc in
>
http://src.chromium.org/viewvc/chrome/trunk/src/base/callback.h?content-type=...
> )
> 
> I'm thinking of something like:
> 
> namespace {
> 
> const std::string& GetUrlFromVector(std::vector<InstantMostVisitedItem>*
items,
> size_t i) {
>   return (*items)[i].url.spec();
> }
> 
> const std::string& GetUrlFromPageValue(base::ListValue* pages_value, size_t i)
{
>   base::DictionaryValue* page_value;
>   std::string url;
>   if (!pages_value->GetDictionary(i, &page_value) ||
>       !page_value->GetString("url", &url))
>     return std::string();
>   return url;
> }
> 
> void RemoveUrlFromVector(base::ListValue* pages_value, size_t i) { ... }
> 
> void RemoveUrlFromPageValue(base::ListValue* pages_value, size_t i) { ... }
> 
> 
> void RemoveUrlsMatchingOpenTabs(
>     const std::set<std::string>& open_urls,
>     size_t num_urls,
>     const base::Callback<const std::string&(size_t)>& getUrl,
>     const base::Callback<void(size_t)>& removeUrl) {
> 
>   // Your big method here, invoking the callbacks with Run();
> 
> }
> 
> }  // namespace
> 
> void TopSites::RemoveItemsMatchingOpenTabs(
>     const std::set<std::string>& open_urls,
>     std::vector<InstantMostVisitedItem>* items) {
>   RemoveUrlsMatchingOpenTabs(open_urls, items->size(),
>                              base::bind(&GetUrlFromVector, open_urls),
>                              base::bind(&RemoveUrlFromVector, open_urls));
> }
> 
> void TopSites::RemovePageValuesMatchingOpenTabs(...) { ... }
> 
> 
> Not entirely sure it's worth it. You may want to see what the owner thinks but
> if you have some free cycles it's a nice exercise. :)

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.h:60: TabStripModel*
tab_strip_model() const;
On 2013/06/20 21:55:44, beaudoin wrote:
> Can you return a const TabStripModel* ?

Done.

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.h:446: // in the Browser.
On 2013/06/20 21:55:44, beaudoin wrote:
> Rephrase: Removes recommended URLs ... if the relevant experiment is enabled.

Done.

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/webui/ntp/m...
File chrome/browser/ui/webui/ntp/most_visited_handler.h (right):

https://codereview.chromium.org/17114002/diff/1/chrome/browser/ui/webui/ntp/m...
chrome/browser/ui/webui/ntp/most_visited_handler.h:94: // in the Browser.
On 2013/06/20 21:55:44, beaudoin wrote:
> Same here.

Done.

https://codereview.chromium.org/17114002/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/17114002/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:7184: Histogram of actions carried out
by Most Visited Tile Placement experiment.
On 2013/06/20 21:55:44, beaudoin wrote:
> This could be a bit more detailed. As such, it's unclear what this is for.
> Explain that we want to identify the cases where the experiment could not
> operate as expected and the reason for it.

Done.

https://codereview.chromium.org/17114002/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:15889: The types of actions performed by
the Most Visited Tile Placement experiment
On 2013/06/20 21:55:44, beaudoin wrote:
> Same here, a bit more details.

Done.

[Alexei Svitkine (slow), 2013-07-04 20:28:29]

https://codereview.chromium.org/17114002/diff/14001/chrome/browser/history/to...
File chrome/browser/history/top_sites.cc (right):

https://codereview.chromium.org/17114002/diff/14001/chrome/browser/history/to...
chrome/browser/history/top_sites.cc:84:
"NewTabPage.MostVisitedTilePlacementExperiment",
I suggest making a helper function to log this histogram that just takes the
enum value.

https://codereview.chromium.org/17114002/diff/14001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17114002/diff/14001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:119: const history::TopSites& ts,
Nit: ts -> top_sites

https://codereview.chromium.org/17114002/diff/14001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:332:
MaybeRemoveMostVisitedItems(&items_copy);
Is |items_copy| being used anywhere? Did you mean to invoke the calls below on
items_copy instead of items?

https://codereview.chromium.org/17114002/diff/14001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:699: #if !defined(OS_ANDROID)
Add a comment about why this shouldn't apply to android.

[Evan Stade, 2013-07-08 20:25:07]

most visited handler lgtm

[sky, 2013-07-16 15:54:30]

Can you upload again, latest patch doesn't contain side-by-side diffs.

[annark, 2013-07-16 16:02:46]

Thanks for letting me know - fixed now.


On Tue, Jul 16, 2013 at 11:54 AM, <sky@chromium.org> wrote:

> Can you upload again, latest patch doesn't contain side-by-side diffs.
>
>
https://codereview.chromium.**org/17114002/<https://codereview.chromium.org/1...
>

[annark1, 2013-07-16 18:07:08]

https://chromiumcodereview.appspot.com/17114002/diff/14001/chrome/browser/his...
File chrome/browser/history/top_sites.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/14001/chrome/browser/his...
chrome/browser/history/top_sites.cc:84:
"NewTabPage.MostVisitedTilePlacementExperiment",
On 2013/07/04 20:28:30, Alexei Svitkine wrote:
> I suggest making a helper function to log this histogram that just takes the
> enum value.

Done.

https://chromiumcodereview.appspot.com/17114002/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/14001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:119: const history::TopSites& ts,
On 2013/07/04 20:28:30, Alexei Svitkine wrote:
> Nit: ts -> top_sites

Done.

https://chromiumcodereview.appspot.com/17114002/diff/14001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:332:
MaybeRemoveMostVisitedItems(&items_copy);
Yes, I did. Thank you for catching this error.

On 2013/07/04 20:28:30, Alexei Svitkine wrote:
> Is |items_copy| being used anywhere? Did you mean to invoke the calls below on
> items_copy instead of items?

https://chromiumcodereview.appspot.com/17114002/diff/14001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:699: #if !defined(OS_ANDROID)
On 2013/07/04 20:28:30, Alexei Svitkine wrote:
> Add a comment about why this shouldn't apply to android.

Done.

[sky, 2013-07-16 21:15:06]

I get whey you're adding this logic to TopSites, but I think it should be kept
in its own separate file. TopSites should be kept at just the interface TopSites
needs to implement. Not the interface plus a handful of some what related
methods.

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
File chrome/browser/history/top_sites.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
chrome/browser/history/top_sites.cc:115: const std::string url =
(*items)[i].url.spec();
const std::string&

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
chrome/browser/history/top_sites.cc:124: items->erase(items->begin()+i);
use spaces

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
chrome/browser/history/top_sites.cc:137: void
TopSites::RemovePageValuesMatchingOpenTabs(
Can't we implement one of these in terms of the other?

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
File chrome/browser/history/top_sites.h (right):

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
chrome/browser/history/top_sites.h:58: static bool IsClientInTabsGroup();
tabs group? I think this method is poorly named.

[annark1, 2013-07-17 17:45:52]

Please see my comments below.

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
File chrome/browser/history/top_sites.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
chrome/browser/history/top_sites.cc:115: const std::string url =
(*items)[i].url.spec();
On 2013/07/16 21:15:07, sky wrote:
> const std::string&

Done.

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
chrome/browser/history/top_sites.cc:124: items->erase(items->begin()+i);
On 2013/07/16 21:15:07, sky wrote:
> use spaces

Done.

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
chrome/browser/history/top_sites.cc:137: void
TopSites::RemovePageValuesMatchingOpenTabs(
I've discussed with Phillippe, and given the fact that they don't share an
interface, we don't see how I can do that.

On 2013/07/16 21:15:07, sky wrote:
> Can't we implement one of these in terms of the other?

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
File chrome/browser/history/top_sites.h (right):

https://chromiumcodereview.appspot.com/17114002/diff/44002/chrome/browser/his...
chrome/browser/history/top_sites.h:58: static bool IsClientInTabsGroup();
On 2013/07/16 21:15:07, sky wrote:
> tabs group? I think this method is poorly named.

Done. Renamed to DontShowOpenURLsEnabled

[sky, 2013-07-17 21:30:50]

How about some tests for most_visited_tiles_experiments?

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.cc:44:
LogInHistogram(NTP_TILE_EXPERIMENT_ACTION_TOO_FEW_URLS_TILES_1_8);
can't you early return here and 50?

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.cc:56: if (data->empty() ||
(*data)[index_to_flip].url.is_empty()) {
Can data ever be empty and can a url ever be empty?

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.cc:75: while (i <
items->size()) {
How about a for loop here and below? It need not increment though, but better
matches what's going on, eg:

for (size_t i = 0; i < ...; ) {

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment.h (right):

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.h:30: static bool
DontShowOpenURLsEnabled();
IsDoneShowOpenURLsEnabled?

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.h:60: 
DISALLOW_IMPLICIT_CONSTRUCTORS, or make this a bunch of functions not in a
class.

[annark1, 2013-07-22 20:35:33]

Added tests for most_visited_tiles_exeriment. PTAL.

Thanks!

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.cc:44:
LogInHistogram(NTP_TILE_EXPERIMENT_ACTION_TOO_FEW_URLS_TILES_1_8);
On 2013/07/17 21:30:51, sky wrote:
> can't you early return here and 50?

Done.

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.cc:56: if (data->empty() ||
(*data)[index_to_flip].url.is_empty()) {
Not so far as I can see. I've removed this code now.
On 2013/07/17 21:30:51, sky wrote:
> Can data ever be empty and can a url ever be empty?

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.cc:75: while (i <
items->size()) {
On 2013/07/17 21:30:51, sky wrote:
> How about a for loop here and below? It need not increment though, but better
> matches what's going on, eg:
> 
> for (size_t i = 0; i < ...; ) {

Done.

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment.h (right):

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.h:30: static bool
DontShowOpenURLsEnabled();
On 2013/07/17 21:30:51, sky wrote:
> IsDoneShowOpenURLsEnabled?

Done.

https://chromiumcodereview.appspot.com/17114002/diff/75001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.h:60: 
On 2013/07/17 21:30:51, sky wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS, or make this a bunch of functions not in a
> class.

Done.

[Alexei Svitkine (slow), 2013-07-22 20:59:05]

This change is doing a lot of things.

It would be easier to review if split into several smaller changes - for
example, first change to add the new file and move existing logic to it, without
changing it. A follow-up change to introduce new logic, etc.

Also, please have the first line of the CL description be a short sentence (< 80
chars), with a blank line after it and another paragraph explaining the details.

https://chromiumcodereview.appspot.com/17114002/diff/85001/tools/metrics/hist...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/tools/metrics/hist...
tools/metrics/histograms/histograms.xml:8871: could not operate as expected, and
the reason for it.
Please mention when this histogram gets recorded. Is it consistent between the
cases or do different cases trigger at different times?

[Alexei Svitkine (slow), 2013-07-22 21:14:09]

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.cc:34:
"NewTabPage.MostVisitedTilePlacementExperiment";
Is there a reason you're defining this here and not using it in
MostVisitedTilesExperiment::LogInHistogram()?

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment.h (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.h:49: static bool
IsDontShowOpenURLsEnabled();
Is there a better name you can come up with for this? This is pretty hard to
read.

For example, IsMostVisitedTilesExperimentEnabled()?

Also, this can be made private if MaybeRemoveMostVisitedItems() is moved to this
file.

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:706: if
(!history::MostVisitedTilesExperiment::IsDontShowOpenURLsEnabled())
This function seems to have identical logic between this impl and the one in
MostVisitedHandler::MaybeRemovePageValues().

Can you refactor them to share the implementation? Perhaps putting the new
function on MostVisitedTilesExperiment.

You could pass in tab_strip_model and top_sites, which is the difference between
the two functions in how they're obtained.

Then, you also don't need to duplicate GetOpenUrls().

[annark1, 2013-07-23 19:52:21]

This patch moves the 1993 experiment logic to instant_page.cc, due to rebase
changes to instant_controller (where it was previously). PTAL.
Thanks!

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.cc:34:
"NewTabPage.MostVisitedTilePlacementExperiment";
I missed implementing it, thanks for catching.
On 2013/07/22 21:14:10, Alexei Svitkine wrote:
> Is there a reason you're defining this here and not using it in
> MostVisitedTilesExperiment::LogInHistogram()?

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment.h (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment.h:49: static bool
IsDontShowOpenURLsEnabled();
This name is as per sky@'s suggestion, and is specific to the feature that
doesn't show URLS on the NTP if they're open in the browser, not the experiment.
On 2013/07/22 21:14:10, Alexei Svitkine wrote:
> Is there a better name you can come up with for this? This is pretty hard to
> read.
> 
> For example, IsMostVisitedTilesExperimentEnabled()?
> 
> Also, this can be made private if MaybeRemoveMostVisitedItems() is moved to
this
> file.

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:706: if
(!history::MostVisitedTilesExperiment::IsDontShowOpenURLsEnabled())
That's how I had originally tried to do this, but it turned out it's not
possible because I can't include the UI component tab_strip_model in history/*,
where the experiment logic is, which is there due to the different places the
code is for 1993 and pre-1993 clients.
On 2013/07/22 21:14:10, Alexei Svitkine wrote:
> This function seems to have identical logic between this impl and the one in
> MostVisitedHandler::MaybeRemovePageValues().
> 
> Can you refactor them to share the implementation? Perhaps putting the new
> function on MostVisitedTilesExperiment.
> 
> You could pass in tab_strip_model and top_sites, which is the difference
between
> the two functions in how they're obtained.
> 
> Then, you also don't need to duplicate GetOpenUrls().

https://chromiumcodereview.appspot.com/17114002/diff/85001/tools/metrics/hist...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/tools/metrics/hist...
tools/metrics/histograms/histograms.xml:8871: could not operate as expected, and
the reason for it.
On 2013/07/22 20:59:06, Alexei Svitkine wrote:
> Please mention when this histogram gets recorded. Is it consistent between the
> cases or do different cases trigger at different times?

Done.

[Alexei Svitkine (slow), 2013-07-23 20:10:51]

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:706: if
(!history::MostVisitedTilesExperiment::IsDontShowOpenURLsEnabled())
On 2013/07/23 19:52:21, annark1 wrote:
> That's how I had originally tried to do this, but it turned out it's not
> possible because I can't include the UI component tab_strip_model in
history/*,
> where the experiment logic is, which is there due to the different places the
> code is for 1993 and pre-1993 clients.

Then it sounds like the helper function should be in a file somewhere under ui/.

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment_unittest.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:46: static void
MakeTestURLString(const int& num, std::string* url) {
No need for any of these functions to be static since they're in the anon
namespace already, which has the same effect.

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:103: virtual
void SetUp() {
OVERRIDE

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:123: if
(histogram) {
Shouldn't it fail if the histogram doesn't exist?

Ditto above.

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:290: // devices due to
different NTP presentation.
Too verbose. How about:

// The code below uses APIs not available on Android and the experiment should
// not run there.

https://chromiumcodereview.appspot.com/17114002/diff/94001/tools/metrics/hist...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/17114002/diff/94001/tools/metrics/hist...
tools/metrics/histograms/histograms.xml:8888: recorded by
most_visited_tiles_experiment.cc, when the experiment action is
Don't need to mention the source file or be so verbose here in general.

How about:

"Records anomalous events for the Most Visited Tile Placement experiment, where
it is not operating as expected. These are recorded during New Tab Page load
time, once for every NTP."

(assuming that matches the semantics)

[annark1, 2013-07-24 01:23:57]

I've moved the two GetOpenURLs functions into a single place, in
tab_strip_model_utils.cc, plus addressed comments. PTAL.

Thanks.

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/85001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:706: if
(!history::MostVisitedTilesExperiment::IsDontShowOpenURLsEnabled())
I was able to consolidate the two GetOpenURLs functions into
tab_strip_model_utils.cc. I've discussed with Philippe for the rest of the two
functions that are so similar, and given the fact that they don't share an
interface, we don't see how I can combine them any further.

On 2013/07/23 20:10:51, Alexei Svitkine wrote:
> On 2013/07/23 19:52:21, annark1 wrote:
> > That's how I had originally tried to do this, but it turned out it's not
> > possible because I can't include the UI component tab_strip_model in
> history/*,
> > where the experiment logic is, which is there due to the different places
the
> > code is for 1993 and pre-1993 clients.
> 
> Then it sounds like the helper function should be in a file somewhere under
ui/.

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/his...
File chrome/browser/history/most_visited_tiles_experiment_unittest.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:46: static void
MakeTestURLString(const int& num, std::string* url) {
On 2013/07/23 20:10:51, Alexei Svitkine wrote:
> No need for any of these functions to be static since they're in the anon
> namespace already, which has the same effect.

Done.

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:103: virtual
void SetUp() {
On 2013/07/23 20:10:51, Alexei Svitkine wrote:
> OVERRIDE

Done.

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/his...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:123: if
(histogram) {
I've done this here, but not in SetUp, because for the first SetUp call in the
suite of tests, it doesn't get a histogram pointer (and so the test fails,
causing all subsequent tests to also fail).

On 2013/07/23 20:10:51, Alexei Svitkine wrote:
> Shouldn't it fail if the histogram doesn't exist?
> 
> Ditto above.

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

https://chromiumcodereview.appspot.com/17114002/diff/94001/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:290: // devices due to
different NTP presentation.
On 2013/07/23 20:10:51, Alexei Svitkine wrote:
> Too verbose. How about:
> 
> // The code below uses APIs not available on Android and the experiment should
> // not run there.

Done.

https://chromiumcodereview.appspot.com/17114002/diff/94001/tools/metrics/hist...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/17114002/diff/94001/tools/metrics/hist...
tools/metrics/histograms/histograms.xml:8888: recorded by
most_visited_tiles_experiment.cc, when the experiment action is
On 2013/07/23 20:10:51, Alexei Svitkine wrote:
> Don't need to mention the source file or be so verbose here in general.
> 
> How about:
> 
> "Records anomalous events for the Most Visited Tile Placement experiment,
where
> it is not operating as expected. These are recorded during New Tab Page load
> time, once for every NTP."
> 
> (assuming that matches the semantics)

Done.

[Alexei Svitkine (slow), 2013-07-24 18:37:36]

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment_unittest.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:109: if
(histogram) {
If this is NULL for the first test run, does it mean your test will fail if ran
in a different order from each other? Or is each test OK with this not running,
in which case do you need this block of code at all?

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:302: }
No need for {}'s.

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:307: history::TopSites* top_sites =
profile_->GetTopSites();
I still think this whole thing can be a shared function somewhere under ui/,
maybe tab strip utils.

All it actually needs to take as a parameter is a TabStripModel*, because from
it you can get a Profile* from which you can get a TopSites*.

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/tabs/t...
File chrome/browser/ui/tabs/tab_strip_model_utils.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model_utils.cc:23: // Creates a set containing
the canonical URLs of the currently open tabs.
No need for a comment here, given that you already have one in the header file.

[Alexei Svitkine (slow), 2013-07-24 19:17:11]

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment.cc:74: if
(open_urls.count(url) != 0) {
I think you can split out this block into a helper functions that takes
open_urls and url and returns whether the item should be deleted. Then, it can
be re-used between the two functions.

Then the body can look like this:

if (ShouldRemoveUrl(open_urls, url, items->size()))
  items->erase(items->begin() + i);
else
  ++i;

(Where ShouldRemoveUrl() can handle logging the histograms.)

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:307: history::TopSites* top_sites =
profile_->GetTopSites();
On 2013/07/24 18:37:36, Alexei Svitkine wrote:
> I still think this whole thing can be a shared function somewhere under ui/,
> maybe tab strip utils.
> 
> All it actually needs to take as a parameter is a TabStripModel*, because from
> it you can get a Profile* from which you can get a TopSites*.

Nevermind, I see it's calling subtly different methods below on the
MostVisitedTilesExperiment.

[annark1, 2013-07-24 20:43:30]

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment.cc:74: if
(open_urls.count(url) != 0) {
Nice! Thanks for this improvement.

On 2013/07/24 19:17:12, Alexei Svitkine wrote:
> I think you can split out this block into a helper functions that takes
> open_urls and url and returns whether the item should be deleted. Then, it can
> be re-used between the two functions.
> 
> Then the body can look like this:
> 
> if (ShouldRemoveUrl(open_urls, url, items->size()))
>   items->erase(items->begin() + i);
> else
>   ++i;
> 
> (Where ShouldRemoveUrl() can handle logging the histograms.)

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment_unittest.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:109: if
(histogram) {
This is necessary and will not fail if the tests are run in a different order.
When the first test is run (whichever test it is), histogram will always be NULL
because base::StatisticsRecorder::FindHistogram() does not find the histogram,
because nothing has been recorded to it yet. Each subsequent call to
FindHistogram does succeed though (because the the tests have forced calls that
log to that histogram), and this code is necessary then to establish the
baseline counts from SnapshotSamples().

On 2013/07/24 18:37:36, Alexei Svitkine wrote:
> If this is NULL for the first test run, does it mean your test will fail if
ran
> in a different order from each other? Or is each test OK with this not
running,
> in which case do you need this block of code at all?

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:302: }
On 2013/07/24 18:37:36, Alexei Svitkine wrote:
> No need for {}'s.

Done.

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:307: history::TopSites* top_sites =
profile_->GetTopSites();
ok.
On 2013/07/24 19:17:12, Alexei Svitkine wrote:
> On 2013/07/24 18:37:36, Alexei Svitkine wrote:
> > I still think this whole thing can be a shared function somewhere under ui/,
> > maybe tab strip utils.
> > 
> > All it actually needs to take as a parameter is a TabStripModel*, because
from
> > it you can get a Profile* from which you can get a TopSites*.
> 
> Nevermind, I see it's calling subtly different methods below on the
> MostVisitedTilesExperiment.

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/tabs/t...
File chrome/browser/ui/tabs/tab_strip_model_utils.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/ui/tabs/t...
chrome/browser/ui/tabs/tab_strip_model_utils.cc:23: // Creates a set containing
the canonical URLs of the currently open tabs.
On 2013/07/24 18:37:36, Alexei Svitkine wrote:
> No need for a comment here, given that you already have one in the header
file.

Done.

[Alexei Svitkine (slow), 2013-07-24 21:04:30]

lgtm with a final set of comments, please be sure to get sky's review as an
OWNER for the files you're changing

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment_unittest.cc (right):

https://codereview.chromium.org/17114002/diff/101001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:109: if
(histogram) {
On 2013/07/24 20:43:31, annark1 wrote:
> This is necessary and will not fail if the tests are run in a different order.
> When the first test is run (whichever test it is), histogram will always be
NULL
> because base::StatisticsRecorder::FindHistogram() does not find the histogram,
> because nothing has been recorded to it yet. Each subsequent call to
> FindHistogram does succeed though (because the the tests have forced calls
that
> log to that histogram), and this code is necessary then to establish the
> baseline counts from SnapshotSamples().

Makes sense, please summarize this in a comment above the if.

> 
> On 2013/07/24 18:37:36, Alexei Svitkine wrote:
> > If this is NULL for the first test run, does it mean your test will fail if
> ran
> > in a different order from each other? Or is each test OK with this not
> running,
> > in which case do you need this block of code at all?
>

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment.cc (right):

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment.cc:117: } else {
Nit: No need for the else block if you're returning above. Remove it and
un-indent the code below.

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment.h (right):

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment.h:12: class ListValue;
This comes from base/values.h, either remove the #include above or this line.

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment.h:13: class
InstantMostVisitedItem;
You're already including instant_types.h, so no need to forward declare this.

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment_unittest.cc (right):

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:46: void
MakeTestURLString(const int& num, std::string* url) {
No need to pass num by ref.

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:51:
url->append(".com");
Just use SStringPrintf.

[annark1, 2013-07-24 21:51:39]

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment.cc (right):

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment.cc:117: } else {
On 2013/07/24 21:04:31, Alexei Svitkine wrote:
> Nit: No need for the else block if you're returning above. Remove it and
> un-indent the code below.

Done.

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment.h (right):

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment.h:12: class ListValue;
On 2013/07/24 21:04:31, Alexei Svitkine wrote:
> This comes from base/values.h, either remove the #include above or this line.

Done.

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment.h:13: class
InstantMostVisitedItem;
On 2013/07/24 21:04:31, Alexei Svitkine wrote:
> You're already including instant_types.h, so no need to forward declare this.

Done.

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment_unittest.cc (right):

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:46: void
MakeTestURLString(const int& num, std::string* url) {
On 2013/07/24 21:04:31, Alexei Svitkine wrote:
> No need to pass num by ref.

Done.

https://codereview.chromium.org/17114002/diff/110001/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:51:
url->append(".com");
On 2013/07/24 21:04:31, Alexei Svitkine wrote:
> Just use SStringPrintf.

Done.

[sky, 2013-07-25 17:20:21]

LGTM with following changes

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment_unittest.cc (right):

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:14: #include
"chrome/browser/history/most_visited_tiles_experiment.h"
nit: this should be your first include (same ordering as
most_visited_Tiles_experiment.h)

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:47:
base::ListValue* list) {
nit: spacing

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:139:
base::HistogramBase* histogram_;
document ownership.

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:140:
scoped_ptr<base::FieldTrialList> field_trial_list_;
nit: AFAICT you don't need a scoped_ptr here. Declare file_trial_list_ by value.

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:158:
scoped_ptr<base::ListValue> pages_value(new ListValue());
nit: don't use scoped_ptr (same comment on 184).

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:170:
ValidateMetrics(NTP_TILE_EXPERIMENT_ACTION_DID_NOT_REMOVE_URL);
I would wrap all calls to this in EXPECT_NO_FATAL_FAILURE, that way if
ValidateMetrics fails you don't continue on. It doesn't matter now as nothing is
after this but that could change.

[annark1, 2013-07-25 18:51:22]

Thanks all for your reviews.

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
File chrome/browser/history/most_visited_tiles_experiment_unittest.cc (right):

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:14: #include
"chrome/browser/history/most_visited_tiles_experiment.h"
On 2013/07/25 17:20:21, sky wrote:
> nit: this should be your first include (same ordering as
> most_visited_Tiles_experiment.h)

Done.

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:47:
base::ListValue* list) {
On 2013/07/25 17:20:21, sky wrote:
> nit: spacing

Done.

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:139:
base::HistogramBase* histogram_;
On 2013/07/25 17:20:21, sky wrote:
> document ownership.

Done.

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:140:
scoped_ptr<base::FieldTrialList> field_trial_list_;
On 2013/07/25 17:20:21, sky wrote:
> nit: AFAICT you don't need a scoped_ptr here. Declare file_trial_list_ by
value.

Done.

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:158:
scoped_ptr<base::ListValue> pages_value(new ListValue());
On 2013/07/25 17:20:21, sky wrote:
> nit: don't use scoped_ptr (same comment on 184).

Done.

https://codereview.chromium.org/17114002/diff/115002/chrome/browser/history/m...
chrome/browser/history/most_visited_tiles_experiment_unittest.cc:170:
ValidateMetrics(NTP_TILE_EXPERIMENT_ACTION_DID_NOT_REMOVE_URL);
On 2013/07/25 17:20:21, sky wrote:
> I would wrap all calls to this in EXPECT_NO_FATAL_FAILURE, that way if
> ValidateMetrics fails you don't continue on. It doesn't matter now as nothing
is
> after this but that could change.

Done.

[commit-bot: I haz the power, 2013-07-25 19:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annark@chromium.org/17114002/127001

[commit-bot: I haz the power, 2013-07-25 22:32:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annark@chromium.org/17114002/127001

[commit-bot: I haz the power, 2013-07-25 22:58:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annark@chromium.org/17114002/127001

[commit-bot: I haz the power, 2013-07-25 23:02:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annark@chromium.org/17114002/127001

[Alexei Svitkine (slow), 2013-07-26 02:13:50]

On 2013/07/25 23:02:58, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/annark%40chromium.org/17114002/127001

Looks like you have test failures in the tests you're adding.

[commit-bot: I haz the power, 2013-07-26 03:41:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annark@chromium.org/17114002/127001

[commit-bot: I haz the power, 2013-07-26 04:34:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[annark1, 2013-07-26 13:38:08]

On 2013/07/26 04:34:34, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on mac.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

I fixed two issues: missing virtual keyword on
most_visited_tiles_experiment_unit_test's destructor; and removed two
unnecessary (and failing) calls to validate the histogram metrics (these calls
would fail when these two experiments were run on their own, as there would be
no histogram created in the tested code, therefore the histogram pointer would
be NULL, causing the failure).

[commit-bot: I haz the power, 2013-07-26 13:41:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annark@chromium.org/17114002/176001

[commit-bot: I haz the power, 2013-07-26 17:45:14]

Change committed as 213947