Add chrome://site-engagement WebUI.

Add chrome://site-engagement WebUI.

This CL adds a debug WebUI built on Mojo for the site engagement at
chrome://site-engagment. The WebUI currently only exposes the profile's site
engagement scores but has the intent of becoming a more powerful engagement
debugging and control UI.

This CL also changes the mojo WebUI build setup to use a common root_gen_dir
which resources are specified relative to in .grd files.

BUG=524857
NOPRESUBMIT=true

Committed: https://crrev.com/d59c103a2605ad0edf425d5b19a0e052566d80d2
Cr-Commit-Position: refs/heads/master@{#350116}

[calamity, 2015-08-26 06:55:31]

calamity@chromium.org changed reviewers:
+ sammc@chromium.org

[calamity, 2015-08-26 06:55:37]

+sammc for initial review.

[Sam McNally, 2015-08-26 08:23:22]

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:22: function
addEngagementTableRow(origin, score) {
It might be worth using polymer data binding and templating instead of this
boilerplate.

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:38: function
redrawEngagementTable() {
Why not make this a method of SiteEngagementPageImpl?

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement.mojom (right):

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement.mojom:15:
HandleSiteEngagementInfo(array<SiteEngagementInfo> info);
Is this called more than once?

If not, you can make |info| a return value for GetSiteEngagementInfo:
GetSiteEngagementInfo() => (array<SiteEngagementInfo> info);

The JS binding for that pattern is a function that returns a promise to a
dictionary containing an |info| field (mojo supports multiple return values,
unlike JS promises).

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc (right):

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
No (c)

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h (right):

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
No (c)

[calamity, 2015-09-08 07:30:07]

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:22: function
addEngagementTableRow(origin, score) {
On 2015/08/26 08:23:21, Sam McNally wrote:
> It might be worth using polymer data binding and templating instead of this
> boilerplate.

Done.

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:38: function
redrawEngagementTable() {
On 2015/08/26 08:23:21, Sam McNally wrote:
> Why not make this a method of SiteEngagementPageImpl?

I guess I thought that SiteEngagementPageImpl was for the message-y stuff. Moved
all of the stuff in.

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement.mojom (right):

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement.mojom:15:
HandleSiteEngagementInfo(array<SiteEngagementInfo> info);
On 2015/08/26 08:23:22, Sam McNally wrote:
> Is this called more than once?
> 
> If not, you can make |info| a return value for GetSiteEngagementInfo:
> GetSiteEngagementInfo() => (array<SiteEngagementInfo> info);
> 
> The JS binding for that pattern is a function that returns a promise to a
> dictionary containing an |info| field (mojo supports multiple return values,
> unlike JS promises).

It most likely will be at some point. We would want the page to update when
someone presses a button, for example.

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc (right):

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
On 2015/08/26 08:23:22, Sam McNally wrote:
> No (c)

Done.

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h (right):

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
On 2015/08/26 08:23:22, Sam McNally wrote:
> No (c)

Done.

[Sam McNally, 2015-09-08 08:25:47]

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h (right):

https://codereview.chromium.org/1316043003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
On 2015/09/08 07:30:07, calamity wrote:
> On 2015/08/26 08:23:22, Sam McNally wrote:
> > No (c)
> 
> Done.

Not done yet.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:4: 
'use strict';

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:14: value: function() {
return []; }
Try value: () => []

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:6:
'mojo/public/js/bindings',
Remove.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:7: 'mojo/public/js/core',
Remove.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:11: ], function(bindings,
core, connection, browser, serviceProvider) {
s/browser/siteEngagementMojom/

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:12: 'use strict';
This can go outside.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:15: var browserProxy =
connection.bindHandleToProxy(
How about uiHandler?

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:20: var pipe =
core.createMessagePipe();
Remove.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:23:
browserProxy.getSiteEngagementInfo().then(function(response) {
then(response => {

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement.mojom (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement.mojom:10: interface
SiteEngagementUIHandlerMojo {
The current convention is to call this SiteEngagementUIHandler and name the
implementation SiteEngagementUIHandlerImpl.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc:20:
mojo::Array<SiteEngagementInfoPtr> engagement_info;
You probably need to resize this to 0. Otherwise, it'll be null if there are no
sites and the validation will fail.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc:27:
origin_info->origin = mojo::String::From(info.first.spec());
#include "mojo/common/url_type_converters.h" and convert directly from
info.first.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h:15: //
Implementation of SiteEngagementUIHandlerMojo. StartOmniboxQuery() calls to a
Update.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h:19: class
SiteEngagementUIHandler : public SiteEngagementUIHandlerMojo,
If you don't need this to be public, consider putting it in
site_engagement_ui.cc.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h:20: public
MojoWebUIHandler {
I don't think this is necessary anymore.

[calamity, 2015-09-10 06:16:13]

Patchset #5 (id:80001) has been deleted

[calamity, 2015-09-10 06:57:32]

calamity@chromium.org changed reviewers:
+ dbeam@chromium.org, raymes@chromium.org, sky@chromium.org

[calamity, 2015-09-10 06:57:33]

+dbeam for chrome/browser/resources/, chrome/browser/ui/webui, and
browser_resources.grd

+raymes for site_engagement_service.*

+sky for chrome_resources.gyp, chrome_web_ui_mojo_bindings.gyp,
chrome/browser/BUILD.gn and chrome/BUILD.gn

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:4: 
On 2015/09/08 08:25:47, Sam McNally wrote:
> 'use strict';

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/engagement_table.js:14: value: function() {
return []; }
On 2015/09/08 08:25:47, Sam McNally wrote:
> Try value: () => []

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/engagement/site_engagement.js (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:6:
'mojo/public/js/bindings',
On 2015/09/08 08:25:47, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:7: 'mojo/public/js/core',
On 2015/09/08 08:25:47, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:11: ], function(bindings,
core, connection, browser, serviceProvider) {
On 2015/09/08 08:25:47, Sam McNally wrote:
> s/browser/siteEngagementMojom/

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:12: 'use strict';
On 2015/09/08 08:25:47, Sam McNally wrote:
> This can go outside.

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:15: var browserProxy =
connection.bindHandleToProxy(
On 2015/09/08 08:25:47, Sam McNally wrote:
> How about uiHandler?

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:20: var pipe =
core.createMessagePipe();
On 2015/09/08 08:25:47, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/resource...
chrome/browser/resources/engagement/site_engagement.js:23:
browserProxy.getSiteEngagementInfo().then(function(response) {
On 2015/09/08 08:25:47, Sam McNally wrote:
> then(response => {

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement.mojom (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement.mojom:10: interface
SiteEngagementUIHandlerMojo {
On 2015/09/08 08:25:47, Sam McNally wrote:
> The current convention is to call this SiteEngagementUIHandler and name the
> implementation SiteEngagementUIHandlerImpl.

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc:20:
mojo::Array<SiteEngagementInfoPtr> engagement_info;
On 2015/09/08 08:25:47, Sam McNally wrote:
> You probably need to resize this to 0. Otherwise, it'll be null if there are
no
> sites and the validation will fail.

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.cc:27:
origin_info->origin = mojo::String::From(info.first.spec());
On 2015/09/08 08:25:47, Sam McNally wrote:
> #include "mojo/common/url_type_converters.h" and convert directly from
> info.first.

Done.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h (right):

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h:15: //
Implementation of SiteEngagementUIHandlerMojo. StartOmniboxQuery() calls to a
On 2015/09/08 08:25:47, Sam McNally wrote:
> Update.

Oops.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h:19: class
SiteEngagementUIHandler : public SiteEngagementUIHandlerMojo,
On 2015/09/08 08:25:47, Sam McNally wrote:
> If you don't need this to be public, consider putting it in
> site_engagement_ui.cc.

Hmm. I can't think of any reason it would need to be. If it turns out we need it
to be down the line, we can always change it.

https://codereview.chromium.org/1316043003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/engagement/site_engagement_ui_handler.h:20: public
MojoWebUIHandler {
On 2015/09/08 08:25:47, Sam McNally wrote:
> I don't think this is necessary anymore.

Done.

[Sam McNally, 2015-09-10 08:24:46]

LGTM

https://codereview.chromium.org/1316043003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/engagement/site_engagement_ui.cc (right):

https://codereview.chromium.org/1316043003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:16: 
namespace {

https://codereview.chromium.org/1316043003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:55: 
}  // namespace

[sky, 2015-09-10 17:22:24]

https://codereview.chromium.org/1316043003/diff/160001/chrome/chrome_resource...
File chrome/chrome_resources.gyp (left):

https://codereview.chromium.org/1316043003/diff/160001/chrome/chrome_resource...
chrome/chrome_resources.gyp:205: '-E',
'omnibox_mojom_file=<(omnibox_mojom_file)',
How is the omnibox.mojom.js file picked up after your changes?

[Dan Beam, 2015-09-10 17:33:55]

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/engagement/engagement_table.css (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/engagement/engagement_table.css:11: padding-right: 4px;
indent all these rules

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/engagement/engagement_table.css:15: background:
rgb(224,236,255);
rgb(224, 236, 255);

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/engagement/engagement_table.js:16: value: () => []
though this is fine, be aware that if you ever need this code to run on iOS, it
may not work

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/engagement/site_engagement.html (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/engagement/site_engagement.html:14: <engagement-table
id="engagement-table">
I think custom elements require end tags (i.e. </engagement-table>)

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/engagement/site_engagement_ui.cc (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:27: :
profile_(profile), binding_(this, request.Pass()) {}
maybe { DCHECK(profile_); }

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:37: DCHECK(service);
nit: DCHECK(thing); thing->DoSomething() has never been that helpful, IMO

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:39: for (auto info :
service->GetScoreMap()) {
nit: auto -> actual type for readability, IMO

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/engagement/site_engagement_ui.h (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.h:8: #include
"base/basictypes.h"
nit: base/macros.h instead

[calamity, 2015-09-11 07:45:39]

Patchset #9 (id:170001) has been deleted

[calamity, 2015-09-11 07:48:24]

https://codereview.chromium.org/1316043003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/engagement/site_engagement_ui.cc (right):

https://codereview.chromium.org/1316043003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:16: 
On 2015/09/10 08:24:46, Sam McNally wrote:
> namespace {

Done.

https://codereview.chromium.org/1316043003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:55: 
On 2015/09/10 08:24:46, Sam McNally wrote:
> }  // namespace

Done.

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/engagement/engagement_table.css (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/engagement/engagement_table.css:11: padding-right: 4px;
On 2015/09/10 17:33:54, Dan Beam wrote:
> indent all these rules

Done.

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/engagement/engagement_table.css:15: background:
rgb(224,236,255);
On 2015/09/10 17:33:54, Dan Beam wrote:
> rgb(224, 236, 255);

Done.

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/engagement/engagement_table.js (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/engagement/engagement_table.js:16: value: () => []
On 2015/09/10 17:33:54, Dan Beam wrote:
> though this is fine, be aware that if you ever need this code to run on iOS,
it
> may not work

Acknowledged.

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/engagement/site_engagement.html (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/engagement/site_engagement.html:14: <engagement-table
id="engagement-table">
On 2015/09/10 17:33:54, Dan Beam wrote:
> I think custom elements require end tags (i.e. </engagement-table>)

Done.

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/engagement/site_engagement_ui.cc (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:27: :
profile_(profile), binding_(this, request.Pass()) {}
On 2015/09/10 17:33:54, Dan Beam wrote:
> maybe { DCHECK(profile_); }

Done.

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:37: DCHECK(service);
On 2015/09/10 17:33:55, Dan Beam wrote:
> nit: DCHECK(thing); thing->DoSomething() has never been that helpful, IMO

Done.

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.cc:39: for (auto info :
service->GetScoreMap()) {
On 2015/09/10 17:33:55, Dan Beam wrote:
> nit: auto -> actual type for readability, IMO

Done.

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/engagement/site_engagement_ui.h (right):

https://codereview.chromium.org/1316043003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/engagement/site_engagement_ui.h:8: #include
"base/basictypes.h"
On 2015/09/10 17:33:55, Dan Beam wrote:
> nit: base/macros.h instead

Done.

https://codereview.chromium.org/1316043003/diff/160001/chrome/chrome_resource...
File chrome/chrome_resources.gyp (left):

https://codereview.chromium.org/1316043003/diff/160001/chrome/chrome_resource...
chrome/chrome_resources.gyp:205: '-E',
'omnibox_mojom_file=<(omnibox_mojom_file)',
On 2015/09/10 17:22:24, sky wrote:
> How is the omnibox.mojom.js file picked up after your changes?

The change in browser_resources.grd specifies the path to the from the
root_gen_dir to the mojom. Added this info to the CL description.

[Dan Beam, 2015-09-11 17:41:14]

can haz new patchset?

[calamity, 2015-09-14 01:53:34]

Oops =(

Done.

[raymes, 2015-09-14 03:06:11]

https://codereview.chromium.org/1316043003/diff/190001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1316043003/diff/190001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_service.h:124: std::map<GURL, int>
GetScoreMap();
Score() returns a double, should this be a double?

[Dan Beam, 2015-09-14 18:48:13]

lgtm

[calamity, 2015-09-15 04:38:50]

https://codereview.chromium.org/1316043003/diff/190001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1316043003/diff/190001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_service.h:124: std::map<GURL, int>
GetScoreMap();
On 2015/09/14 03:06:11, raymes wrote:
> Score() returns a double, should this be a double?

GetScore() returns an int. I believe the intention is to keep the external
representation fairly coarse-grained.

[raymes, 2015-09-15 07:23:01]

lgtm

https://codereview.chromium.org/1316043003/diff/190001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1316043003/diff/190001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_service.h:124: std::map<GURL, int>
GetScoreMap();
Ahh right, that makes sense. Sorry I was looking at the wrong function!

[calamity, 2015-09-18 00:44:14]

Friendly ping for sky@.

On 2015/09/10 17:22:24, sky wrote:
> How is the omnibox.mojom.js file picked up after your changes?

The change in browser_resources.grd specifies the path to the from the
root_gen_dir to the mojom. Added this info to the CL description.

[sky, 2015-09-18 15:55:50]

LGTM

[calamity, 2015-09-21 04:42:33]

Patchset #10 (id:210001) has been deleted

[calamity, 2015-09-22 03:00:17]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-09-22 03:00:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
raymes@chromium.org, dbeam@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1316043003/#ps250001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-22 03:00:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1316043003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1316043003/250001

[commit-bot: I haz the power, 2015-09-22 03:10:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 03:10:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[calamity, 2015-09-22 04:47:17]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2015-09-22 04:47:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1316043003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1316043003/250001

[calamity, 2015-09-22 05:15:18]

The CQ bit was unchecked by calamity@chromium.org

[commit-bot: I haz the power, 2015-09-22 05:15:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 05:15:24]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[calamity, 2015-09-22 06:03:04]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-09-22 06:03:04]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
raymes@chromium.org, dbeam@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1316043003/#ps270001
(title: "fix dependencies")

[commit-bot: I haz the power, 2015-09-22 06:03:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1316043003/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1316043003/270001

[commit-bot: I haz the power, 2015-09-22 06:35:59]

Committed patchset #12 (id:270001)

[commit-bot: I haz the power, 2015-09-22 06:37:08]

Patchset 12 (id:??) landed as
https://crrev.com/d59c103a2605ad0edf425d5b19a0e052566d80d2
Cr-Commit-Position: refs/heads/master@{#350116}