[invalidations] Added table with registered objectsIds

[invalidations] Added table with registered objectsIds

This patch is a follow-up to issue 147353011.

The patch adds a display mechanism for the about:invalidations page
that shows a table with every implementer of InvalidationHandler that
has registered for at least one ObjectId for invalidations. The table
also registers for every ObjectId how many invalidations were received
of that kind, the last time one of those arrived and what was the
payload it contained, in case there were any.

There are minimal modifications to every file that implemented
InvalidationHandler because there was a small change in the interface,
in order to be able to return a string stating who is the owner of the
InvalidationHandler without passing raw pointers around.

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

[mferreria_g, 2014-02-11 19:07:45]

Good morning. I've marked down which files you should review for this CL.

atwilson:
chrome/browser/policy/cloud/cloud_policy_invalidator.cc
chrome/browser/policy/cloud/cloud_policy_invalidator.h

dcheng:
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h

rlarocque:
chrome/browser/invalidation/invalidation_logger.cc
chrome/browser/invalidation/invalidation_logger.h
chrome/browser/invalidation/invalidation_logger_observer.h
chrome/browser/invalidation/invalidation_logger_unittest.cc
chrome/browser/invalidation/p2p_invalidation_service.h
chrome/browser/invalidation/ticl_invalidation_service.cc
chrome/browser/invalidation/ticl_invalidation_service.h

zea:
chrome/browser/sync/glue/sync_backend_host_impl.cc
chrome/browser/sync/glue/sync_backend_host_impl.h
sync/internal_api/public/sync_manager.h
sync/internal_api/public/test/fake_sync_manager.h
sync/internal_api/sync_manager_impl.cc
sync/internal_api/sync_manager_impl.h
sync/internal_api/test/fake_sync_manager.cc
sync/notifier/fake_invalidation_handler.cc
sync/notifier/fake_invalidation_handler.h
sync/notifier/invalidation_handler.h
sync/notifier/invalidator_registrar.cc
sync/notifier/invalidator_registrar.h
sync/notifier/non_blocking_invalidator.cc
sync/notifier/non_blocking_invalidator.h
sync/tools/sync_listen_notifications.cc


jhawkins:
chrome/browser/drive/drive_notification_manager.cc
chrome/browser/drive/drive_notification_manager.h
chrome/browser/resources/about_invalidations.css
chrome/browser/resources/about_invalidations.html
chrome/browser/resources/about_invalidations.js
chrome/browser/ui/webui/invalidations_message_handler.cc
chrome/browser/ui/webui/invalidations_message_handler.h

Thank you.

[dcheng, 2014-02-11 20:02:41]

https://codereview.chromium.org/159773006/diff/1/sync/notifier/invalidation_h...
File sync/notifier/invalidation_handler.h (right):

https://codereview.chromium.org/159773006/diff/1/sync/notifier/invalidation_h...
sync/notifier/invalidation_handler.h:26: virtual std::string GetOwnerName() = 0;
Can't this be const?

[mferreria_g, 2014-02-11 20:50:35]

https://codereview.chromium.org/159773006/diff/1/sync/notifier/invalidation_h...
File sync/notifier/invalidation_handler.h (right):

https://codereview.chromium.org/159773006/diff/1/sync/notifier/invalidation_h...
sync/notifier/invalidation_handler.h:26: virtual std::string GetOwnerName() = 0;
On 2014/02/11 20:02:41, dcheng wrote:
> Can't this be const?

Yes. Done.

[Nicolas Zea, 2014-02-11 20:54:10]

https://codereview.chromium.org/159773006/diff/1/sync/internal_api/public/syn...
File sync/internal_api/public/sync_manager.h (right):

https://codereview.chromium.org/159773006/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_manager.h:340: virtual std::string GetOwnerName()
= 0;
What calls this? Does it need to be part of the SyncManager interface, or can it
just be a function of SyncManagerImpl?

Also, make const

[mferreria_g, 2014-02-11 21:23:38]

https://codereview.chromium.org/159773006/diff/1/sync/internal_api/public/syn...
File sync/internal_api/public/sync_manager.h (right):

https://codereview.chromium.org/159773006/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_manager.h:340: virtual std::string GetOwnerName()
= 0;
On 2014/02/11 20:54:11, Nicolas Zea wrote:
> What calls this? Does it need to be part of the SyncManager interface, or can
it
> just be a function of SyncManagerImpl?
> 
> Also, make const

Removed. Hadn't realized that, since it was already defined like that on the
InvalidationHandler interface, there was no need to redefine it the same here.
It is already implemented in SyncManagerImpl.

[Nicolas Zea, 2014-02-11 21:26:35]

sync LGTM

[Nicolas Zea, 2014-02-11 21:58:24]

Doing the invalidations review for rlarocque. Mainly google/chromium style guide
and readability issues

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.h (right):

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:39: InvalidationLogger();
nit: constructor/destructor should be the first methods in the class definition.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:48: void EmitUpdatedIds();
The comment above should below this line. I'd also prefer having a comment for
this method too.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:52: std::map<std::string,
syncer::ObjectIdSet> latest_ids_;
add DISALLOW_COPY_AND_ASSIGN to the private members

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:55: }  // namespace
invalidation
nit: newline after

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger_observer.h (right):

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_observer.h:22: namespace
invalidation {
nit: newline after this

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_observer.h:32: virtual void
OnUpdateIds(std::string handlerName,
pass as a const ref.

Also, handlerName should be handler_name (newState above should also be
new_state).

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger_unittest.cc (right):

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_unittest.cc:22:
registrationReceived = false;
all of these variables (and all variables in the rest of this file) should be in
the format unix_hacker_style.

Not sure if it checks variable names, but you git cl format might help

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_unittest.cc:157: // what was is
sent to the Observer.
nit: what was is -> what was

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_unittest.cc:158: // The
ObserverTest rebuilds the map that is sent in pieces by the logger.
nit: that is sent -> that was sent

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
File chrome/browser/invalidation/p2p_invalidation_service.h (right):

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/p2p_invalidation_service.h:19: class
InvalidationLogger;
nit: newline above

[mferreria_g, 2014-02-12 00:03:46]

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.h (right):

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:39: InvalidationLogger();
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> nit: constructor/destructor should be the first methods in the class
definition.

Done.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:48: void EmitUpdatedIds();
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> The comment above should below this line. I'd also prefer having a comment for
> this method too.

Done.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:52: std::map<std::string,
syncer::ObjectIdSet> latest_ids_;
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> add DISALLOW_COPY_AND_ASSIGN to the private members

Done.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:55: }  // namespace
invalidation
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> nit: newline after

Done.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger_observer.h (right):

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_observer.h:22: namespace
invalidation {
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> nit: newline after this

Done.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_observer.h:32: virtual void
OnUpdateIds(std::string handlerName,
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> pass as a const ref.
> 
> Also, handlerName should be handler_name (newState above should also be
> new_state).

Done.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger_unittest.cc (right):

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_unittest.cc:22:
registrationReceived = false;
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> all of these variables (and all variables in the rest of this file) should be
in
> the format unix_hacker_style.
> 
> Not sure if it checks variable names, but you git cl format might help
They are not checked by cl format. Applied that modification to all variables.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_unittest.cc:157: // what was is
sent to the Observer.
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> nit: what was is -> what was

Done.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger_unittest.cc:158: // The
ObserverTest rebuilds the map that is sent in pieces by the logger.
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> nit: that is sent -> that was sent

Done.

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
File chrome/browser/invalidation/p2p_invalidation_service.h (right):

https://codereview.chromium.org/159773006/diff/100001/chrome/browser/invalida...
chrome/browser/invalidation/p2p_invalidation_service.h:19: class
InvalidationLogger;
On 2014/02/11 21:58:25, Nicolas Zea wrote:
> nit: newline above

Done.

[dcheng, 2014-02-12 00:07:04]

c/b/e/a/push_messaging/* LGTM with a nit.

https://codereview.chromium.org/159773006/diff/180001/chrome/browser/extensio...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

https://codereview.chromium.org/159773006/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:184:
return "Push";
Nit: It might make things clearer in about:invalidations to name this
"PushMessagingApi".

[mferreria_g, 2014-02-12 00:11:13]

https://codereview.chromium.org/159773006/diff/180001/chrome/browser/extensio...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

https://codereview.chromium.org/159773006/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:184:
return "Push";
On 2014/02/12 00:07:05, dcheng wrote:
> Nit: It might make things clearer in about:invalidations to name this
> "PushMessagingApi".

Done.

[James Hawkins, 2014-02-13 00:54:47]

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.html (right):

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.html:19: <div class="section"
id="objectsid-table-div">
nit: ID first.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.html:23: as jstemplate can't deal
with them-->
I think you can remove this comment.  If someone tries to change this, they'll
find out quickly it doesn't work, right?

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.js (right):

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:31: *  Function that updates the
jstemplate with the latest
'Function that' is superfluous:

'Updates the...'

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:31: *  Function that updates the
jstemplate with the latest
Please fill out the 80 cols.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:46: for (var key in
tableObjects) {
nit: No braces for single-line block.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:53:
sortedInvalidations.push(tableObjects[keys[i]]);
nit: No braces.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:92: var isInvalidation = true;
What is the purpose of this variable?

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:141: /**
nit: Indentation is off, blank line required above this.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:151: 
nit: Remove blank line.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:157: tableObjects[key].type =
'greyed';
nit: Remove braces.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/invalidations_message_handler.cc (right):

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/invalidations_message_handler.cc:40: if (!is_registered_
&& profile) {
You can't rid of the need for this member variable by using (an added)
logger_->IsObserverRegistered(this).

The need for the private member variable seems off to me.  How many times is
UIReady called?  Should only be once per lifetime of this object.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/invalidations_message_handler.h (right):

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/invalidations_message_handler.h:50: // Called by the
javascript whenever the page is ready to receive
I think 'messages' can fit on this line, but I'm eyeballing it.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/invalidations_message_handler.h:55: // Transform a set
to a ListValue in order to be automatically
Transforms

[mferreria_g, 2014-02-13 01:05:52]

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.html (right):

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.html:19: <div class="section"
id="objectsid-table-div">
On 2014/02/13 00:54:47, James Hawkins wrote:
> nit: ID first.

Done.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.html:23: as jstemplate can't deal
with them-->
On 2014/02/13 00:54:47, James Hawkins wrote:
> I think you can remove this comment.  If someone tries to change this, they'll
> find out quickly it doesn't work, right?

I guess so, but it was quite hard to debug the first time as it is not obvious
why it stops working.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.js (right):

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:31: *  Function that updates the
jstemplate with the latest
On 2014/02/13 00:54:47, James Hawkins wrote:
> Please fill out the 80 cols.

Done.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:31: *  Function that updates the
jstemplate with the latest
On 2014/02/13 00:54:47, James Hawkins wrote:
> 'Function that' is superfluous:
> 
> 'Updates the...'

Done.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:46: for (var key in
tableObjects) {
On 2014/02/13 00:54:47, James Hawkins wrote:
> nit: No braces for single-line block.

Done.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:53:
sortedInvalidations.push(tableObjects[keys[i]]);
On 2014/02/13 00:54:47, James Hawkins wrote:
> nit: No braces.

Done.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:92: var isInvalidation = true;
On 2014/02/13 00:54:47, James Hawkins wrote:
> What is the purpose of this variable?

Clarity, to easily see what true or false of this parameter means.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:141: /**
On 2014/02/13 00:54:47, James Hawkins wrote:
> nit: Indentation is off, blank line required above this.

Done.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:151: 
On 2014/02/13 00:54:47, James Hawkins wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:157: tableObjects[key].type =
'greyed';
On 2014/02/13 00:54:47, James Hawkins wrote:
> nit: Remove braces.

Done.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/invalidations_message_handler.cc (right):

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/invalidations_message_handler.cc:40: if (!is_registered_
&& profile) {
On 2014/02/13 00:54:47, James Hawkins wrote:
> You can't rid of the need for this member variable by using (an added)
> logger_->IsObserverRegistered(this).
> 
> The need for the private member variable seems off to me.  How many times is
> UIReady called?  Should only be once per lifetime of this object.
It actually isn't. UIReady is called every time the page is refreshed, because
it is called by the JS onDocumentLoad. So this flag was added here to prevent
the message handler from being reregistered. Also, this was done as not to
expose any more of the ObserverList implementation of InvalidationLogger.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/invalidations_message_handler.h (right):

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/invalidations_message_handler.h:50: // Called by the
javascript whenever the page is ready to receive
On 2014/02/13 00:54:47, James Hawkins wrote:
> I think 'messages' can fit on this line, but I'm eyeballing it.

Done.

https://codereview.chromium.org/159773006/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/invalidations_message_handler.h:55: // Transform a set
to a ListValue in order to be automatically
On 2014/02/13 00:54:47, James Hawkins wrote:
> Transforms

Done.

[Andrew T Wilson (Slow), 2014-02-13 08:36:59]

policy lgtm

[Nicolas Zea, 2014-02-14 23:23:22]

invalidations LGTM with a few nits. Richard will need to give an owners stamp

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.cc (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.cc:44: std::map<std::string,
syncer::ObjectIdSet> updatedIds) {
here too

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.h (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:30: ~InvalidationLogger();
nit: newline after

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:35: void OnStateChange(const
syncer::InvalidatorState& newState);
newState and updatedIds should be unix_hacker_style

[mferreria_g, 2014-02-14 23:57:02]

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.cc (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.cc:44: std::map<std::string,
syncer::ObjectIdSet> updatedIds) {
On 2014/02/14 23:23:22, Nicolas Zea wrote:
> here too

Done.

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.h (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:30: ~InvalidationLogger();
On 2014/02/14 23:23:22, Nicolas Zea wrote:
> nit: newline after

Done.

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:35: void OnStateChange(const
syncer::InvalidatorState& newState);
On 2014/02/14 23:23:22, Nicolas Zea wrote:
> newState and updatedIds should be unix_hacker_style

Done.

[rlarocque, 2014-02-15 00:09:38]

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.cc (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.cc:48: ++it) {
The indentation here looks strange.  Does this need to be four lines?

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.css (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.css:20: 
Remove extra newline.

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.js (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:34: var header = {
Why not just put this in the HTML?  Putting it there inside a <thead> element
might make it easier to style, too.

[mferreria_g, 2014-02-15 01:27:38]

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.cc (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.cc:48: ++it) {
On 2014/02/15 00:09:39, rlarocque wrote:
> The indentation here looks strange.  Does this need to be four lines?

That's what cl format did. I've managed to get in two lines, I think it's better
now.

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.css (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.css:20: 
On 2014/02/15 00:09:39, rlarocque wrote:
> Remove extra newline.

Done.

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.js (right):

https://codereview.chromium.org/159773006/diff/350001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.js:34: var header = {
On 2014/02/15 00:09:39, rlarocque wrote:
> Why not just put this in the HTML?  Putting it there inside a <thead> element
> might make it easier to style, too.

Wasn't aware of the <thead> element. Done.

[rlarocque, 2014-02-15 01:35:17]

LGTM with nits.

https://codereview.chromium.org/159773006/diff/530001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.html (right):

https://codereview.chromium.org/159773006/diff/530001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.html:26: <td>Registrar</td>
I think <th> would be more appropriate.

https://codereview.chromium.org/159773006/diff/530001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.html:35: <tr
jsselect="objectsidtable" jsvalues="class:$this.type">
There's a <tbody> element, too.  I'd be surprised if it has any practical
effect, but I think it would be slightly more correct to use it.

[mferreria_g, 2014-02-15 01:54:43]

James, I am waiting for your final input on this issue, thanks.

https://codereview.chromium.org/159773006/diff/530001/chrome/browser/resource...
File chrome/browser/resources/about_invalidations.html (right):

https://codereview.chromium.org/159773006/diff/530001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.html:26: <td>Registrar</td>
On 2014/02/15 01:35:18, rlarocque wrote:
> I think <th> would be more appropriate.
Agree. Done.

https://codereview.chromium.org/159773006/diff/530001/chrome/browser/resource...
chrome/browser/resources/about_invalidations.html:35: <tr
jsselect="objectsidtable" jsvalues="class:$this.type">
On 2014/02/15 01:35:18, rlarocque wrote:
> There's a <tbody> element, too.  I'd be surprised if it has any practical
> effect, but I think it would be slightly more correct to use it.
Done. No effect, but made it a bit more HTML compliant.

[James Hawkins, 2014-02-18 17:19:21]

https://codereview.chromium.org/159773006/diff/580001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.h (right):

https://codereview.chromium.org/159773006/diff/580001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:27: class InvalidationLogger {
Please add documentation for this class.  It's hard to reason about the design
otherwise.

https://codereview.chromium.org/159773006/diff/580001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:45: // Add and remove
observers listening for messages.
The above need for documentation aside, I don't think this we're handling the
observations correctly; we've exposed some of the Observer API, but not enough
to prevent clients from needing to do hacky things (like store registered
state).  I'll wait for the class documentation in order to dive further.

[mferreria_g, 2014-02-18 18:06:50]

https://codereview.chromium.org/159773006/diff/580001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.h (right):

https://codereview.chromium.org/159773006/diff/580001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:27: class InvalidationLogger {
On 2014/02/18 17:19:22, James Hawkins wrote:
> Please add documentation for this class.  It's hard to reason about the design
> otherwise.
Done. Is this clearer?

https://codereview.chromium.org/159773006/diff/580001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:45: // Add and remove
observers listening for messages.
On 2014/02/18 17:19:22, James Hawkins wrote:
> The above need for documentation aside, I don't think this we're handling the
> observations correctly; we've exposed some of the Observer API, but not enough
> to prevent clients from needing to do hacky things (like store registered
> state).  I'll wait for the class documentation in order to dive further.

The stored registration is something related to an implementation detail of
WebUIMessageHandler. WebUIMessageHandler provides no way of distinguishing when
a webpage is refreshed compared to when a new browser instance is created. That
means "UIReady" can be called multiple times during the MessageHandler lifespan,
even though it is already registered (because it is a DOM onLoad event).
I don't think it is necessary to expose that internal state because I don't see
most use cases having those kinds of problems.

[James Hawkins, 2014-02-19 00:09:50]

https://codereview.chromium.org/159773006/diff/630001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.h (right):

https://codereview.chromium.org/159773006/diff/630001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:29: // This class is in charge
of logging invalidation-related information.
nit: Class documentation should go just above the class declaration (line 26).

https://codereview.chromium.org/159773006/diff/630001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:59: void
RegisterObserver(InvalidationLoggerObserver* debug_observer);
I'm still not sure why this is better than just exposing an Observer instance,
e.g., getObserver()->RegisterObserver().

Then you can use getObserver()->IsRegistered() in the webui code.

[mferreria, 2014-02-19 00:45:21]

https://codereview.chromium.org/159773006/diff/630001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_logger.h (right):

https://codereview.chromium.org/159773006/diff/630001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:29: // This class is in charge
of logging invalidation-related information.
On 2014/02/19 00:09:51, James Hawkins wrote:
> nit: Class documentation should go just above the class declaration (line 26).

Done.

https://codereview.chromium.org/159773006/diff/630001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_logger.h:59: void
RegisterObserver(InvalidationLoggerObserver* debug_observer);
On 2014/02/19 00:09:51, James Hawkins wrote:
> I'm still not sure why this is better than just exposing an Observer instance,
> e.g., getObserver()->RegisterObserver().
> 
> Then you can use getObserver()->IsRegistered() in the webui code.

Added a function isObserverRegistered that exposes this functionality from the
underlying ObserverList implementation. This removes the is_registered boolean
flag.

[James Hawkins, 2014-02-19 01:02:29]

On 2014/02/19 00:45:21, mferreria wrote:
>
https://codereview.chromium.org/159773006/diff/630001/chrome/browser/invalida...
> File chrome/browser/invalidation/invalidation_logger.h (right):
> 
>
https://codereview.chromium.org/159773006/diff/630001/chrome/browser/invalida...
> chrome/browser/invalidation/invalidation_logger.h:29: // This class is in
charge
> of logging invalidation-related information.
> On 2014/02/19 00:09:51, James Hawkins wrote:
> > nit: Class documentation should go just above the class declaration (line
26).
> 
> Done.
> 
>
https://codereview.chromium.org/159773006/diff/630001/chrome/browser/invalida...
> chrome/browser/invalidation/invalidation_logger.h:59: void
> RegisterObserver(InvalidationLoggerObserver* debug_observer);
> On 2014/02/19 00:09:51, James Hawkins wrote:
> > I'm still not sure why this is better than just exposing an Observer
instance,
> > e.g., getObserver()->RegisterObserver().
> > 
> > Then you can use getObserver()->IsRegistered() in the webui code.
> 
> Added a function isObserverRegistered that exposes this functionality from the
> underlying ObserverList implementation. This removes the is_registered boolean
> flag.

OK; I don't think this addresses my underlying design concern, but I don't want
to hold this up any longer.

LGTM

[mferreria, 2014-02-19 01:03:06]

The CQ bit was checked by mferreria@chromium.org

[commit-bot: I haz the power, 2014-02-19 01:04:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mferreria@chromium.org/159773006/690001

[commit-bot: I haz the power, 2014-02-19 02:19:10]

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

[commit-bot: I haz the power, 2014-02-19 02:19:11]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[mferreria, 2014-02-19 22:23:16]

The CQ bit was checked by mferreria@chromium.org

[commit-bot: I haz the power, 2014-02-19 22:48:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mferreria@chromium.org/159773006/960001

[commit-bot: I haz the power, 2014-02-20 00:53:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mferreria@chromium.org/159773006/960001

[mferreria, 2014-02-20 02:39:12]

The CQ bit was checked by mferreria@chromium.org

[commit-bot: I haz the power, 2014-02-20 04:43:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mferreria@chromium.org/159773006/1180001

[commit-bot: I haz the power, 2014-02-20 08:48:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mferreria@chromium.org/159773006/1180001

[commit-bot: I haz the power, 2014-02-20 12:20:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mferreria@chromium.org/159773006/1180001

[commit-bot: I haz the power, 2014-02-20 13:16:08]

Change committed as 252210