Most visited iframe now postMessage to signal the iframing page that the link has been displayed

- Most visited iframe now uses postMessage() to signal the iframing page that the link has been displayed.
- Refactor most_visited_thumbnail.js to eliminate client-side secondary thumbnail fallback.
- Refactor UMA logging of NTP suggestions events.

The new postMessage() signal is sent whenever the iframe has failed loading the thumbnails, or when the thumbnail has been successfully loaded. This way it's impossible for the iframing page to learn whether the user has a given URL in its TopSites. The iframing page can use that signal to know when it's possible to display a fallback visual under the iframe.

The refactor significantly simplifies the iframe javascript by dropping support for multiple thumbnails, which can now be handled by the iframing page. UMA logging of NTP suggestions related statistics have also been both simplified and augmented. The patch also includes better unit tests for the UMA logging.

BUG=None

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

[beaudoin, 2013-12-19 16:35:45]

Hi Mathieu,

Can you take a look at this patch? One thing I'm considering is completely
killing the fallback mechanism. If we ever want to fallback to server-side
generated thumbnails we can now do it following the postmessage. The only reason
to keep the fallback mechanism would be to support multiple client-side
thumbnails. (eg. your idea of sending multiple chrome-search://thumb2/... URLs)

[Mathieu, 2013-12-19 20:30:26]

On 2013/12/19 16:35:45, beaudoin wrote:
> Hi Mathieu,
> 
> Can you take a look at this patch? One thing I'm considering is completely
> killing the fallback mechanism. If we ever want to fallback to server-side
> generated thumbnails we can now do it following the postmessage. The only
reason
> to keep the fallback mechanism would be to support multiple client-side
> thumbnails. (eg. your idea of sending multiple chrome-search://thumb2/...
URLs)

It's always been a bit clunky to pass in a fallback URL to the iframe, so if we
can do it from the page instead of the iframe I would say go for it. It will
also allow us to more rapidly iterate on the design/fixes of the fallbacks.

[beaudoin, 2014-01-08 21:27:36]

mathp: Overall logic
jered: c/b/r/local_ntp/
estade: c/b/ui/webui/ntp/
sky: c/common/
asvitkine: tools/metrics/histograms.xml

[Evan Stade, 2014-01-08 21:59:11]

is it necessary for the 3 bullet points from the CL description to all be done
in one CL? If not, could you break into independent CLs?

[beaudoin, 2014-01-08 22:02:20]

On 2014/01/08 21:59:11, Evan Stade wrote:
> is it necessary for the 3 bullet points from the CL description to all be done
> in one CL? If not, could you break into independent CLs?

I thought about it, but the second bullet point is a direct consequence of the
first (we handle the fallback externally now). The third bullet point is because
the UMA stats related to fallback no longer make sense, so I just took that
opportunity to clean everything up instead of just removing the obsoleted UMA
metrics.

I personally don't think it's worth splitting up.

[sky, 2014-01-09 00:25:33]

LGTM

https://codereview.chromium.org/102433009/diff/40001/chrome/common/ntp_loggin...
File chrome/common/ntp_logging_events.h (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/common/ntp_loggin...
chrome/common/ntp_logging_events.h:20: // DEPRECATED:
NTP_FALLBACK_THUMBNAIL_REQUESTED = 3,
nit: Since you're commenting out the constants they aren't available anymore. I
thought 'removed' or 'historical' would be better here.

[Evan Stade, 2014-01-09 00:45:41]

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:44:
FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest, TestLogging);
if this is just for the constructor, I'd prefer you create a testing subclass
with a public constructor. FRIEND_TEST is annoying and leaks test code into
prod.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:80: size_t
number_of_gray_tiles_;
can you make the order of these member variables match the order in the enum

[Mathieu, 2014-01-09 18:37:18]

nits!

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:38: 
nit: remove space here to be consistent

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:39: // Creates and
add an image.
nit: adds

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_util.js:25: // DEPRECATED:
NTP_FALLBACK_THUMBNAIL_REQUESTED: 3,
let's remove them?

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:61: // Only log the
following statistics if there is we recorded at least one
*if at least one tile was recorded

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:63: // the user changes tab
away from the NTP. However, if he comes back to that
he -> the user

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:86: number_of_gray_tiles_,
0, 8, 9 );
you tested this does the right thing at boundary cases by checking
chrome://histograms?

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:14:
base::HistogramBase::Count getTotalCount(const std::string& histogramName) {
no indent

[Alexei Svitkine (slow), 2014-01-09 18:54:40]

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:68: number_of_tiles_, 0, 8,
9 );
Perhaps its worth defining your own macro for this to avoid repeating the later
params?

e.g.

#define UMA_HISTOGRAM_NTP_TILES(name, value) \
    UMA_HISTOGRAM_CUSTOM_COUNTS(name, value, 0, 8, 9)

and changing these macros calls to:

UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfTiles", number_of_tiles_);

Also, the values 0, 8, 9 should be explained somewhere in a comment.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:14:
base::HistogramBase::Count getTotalCount(const std::string& histogramName) {
Capitalize function name. histogramName -> histogram_name.

Here and below.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:16: histogramName);
This should be indented 4 more than previous line. Here and below.

[Jered, 2014-01-10 15:12:35]

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
window.parent.postMessage('linkDisplayed', '*');
If this is always going to happen, can you just use the iframe onload event
instead?

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
window.parent.postMessage('linkDisplayed', '*');
'*' makes me nervous. While I think this message is harmless, it would be better
to restrict to just the new tab page origin.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:64: // NTP tab later the
statistics are not regenerated (ie. they are all 0). If
nit: NTP tab -> NTP, ie. -> i.e.

[beaudoin, 2014-01-15 23:39:56]

Answered comments,
Mathieu, Jered, Evan, Alexei: PTAL

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
window.parent.postMessage('linkDisplayed', '*');
On 2014/01/10 15:12:36, Jered wrote:
> If this is always going to happen, can you just use the iframe onload event
> instead?

This event is fired after the onload, specifically when the dynamically created
<img> tiles have loaded. The timing is important, otherwise we could display the
fallback image (which sits behind the regular tile) before the tile has
completed loading, which results in flashing.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
window.parent.postMessage('linkDisplayed', '*');
On 2014/01/10 15:12:36, Jered wrote:
> '*' makes me nervous. While I think this message is harmless, it would be
better
> to restrict to just the new tab page origin.

The problem here is that I cannot know the URL of the iframing page.
document.referrer wont work because they are on different domains. Looks like
chrome://newtab doesn't work, I probably need the real URL from
window.location.orign, which I can't bake in the JS because it's not a constant
(it can use different TLD, it's different when debugging, etc.)

Think I can go with '*' here?

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:38: 
On 2014/01/09 18:37:18, Mathieu Perreault wrote:
> nit: remove space here to be consistent

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:39: // Creates and
add an image.
On 2014/01/09 18:37:18, Mathieu Perreault wrote:
> nit: adds

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_util.js:25: // DEPRECATED:
NTP_FALLBACK_THUMBNAIL_REQUESTED: 3,
On 2014/01/09 18:37:18, Mathieu Perreault wrote:
> let's remove them?

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:61: // Only log the
following statistics if there is we recorded at least one
On 2014/01/09 18:37:18, Mathieu Perreault wrote:
> *if at least one tile was recorded

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:63: // the user changes tab
away from the NTP. However, if he comes back to that
On 2014/01/09 18:37:18, Mathieu Perreault wrote:
> he -> the user

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:64: // NTP tab later the
statistics are not regenerated (ie. they are all 0). If
On 2014/01/10 15:12:36, Jered wrote:
> nit: NTP tab -> NTP, ie. -> i.e.

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:68: number_of_tiles_, 0, 8,
9 );
On 2014/01/09 18:54:41, Alexei Svitkine wrote:
> Perhaps its worth defining your own macro for this to avoid repeating the
later
> params?
> 
> e.g.
> 
> #define UMA_HISTOGRAM_NTP_TILES(name, value) \
>     UMA_HISTOGRAM_CUSTOM_COUNTS(name, value, 0, 8, 9)
> 
> and changing these macros calls to:
> 
> UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfTiles", number_of_tiles_);
> 
> Also, the values 0, 8, 9 should be explained somewhere in a comment.

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:86: number_of_gray_tiles_,
0, 8, 9 );
On 2014/01/09 18:37:18, Mathieu Perreault wrote:
> you tested this does the right thing at boundary cases by checking
> chrome://histograms?

Yes. (It didn't, that's why I has to use CUSTOM_COUNTS :)) The unit test
exercises the boundary cases, but I'm not sure it's indicative of what show s up
in chrome://histograms. In all cases, I've tested it manually.

Ack.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:44:
FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest, TestLogging);
On 2014/01/09 00:45:42, Evan Stade wrote:
> if this is just for the constructor, I'd prefer you create a testing subclass
> with a public constructor. FRIEND_TEST is annoying and leaks test code into
> prod.

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:80: size_t
number_of_gray_tiles_;
On 2014/01/09 00:45:42, Evan Stade wrote:
> can you make the order of these member variables match the order in the enum

I cleaned the enums up, used more uniform names and reordered everything in a
consistant manner. My OCD-self is happy. :)

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:14:
base::HistogramBase::Count getTotalCount(const std::string& histogramName) {
On 2014/01/09 18:37:18, Mathieu Perreault wrote:
> no indent

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:14:
base::HistogramBase::Count getTotalCount(const std::string& histogramName) {
On 2014/01/09 18:54:41, Alexei Svitkine wrote:
> Capitalize function name. histogramName -> histogram_name.
> 
> Here and below.

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:16: histogramName);
On 2014/01/09 18:54:41, Alexei Svitkine wrote:
> This should be indented 4 more than previous line. Here and below.

Done.

https://codereview.chromium.org/102433009/diff/40001/chrome/common/ntp_loggin...
File chrome/common/ntp_logging_events.h (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/common/ntp_loggin...
chrome/common/ntp_logging_events.h:20: // DEPRECATED:
NTP_FALLBACK_THUMBNAIL_REQUESTED = 3,
On 2014/01/09 00:25:34, sky wrote:
> nit: Since you're commenting out the constants they aren't available anymore.
I
> thought 'removed' or 'historical' would be better here.

Just cleaned up this whole enum. Since it's only internal it shouldn't matter.

[Evan Stade, 2014-01-16 00:51:40]

lgtm

https://codereview.chromium.org/102433009/diff/210001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right):

https://codereview.chromium.org/102433009/diff/210001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:50: for (int i=0;
i<20; ++i)
spaces around binary operators.

[beaudoin, 2014-01-16 15:57:04]

https://codereview.chromium.org/102433009/diff/210001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right):

https://codereview.chromium.org/102433009/diff/210001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:50: for (int i=0;
i<20; ++i)
On 2014/01/16 00:51:41, Evan Stade wrote:
> spaces around binary operators.

Done.

[Mathieu, 2014-01-16 16:07:00]

lgtm

https://codereview.chromium.org/102433009/diff/260001/chrome/browser/resource...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/102433009/diff/260001/chrome/browser/resource...
chrome/browser/resources/local_ntp/most_visited_util.js:183: data.thumbnailUrl2
= params.tu2 || '';
we should remove this, and below

[Jered, 2014-01-16 18:07:49]

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
window.parent.postMessage('linkDisplayed', '*');
On 2014/01/15 23:39:56, beaudoin wrote:
> On 2014/01/10 15:12:36, Jered wrote:
> > If this is always going to happen, can you just use the iframe onload event
> > instead?
> 
> This event is fired after the onload, specifically when the dynamically
created
> <img> tiles have loaded. The timing is important, otherwise we could display
the
> fallback image (which sits behind the regular tile) before the tile has
> completed loading, which results in flashing.

That makes sense. We should probably also be using this message to wait to show
most visited tiles so that we don't flash empty tiles before showing the images.

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
window.parent.postMessage('linkDisplayed', '*');
On 2014/01/15 23:39:56, beaudoin wrote:
> On 2014/01/10 15:12:36, Jered wrote:
> > '*' makes me nervous. While I think this message is harmless, it would be
> better
> > to restrict to just the new tab page origin.
> 
> The problem here is that I cannot know the URL of the iframing page.
> document.referrer wont work because they are on different domains. Looks like
> chrome://newtab doesn't work, I probably need the real URL from
> window.location.orign, which I can't bake in the JS because it's not a
constant
> (it can use different TLD, it's different when debugging, etc.)
> 
> Think I can go with '*' here?

I solved this problem once before for suggestion iframes but put up a mental
block about that code. If you send this JS via SendJSWithOrigin(), and include a
string like "{{ORIGIN}}", it should be the origin you want. :-)

[Alexei Svitkine (slow), 2014-01-16 19:50:29]

lgtm

https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:19: #define
UMA_HISTOGRAM_NTP_TILES(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
Nit: I'd wrap it like this:

#define UMA_HISTOGRAM_NTP_TILES(name, sample) \
    UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 0, 8, 9)

https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right):

https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:50: bool
server_side_suggestions_;
Optional nit: I think has_server_side_suggestions_ would be a slightly better
name. Up to you.

https://codereview.chromium.org/102433009/diff/320001/chrome/common/ntp_loggi...
File chrome/common/ntp_logging_events.h (right):

https://codereview.chromium.org/102433009/diff/320001/chrome/common/ntp_loggi...
chrome/common/ntp_logging_events.h:8: // The different types of events that are
logged from the NTP.
Nit: Might be worth expanding the comment to mention how these are used. (E.g.
clarifying that this isn't an UMA enum histogram's logged value (so its okay to
renumber them, etc)).

[beaudoin, 2014-01-17 03:51:45]

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
window.parent.postMessage('linkDisplayed', '*');
On 2014/01/16 18:07:55, Jered wrote:
> On 2014/01/15 23:39:56, beaudoin wrote:
> > On 2014/01/10 15:12:36, Jered wrote:
> > > If this is always going to happen, can you just use the iframe onload
event
> > > instead?
> > 
> > This event is fired after the onload, specifically when the dynamically
> created
> > <img> tiles have loaded. The timing is important, otherwise we could display
> the
> > fallback image (which sits behind the regular tile) before the tile has
> > completed loading, which results in flashing.
> 
> That makes sense. We should probably also be using this message to wait to
show
> most visited tiles so that we don't flash empty tiles before showing the
images.

Good point, I'll add that as a TODO on my side. :)

https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
window.parent.postMessage('linkDisplayed', '*');
On 2014/01/16 18:07:55, Jered wrote:
> On 2014/01/15 23:39:56, beaudoin wrote:
> > On 2014/01/10 15:12:36, Jered wrote:
> > > '*' makes me nervous. While I think this message is harmless, it would be
> > better
> > > to restrict to just the new tab page origin.
> > 
> > The problem here is that I cannot know the URL of the iframing page.
> > document.referrer wont work because they are on different domains. Looks
like
> > chrome://newtab doesn't work, I probably need the real URL from
> > window.location.orign, which I can't bake in the JS because it's not a
> constant
> > (it can use different TLD, it's different when debugging, etc.)
> > 
> > Think I can go with '*' here?
> 
> I solved this problem once before for suggestion iframes but put up a mental
> block about that code. If you send this JS via SendJSWithOrigin(), and include
a
> string like "{{ORIGIN}}", it should be the origin you want. :-)

This was significantly harder than expected, you'll see my various changes: one
to account for the fact that we now have the render_frame_id not the
render_view_id, one to make sure we get the real URL not the "chrome://newtab"
one. Nobody else seemed to be using SendJSWithOrigin() save from the test so I
believe the changes are fine.

Please take a thorough look at these changes, in /c/b/search/*

https://codereview.chromium.org/102433009/diff/260001/chrome/browser/resource...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/102433009/diff/260001/chrome/browser/resource...
chrome/browser/resources/local_ntp/most_visited_util.js:183: data.thumbnailUrl2
= params.tu2 || '';
On 2014/01/16 16:07:00, Mathieu Perreault wrote:
> we should remove this, and below

Done.

https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:19: #define
UMA_HISTOGRAM_NTP_TILES(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
On 2014/01/16 19:50:30, Alexei Svitkine wrote:
> Nit: I'd wrap it like this:
> 
> #define UMA_HISTOGRAM_NTP_TILES(name, sample) \
>     UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 0, 8, 9)

I wrapped it exactly like in histogram.h, but I happen to agree your version
looks nicer. :)

Done.

https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right):

https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:50: bool
server_side_suggestions_;
On 2014/01/16 19:50:30, Alexei Svitkine wrote:
> Optional nit: I think has_server_side_suggestions_ would be a slightly better
> name. Up to you.

Done.

https://codereview.chromium.org/102433009/diff/320001/chrome/common/ntp_loggi...
File chrome/common/ntp_logging_events.h (right):

https://codereview.chromium.org/102433009/diff/320001/chrome/common/ntp_loggi...
chrome/common/ntp_logging_events.h:8: // The different types of events that are
logged from the NTP.
On 2014/01/16 19:50:30, Alexei Svitkine wrote:
> Nit: Might be worth expanding the comment to mention how these are used. (E.g.
> clarifying that this isn't an UMA enum histogram's logged value (so its okay
to
> renumber them, etc)).

Done.

[Jered, 2014-01-17 16:34:55]

On 2014/01/17 03:51:45, beaudoin wrote:
>
https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
> File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):
> 
>
https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
> chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
> window.parent.postMessage('linkDisplayed', '*');
> On 2014/01/16 18:07:55, Jered wrote:
> > On 2014/01/15 23:39:56, beaudoin wrote:
> > > On 2014/01/10 15:12:36, Jered wrote:
> > > > If this is always going to happen, can you just use the iframe onload
> event
> > > > instead?
> > > 
> > > This event is fired after the onload, specifically when the dynamically
> > created
> > > <img> tiles have loaded. The timing is important, otherwise we could
display
> > the
> > > fallback image (which sits behind the regular tile) before the tile has
> > > completed loading, which results in flashing.
> > 
> > That makes sense. We should probably also be using this message to wait to
> show
> > most visited tiles so that we don't flash empty tiles before showing the
> images.
> 
> Good point, I'll add that as a TODO on my side. :)
> 
>
https://codereview.chromium.org/102433009/diff/40001/chrome/browser/resources...
> chrome/browser/resources/local_ntp/most_visited_thumbnail.js:22:
> window.parent.postMessage('linkDisplayed', '*');
> On 2014/01/16 18:07:55, Jered wrote:
> > On 2014/01/15 23:39:56, beaudoin wrote:
> > > On 2014/01/10 15:12:36, Jered wrote:
> > > > '*' makes me nervous. While I think this message is harmless, it would
be
> > > better
> > > > to restrict to just the new tab page origin.
> > > 
> > > The problem here is that I cannot know the URL of the iframing page.
> > > document.referrer wont work because they are on different domains. Looks
> like
> > > chrome://newtab doesn't work, I probably need the real URL from
> > > window.location.orign, which I can't bake in the JS because it's not a
> > constant
> > > (it can use different TLD, it's different when debugging, etc.)
> > > 
> > > Think I can go with '*' here?
> > 
> > I solved this problem once before for suggestion iframes but put up a mental
> > block about that code. If you send this JS via SendJSWithOrigin(), and
include
> a
> > string like "{{ORIGIN}}", it should be the origin you want. :-)
> 
> This was significantly harder than expected, you'll see my various changes:
one
> to account for the fact that we now have the render_frame_id not the
> render_view_id, one to make sure we get the real URL not the "chrome://newtab"
> one. Nobody else seemed to be using SendJSWithOrigin() save from the test so I
> believe the changes are fine.
> 
> Please take a thorough look at these changes, in /c/b/search/*
> 
>
https://codereview.chromium.org/102433009/diff/260001/chrome/browser/resource...
> File chrome/browser/resources/local_ntp/most_visited_util.js (right):
> 
>
https://codereview.chromium.org/102433009/diff/260001/chrome/browser/resource...
> chrome/browser/resources/local_ntp/most_visited_util.js:183:
data.thumbnailUrl2
> = params.tu2 || '';
> On 2014/01/16 16:07:00, Mathieu Perreault wrote:
> > we should remove this, and below
> 
> Done.
> 
>
https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):
> 
>
https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:19: #define
> UMA_HISTOGRAM_NTP_TILES(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
> On 2014/01/16 19:50:30, Alexei Svitkine wrote:
> > Nit: I'd wrap it like this:
> > 
> > #define UMA_HISTOGRAM_NTP_TILES(name, sample) \
> >     UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 0, 8, 9)
> 
> I wrapped it exactly like in histogram.h, but I happen to agree your version
> looks nicer. :)
> 
> Done.
> 
>
https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/ntp/ntp_user_data_logger.h (right):
> 
>
https://codereview.chromium.org/102433009/diff/320001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/ntp/ntp_user_data_logger.h:50: bool
> server_side_suggestions_;
> On 2014/01/16 19:50:30, Alexei Svitkine wrote:
> > Optional nit: I think has_server_side_suggestions_ would be a slightly
better
> > name. Up to you.
> 
> Done.
> 
>
https://codereview.chromium.org/102433009/diff/320001/chrome/common/ntp_loggi...
> File chrome/common/ntp_logging_events.h (right):
> 
>
https://codereview.chromium.org/102433009/diff/320001/chrome/common/ntp_loggi...
> chrome/common/ntp_logging_events.h:8: // The different types of events that
are
> logged from the NTP.
> On 2014/01/16 19:50:30, Alexei Svitkine wrote:
> > Nit: Might be worth expanding the comment to mention how these are used.
(E.g.
> > clarifying that this isn't an UMA enum histogram's logged value (so its okay
> to
> > renumber them, etc)).
> 
> Done.

lgtm

Ah yeah that code was back before render frame, and was missed in the
conversion.
Thanks for doing the right thing here.

[commit-bot: I haz the power, 2014-01-17 17:23:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/102433009/460001

[commit-bot: I haz the power, 2014-01-17 19:25:07]

Change committed as 245579