[Sync] Add a simple UI to sync-internals to create UserEvents.

[Sync] Add a simple UI to sync-internals to create UserEvents.

Very simple UI and plumbing to create UserEvents from
chrome://sync-internals. Since there are not currently any defined
event types, we cannot do anything interesting. Currently expose text
fields to specify int64 event time and navigation time. Invalid
navigation time will result in meaningless event. Tab should be hidden
if the feature is not enabled (disabled is the default).

BUG=719044
Review-Url: https://codereview.chromium.org/2872023002
Cr-Commit-Position: refs/heads/master@{#474814}
Committed: https://chromium.googlesource.com/chromium/src/+/6c146620037bf6c1222d5a0255be9a6ef2ce5137

[skym, 2017-05-09 23:25:46]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 23:27:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-05-09 23:28:06]

Description was changed from

==========
[Sync] Add a simple UI to sync-internals to create UserEvents.

BUG=719044
==========

to

==========
[Sync] Add a simple UI to sync-internals to create UserEvents.

Very simple UI and plumbing to create UserEvents from
chrome://sync-internals. Since there are not currently any defined
event types, we cannot do anything interesting. Currently expose text
fields to specify int64 event time and navigation time. Invalid
navigation time will result in meaningless event. Tab should be hidden
if the feature is not enabled (disabled is the default).

BUG=719044
==========

[skym, 2017-05-09 23:28:06]

skym@chromium.org changed reviewers:
+ dbeam@chromium.org, pnoland@chromium.org

[skym, 2017-05-09 23:29:27]

PTAL
components/sync/* - pnoland@
chrome/browser/ui/webui/* and components/resources/* - dbeam@

[Patrick Noland, 2017-05-09 23:46:23]

https://codereview.chromium.org/2872023002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2872023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:45: int64_t
IndexToInt64(const base::ListValue* list, int index) {
From the name I'd expect this to convert an index, not the value in the list at
the index. Maybe something like StringAtIndexToInt64?

https://codereview.chromium.org/2872023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:53: return 0;
So if you fat-finger one of the inputs the value will just silently default to
0. Are you sure that's what you want? I'm not sure how much validation there is
for UserEvents, but I know that e.g. ChromeHistory protos won't be written at
the server level if their timestamp falls outside of an acceptable window.

https://codereview.chromium.org/2872023002/diff/20001/components/sync/driver/...
File components/sync/driver/resources/user_events.js (right):

https://codereview.chromium.org/2872023002/diff/20001/components/sync/driver/...
components/sync/driver/resources/user_events.js:8:
document.getElementById('event-time-usec-input').value,
What happens if either of these fields has whitespace?

[skym, 2017-05-10 00:06:46]

Updates for Patrick.

https://codereview.chromium.org/2872023002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2872023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:45: int64_t
IndexToInt64(const base::ListValue* list, int index) {
On 2017/05/09 23:46:23, Patrick Noland wrote:
> From the name I'd expect this to convert an index, not the value in the list
at
> the index. Maybe something like StringAtIndexToInt64?

Done.

https://codereview.chromium.org/2872023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:53: return 0;
On 2017/05/09 23:46:23, Patrick Noland wrote:
> So if you fat-finger one of the inputs the value will just silently default to
> 0. Are you sure that's what you want? I'm not sure how much validation there
is
> for UserEvents, but I know that e.g. ChromeHistory protos won't be written at
> the server level if their timestamp falls outside of an acceptable window. 

I think feedback in the UI that they've entered a value that doesn't convert
nicely to an int would be a great improvement, but I think we should improve
iteratively, seem reasonable?

https://codereview.chromium.org/2872023002/diff/20001/components/sync/driver/...
File components/sync/driver/resources/user_events.js (right):

https://codereview.chromium.org/2872023002/diff/20001/components/sync/driver/...
components/sync/driver/resources/user_events.js:8:
document.getElementById('event-time-usec-input').value,
On 2017/05/09 23:46:23, Patrick Noland wrote:
> What happens if either of these fields has whitespace?

"" => 0
" " => 0
" 123 " => 0
"12 3" => 0
"123" => 123

[Patrick Noland, 2017-05-10 00:08:14]

lgtm

[skym, 2017-05-12 18:45:18]

Friendly ping dbeam@

[skym, 2017-05-15 18:34:19]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 18:35:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 20:47:12]

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

[commit-bot: I haz the power, 2017-05-15 20:47:13]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-05-16 03:35:52]

https://codereview.chromium.org/2872023002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2872023002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:205:
web_ui()->CallJavascriptFunctionUnsafe(
can we use the safe version instead?

https://codereview.chromium.org/2872023002/diff/40001/components/resources/sy...
File components/resources/sync_driver_resources.grdp (right):

https://codereview.chromium.org/2872023002/diff/40001/components/resources/sy...
components/resources/sync_driver_resources.grdp:14: <include
name="IDR_SYNC_DRIVER_SYNC_INTERNALS_USER_EVENTS_JS" 
file="../sync/driver/resources/user_events.js" type="BINDATA" />
can you remove the double space before file=?  it seems the copy pasta will
continue otherwise

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
File components/sync/driver/resources/sync_index.js (right):

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
components/sync/driver/resources/sync_index.js:17:
document.getElementById('sync-user-events-tab').style.display = display;
can you just use .hidden = true/value instead?

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
File components/sync/driver/resources/user_events.js (right):

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
components/sync/driver/resources/user_events.js:8:
document.getElementById('event-time-usec-input').value,
can you just use $('event-time-usec-input') instead of
document.getElementById()?

[skym, 2017-05-16 18:19:40]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 18:21:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-05-16 19:25:08]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 19:26:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-05-16 19:26:49]

Updates for dbeam@

https://codereview.chromium.org/2872023002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2872023002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:205:
web_ui()->CallJavascriptFunctionUnsafe(
On 2017/05/16 03:35:51, Dan Beam wrote:
> can we use the safe version instead?

Done. And switched all the other invocations in this file to
content::WebUIMessageHandler::CallJavascriptFunction(), but now I need to call
AllowJavascript() at some point.

So I put AllowJavascript() into the start of every HandleX method. Is this
reasonable? It seems to be the way most people deal with this
https://cs.chromium.org/search/?q=%22+AllowJavascript();%22&type=cs , but does
it defeat the purpose of calling the safe version?

Maybe it would be more elegant if I had a single DOMContentLoaded listener that
called up the the message handler telling me that the page has finished loading,
but, we have a whole bunch of independent DOMContentLoaded listeners on this
page,
https://cs.chromium.org/search/?q=DOMContentLoaded+file:%5Esrc/components/syn...

Should I try to consolidate these into a single on ui ready call? It seems kind
of nice that everything is modular and independent right now, although which
DOMContentLoaded listener gets called first seems very unclear.

https://codereview.chromium.org/2872023002/diff/40001/components/resources/sy...
File components/resources/sync_driver_resources.grdp (right):

https://codereview.chromium.org/2872023002/diff/40001/components/resources/sy...
components/resources/sync_driver_resources.grdp:14: <include
name="IDR_SYNC_DRIVER_SYNC_INTERNALS_USER_EVENTS_JS" 
file="../sync/driver/resources/user_events.js" type="BINDATA" />
On 2017/05/16 03:35:51, Dan Beam wrote:
> can you remove the double space before file=?  it seems the copy pasta will
> continue otherwise

Done. Updated the others as well.

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
File components/sync/driver/resources/sync_index.js (right):

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
components/sync/driver/resources/sync_index.js:17:
document.getElementById('sync-user-events-tab').style.display = display;
On 2017/05/16 03:35:52, Dan Beam wrote:
> can you just use .hidden = true/value instead?

I'm still learning how these things work, so I might be overlooking something
obvious. My interpretation of your comment is that you wanted me to add
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden

However, this attribute seems to get overriden by the CSS display property,
which we currently inherit from
https://cs.chromium.org/chromium/src/ui/webui/resources/css/tabs.css?l=38

display: block; seems to significantly change the visual look, so even if I
removed the display property when hidden was applied, I would also need to add
it back when I removed the hidden attribute, which means this code cannot be
simplified.

This page doesn't seem to be the only consumer of tabs.css, so removing the
display property from the style rule seems dangerous.
https://cs.chromium.org/search/?q=chrome://resources/css/tabs.css&type=cs

I tried copying what was done in
https://chromiumcodereview.appspot.com/15894032/ but I'm not sure what I was
really expecting, it just adds more stuff, the display: block; property still
gets through.

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
File components/sync/driver/resources/user_events.js (right):

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
components/sync/driver/resources/user_events.js:8:
document.getElementById('event-time-usec-input').value,
On 2017/05/16 03:35:52, Dan Beam wrote:
> can you just use $('event-time-usec-input') instead of
> document.getElementById()?

Done. Changed sync_index.js's usage of document.getElementById as well.

Can you explain to me what's going on with this $('id') syntax? Googling for
this functionality is seems kind of like jquery but it doesn't contain a '#'.
Have we defined this lookup mechanism ourselves within our internal Chrome web
ui somewhere?

Also, why exactly is the $('id') version superior to the
document.getElementById('id') approach? Just because it's shorter and consistent
with the rest of the code? Or does it actually do something different?

I'm realizing some other .js files in this folder setup dependencies like

// require: cr.js
// require: cr/ui.js
// require: cr/ui/tree.js

I'm having a very difficult time figuring out code prompts the cr.js dependency.
Should I be doing any of this in user_events.js or sync_index.js?

[commit-bot: I haz the power, 2017-05-16 22:06:22]

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

[commit-bot: I haz the power, 2017-05-16 22:06:22]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-05-16 22:40:54]

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:51: }
no curlies

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:89: }
you probably want to put this in a OnJavascriptDisallowed() override method

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:204:
weak_ptr_factory_.GetWeakPtr(), request_id));
for something like this, the weak_ptr_factory_ is associated with this class,
but this class is created before the renderer exists and continues living even
if the renderer is refreshed, crashed, etc.

so it seems that this callback may fire when javascript is not allowed.

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:288:
TryCallDispatch(Value(name), details.Get());
i don't really see how doing AllowJavascript() then CallJavascriptFunction()
right after would fail

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:310: if
(IsJavascriptAllowed()) {
this does work but maybe it'd be easier just to disconnect from listening to
events when OnJavascriptDisallowed() is called and start listening to events
when OnJavascriptAllowed() is called?

https://codereview.chromium.org/2872023002/diff/80001/components/sync/driver/...
File components/sync/driver/resources/index.html (right):

https://codereview.chromium.org/2872023002/diff/80001/components/sync/driver/...
components/sync/driver/resources/index.html:69: <tab id="sync-user-events-tab"
style="display:none;">User Events</tab>
<tab id="sync-user-events-tab" hidden>

https://codereview.chromium.org/2872023002/diff/80001/components/sync/driver/...
File components/sync/driver/resources/sync_index.js (right):

https://codereview.chromium.org/2872023002/diff/80001/components/sync/driver/...
components/sync/driver/resources/sync_index.js:16:
$('sync-user-events-tab').style.display = visible ? 'block' : 'none';
$('sync-user-events-tab').hidden = !visible;

[skym, 2017-05-22 21:41:59]

skym@chromium.org changed reviewers:
+ jochen@chromium.org

[skym, 2017-05-22 21:42:45]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 21:43:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-05-22 21:55:48]

More updates for dbeam, split out some refactoring of
SyncInternalsMessageHandler into https://codereview.chromium.org/2898723003/ ,
which simplifies this CL.

+jochen for components/resources/sync_driver_resources.grdp

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:51: }
On 2017/05/16 22:40:54, Dan Beam wrote:
> no curlies

How strongly do you feel about this? It seems that
https://google.github.io/styleguide/cppguide.html#Conditionals seems to indicate
it's optional. I like putting curly braces everywhere.

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:89: }
On 2017/05/16 22:40:54, Dan Beam wrote:
> you probably want to put this in a OnJavascriptDisallowed() override method

Done in https://codereview.chromium.org/2898723003/

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:204:
weak_ptr_factory_.GetWeakPtr(), request_id));
On 2017/05/16 22:40:53, Dan Beam wrote:
> for something like this, the weak_ptr_factory_ is associated with this class,
> but this class is created before the renderer exists and continues living even
> if the renderer is refreshed, crashed, etc.
> 
> so it seems that this callback may fire when javascript is not allowed.

You're right, and I didn't have a try in OnReceivedAllNodes, whoops. Added an
explicit check that javascript is still allowed in Done in
https://codereview.chromium.org/2898723003/

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:288:
TryCallDispatch(Value(name), details.Get());
On 2017/05/16 22:40:54, Dan Beam wrote:
> i don't really see how doing AllowJavascript() then CallJavascriptFunction()
> right after would fail

Whoops, that was sloppy of me, thanks for catching this. Removed the allow call.

https://codereview.chromium.org/2872023002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:310: if
(IsJavascriptAllowed()) {
On 2017/05/16 22:40:54, Dan Beam wrote:
> this does work but maybe it'd be easier just to disconnect from listening to
> events when OnJavascriptDisallowed() is called and start listening to events
> when OnJavascriptAllowed() is called?

Done in https://codereview.chromium.org/2898723003/

https://codereview.chromium.org/2872023002/diff/80001/components/sync/driver/...
File components/sync/driver/resources/index.html (right):

https://codereview.chromium.org/2872023002/diff/80001/components/sync/driver/...
components/sync/driver/resources/index.html:69: <tab id="sync-user-events-tab"
style="display:none;">User Events</tab>
On 2017/05/16 22:40:54, Dan Beam wrote:
> <tab id="sync-user-events-tab" hidden>

Has no effect, see my comment at
https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...

https://codereview.chromium.org/2872023002/diff/80001/components/sync/driver/...
File components/sync/driver/resources/sync_index.js (right):

https://codereview.chromium.org/2872023002/diff/80001/components/sync/driver/...
components/sync/driver/resources/sync_index.js:16:
$('sync-user-events-tab').style.display = visible ? 'block' : 'none';
On 2017/05/16 22:40:54, Dan Beam wrote:
> $('sync-user-events-tab').hidden = !visible;

This doesn't work, see
https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
for my explanation of why not.

[skym, 2017-05-22 22:05:46]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 22:06:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2017-05-22 22:15:49]

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
File components/sync/driver/resources/sync_index.js (right):

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
components/sync/driver/resources/sync_index.js:17:
document.getElementById('sync-user-events-tab').style.display = display;
On 2017/05/16 19:26:49, skym wrote:
> On 2017/05/16 03:35:52, Dan Beam wrote:
> > can you just use .hidden = true/value instead?
> 
> I'm still learning how these things work, so I might be overlooking something
> obvious. My interpretation of your comment is that you wanted me to add
> https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden
> 
> However, this attribute seems to get overriden by the CSS display property,
> which we currently inherit from
> https://cs.chromium.org/chromium/src/ui/webui/resources/css/tabs.css?l=38
> 
> display: block; seems to significantly change the visual look, so even if I
> removed the display property when hidden was applied, I would also need to add
> it back when I removed the hidden attribute, which means this code cannot be
> simplified.
> 
> This page doesn't seem to be the only consumer of tabs.css, so removing the
> display property from the style rule seems dangerous.
> https://cs.chromium.org/search/?q=chrome://resources/css/tabs.css&type=cs
> 
> I tried copying what was done in
> https://chromiumcodereview.appspot.com/15894032/ but I'm not sure what I was
> really expecting, it just adds more stuff, the display: block; property still
> gets through.

add this rule somewhere in .css or in a <style> in your .html.  pretty much
everywhere does this.  the spec on [hidden] was kinda poorly thought out.

[hidden] {
  display: none !important;
}

[skym, 2017-05-22 22:55:44]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[skym, 2017-05-22 22:55:46]

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
File components/sync/driver/resources/sync_index.js (right):

https://codereview.chromium.org/2872023002/diff/40001/components/sync/driver/...
components/sync/driver/resources/sync_index.js:17:
document.getElementById('sync-user-events-tab').style.display = display;
On 2017/05/22 22:15:49, Dan Beam wrote:
> On 2017/05/16 19:26:49, skym wrote:
> > On 2017/05/16 03:35:52, Dan Beam wrote:
> > > can you just use .hidden = true/value instead?
> > 
> > I'm still learning how these things work, so I might be overlooking
something
> > obvious. My interpretation of your comment is that you wanted me to add
> > https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden
> > 
> > However, this attribute seems to get overriden by the CSS display property,
> > which we currently inherit from
> > https://cs.chromium.org/chromium/src/ui/webui/resources/css/tabs.css?l=38
> > 
> > display: block; seems to significantly change the visual look, so even if I
> > removed the display property when hidden was applied, I would also need to
add
> > it back when I removed the hidden attribute, which means this code cannot be
> > simplified.
> > 
> > This page doesn't seem to be the only consumer of tabs.css, so removing the
> > display property from the style rule seems dangerous.
> > https://cs.chromium.org/search/?q=chrome://resources/css/tabs.css&type=cs
> > 
> > I tried copying what was done in
> > https://chromiumcodereview.appspot.com/15894032/ but I'm not sure what I was
> > really expecting, it just adds more stuff, the display: block; property
still
> > gets through.
> 
> add this rule somewhere in .css or in a <style> in your .html.  pretty much
> everywhere does this.  the spec on [hidden] was kinda poorly thought out.
> 
> [hidden] {
>   display: none !important;
> }

Huh, that's pretty clever. Thanks for the information! Done.

[commit-bot: I haz the power, 2017-05-22 22:56:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2017-05-22 23:44:47]

lgtm i'll make sure to keep looking at your dependent patchset, though

https://codereview.chromium.org/2872023002/diff/140001/components/sync/driver...
File components/sync/driver/resources/chrome_sync.js (right):

https://codereview.chromium.org/2872023002/diff/140001/components/sync/driver...
components/sync/driver/resources/chrome_sync.js:88: * Sends data to construct a
user event that should be committed.
can you document each @param here?

https://codereview.chromium.org/2872023002/diff/140001/components/sync/driver...
File components/sync/driver/resources/index.html (right):

https://codereview.chromium.org/2872023002/diff/140001/components/sync/driver...
components/sync/driver/resources/index.html:59: tab[hidden] {
i think you should remove the "tab" part and just make this "[hidden] {" because
others might try to use hidden assuming it'll work.

note: this is a really common pattern

https://cs.chromium.org/search/?q=%22%5Bhidden%5D+%7B%22&sq=package:chromium&...

[commit-bot: I haz the power, 2017-05-23 00:55:47]

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

[commit-bot: I haz the power, 2017-05-23 00:55:48]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jochen (gone - plz use gerrit), 2017-05-23 11:16:58]

would it make sense to make //components/sync/OWNERS also own
//components/resource/sync_driver_resources.grdp?

[skym, 2017-05-24 00:19:44]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[skym, 2017-05-24 00:20:06]

On 2017/05/23 11:16:58, jochen wrote:
> would it make sense to make //components/sync/OWNERS also own
> //components/resource/sync_driver_resources.grdp?

Done.

[skym, 2017-05-24 00:20:25]

Updates for dbeam and jochen.

https://codereview.chromium.org/2872023002/diff/140001/components/sync/driver...
File components/sync/driver/resources/chrome_sync.js (right):

https://codereview.chromium.org/2872023002/diff/140001/components/sync/driver...
components/sync/driver/resources/chrome_sync.js:88: * Sends data to construct a
user event that should be committed.
On 2017/05/22 23:44:46, Dan Beam wrote:
> can you document each @param here?

Done.

https://codereview.chromium.org/2872023002/diff/140001/components/sync/driver...
File components/sync/driver/resources/index.html (right):

https://codereview.chromium.org/2872023002/diff/140001/components/sync/driver...
components/sync/driver/resources/index.html:59: tab[hidden] {
On 2017/05/22 23:44:46, Dan Beam wrote:
> i think you should remove the "tab" part and just make this "[hidden] {"
because
> others might try to use hidden assuming it'll work.
> 
> note: this is a really common pattern
> 
>
https://cs.chromium.org/search/?q=%22%5Bhidden%5D+%7B%22&sq=package:chromium&...

Done.

[commit-bot: I haz the power, 2017-05-24 00:20:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 04:59:45]

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

[commit-bot: I haz the power, 2017-05-24 04:59:46]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-05-24 19:38:05]

lgtm

[skym, 2017-05-25 19:58:11]

The CQ bit was checked by skym@chromium.org

[skym, 2017-05-25 19:58:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
pnoland@chromium.org, dbeam@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2872023002/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-25 19:58:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 21:39:08]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1495742291924740,
"parent_rev": "e96185d01c286ffb9892e3f7dff6765bea1dc81b", "commit_rev":
"6c146620037bf6c1222d5a0255be9a6ef2ce5137"}

[commit-bot: I haz the power, 2017-05-25 21:39:21]

Description was changed from

==========
[Sync] Add a simple UI to sync-internals to create UserEvents.

Very simple UI and plumbing to create UserEvents from
chrome://sync-internals. Since there are not currently any defined
event types, we cannot do anything interesting. Currently expose text
fields to specify int64 event time and navigation time. Invalid
navigation time will result in meaningless event. Tab should be hidden
if the feature is not enabled (disabled is the default).

BUG=719044
==========

to

==========
[Sync] Add a simple UI to sync-internals to create UserEvents.

Very simple UI and plumbing to create UserEvents from
chrome://sync-internals. Since there are not currently any defined
event types, we cannot do anything interesting. Currently expose text
fields to specify int64 event time and navigation time. Invalid
navigation time will result in meaningless event. Tab should be hidden
if the feature is not enabled (disabled is the default).

BUG=719044

Review-Url: https://codereview.chromium.org/2872023002
Cr-Commit-Position: refs/heads/master@{#474814}
Committed:
https://chromium.googlesource.com/chromium/src/+/6c146620037bf6c1222d5a0255be...
==========

[commit-bot: I haz the power, 2017-05-25 21:39:23]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/6c146620037bf6c1222d5a0255be...