Process incoming updates and deletes properly.

Process incoming updates and deletes properly.

There are two paths into synced notifications - MergeDataAndStartSyncing, and
ProcessSyncChanges.

We had been handling updates and deletes on the MergeDataAndStartSyncing (catch
up pull path) case, but not in the ProcessSyncChanges (push path).  This change
adds the missing code for the push path and some unit tests.

We also refactor slightly the Display method, replacing calls with
"UpdateInMessageCenter", which will display or remove as needed.

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

[dewittj, 2013-08-08 22:15:32]

https://codereview.chromium.org/22470006/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/22470006/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:195: if
(found == NULL) {
Unless I understand the protocol wrong, this could cause redundant
notifications.  Say an ACTION_UPDATE happens at the same time as a user clicking
(x), then they will get the same (updated) notification again as a toast. 
Perhaps there should be a cache of changes made on the client that can override
changes coming down from the server?

[Pete Williamson, 2013-08-09 01:27:22]

https://codereview.chromium.org/22470006/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/22470006/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:195: if
(found == NULL) {
On 2013/08/08 22:15:32, dewittj wrote:
> Unless I understand the protocol wrong, this could cause redundant
> notifications.  Say an ACTION_UPDATE happens at the same time as a user
clicking
> (x), then they will get the same (updated) notification again as a toast. 
> Perhaps there should be a cache of changes made on the client that can
override
> changes coming down from the server?

This seems like a fairly rare race condition, it seems abnormal for
notifications to be updated while onscreen since then normally only live for 7
seconds as toast.  Also, if the message changed in a visible way (say a new
message line) the user would understand why it is being shown again.

I propose we leave the code as is here.

[dewittj, 2013-08-09 19:40:28]

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:223:
NOTREACHED();
You don't handle ACTION_INVALID, yet you have a NOTREACHED here.  I suspect
you'd want to handle that rather than crash the browser in the event of an
invalid action.

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:321:
notification_data_.begin();
This indentation is really weird here.  Can I recommend:

ScopedVector<SyncedNotification>::iterator it = notification_data_.begin();
for (; it != notification_data_.end(); ++it) { ... }

In fact, I think (unless my c++ fu is failing me here) that you should use
ScopedVector<SyncedNotification>::iterator everywhere in this file instead of
std::vector<..*>::iterator since that's an implementation detail.

[Nicolas Zea, 2013-08-09 20:04:13]

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:208:
break;
it seems to me this is the same as the code above, perhaps merge the two?

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:223:
NOTREACHED();
On 2013/08/09 19:40:28, dewittj (OOO until Aug 20) wrote:
> You don't handle ACTION_INVALID, yet you have a NOTREACHED here.  I suspect
> you'd want to handle that rather than crash the browser in the event of an
> invalid action.

The syncer will never return ACTION_INVALID in a valid scenario, and if it does
something is very broken. NOTREACHED would help us identify and fix the issue
quicker (and keep in mind only CHECK's in debug builds).

[Pete Williamson, 2013-08-09 21:00:18]

CR fixes per DewittJ and Zea

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:208:
break;
On 2013/08/09 20:04:13, Nicolas Zea wrote:
> it seems to me this is the same as the code above, perhaps merge the two?

Done.  I've heard before that it is worse to fall through than to duplicate
code, which is why I Originally didn"t do a fall through here.  See if you like
this better.

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:223:
NOTREACHED();
On 2013/08/09 20:04:13, Nicolas Zea wrote:
> On 2013/08/09 19:40:28, dewittj (OOO until Aug 20) wrote:
> > You don't handle ACTION_INVALID, yet you have a NOTREACHED here.  I suspect
> > you'd want to handle that rather than crash the browser in the event of an
> > invalid action.
> 
> The syncer will never return ACTION_INVALID in a valid scenario, and if it
does
> something is very broken. NOTREACHED would help us identify and fix the issue
> quicker (and keep in mind only CHECK's in debug builds).

Left as is.

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:223:
NOTREACHED();
On 2013/08/09 19:40:28, dewittj (OOO until Aug 20) wrote:
> You don't handle ACTION_INVALID, yet you have a NOTREACHED here.  I suspect
> you'd want to handle that rather than crash the browser in the event of an
> invalid action.

Left as is per Zea@'s suggestion (below)

https://codereview.chromium.org/22470006/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:321:
notification_data_.begin();
On 2013/08/09 19:40:28, dewittj (OOO until Aug 20) wrote:
> This indentation is really weird here.  Can I recommend:
> 
> ScopedVector<SyncedNotification>::iterator it = notification_data_.begin();
> for (; it != notification_data_.end(); ++it) { ... }
> 
> In fact, I think (unless my c++ fu is failing me here) that you should use
> ScopedVector<SyncedNotification>::iterator everywhere in this file instead of
> std::vector<..*>::iterator since that's an implementation detail.

Done.

[dewittj, 2013-08-09 21:17:58]

lgtm

[Nicolas Zea, 2013-08-09 21:33:47]

lgtm

[commit-bot: I haz the power, 2013-08-09 21:46:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/22470006/18001

[commit-bot: I haz the power, 2013-08-10 04:54:11]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 04:59:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/22470006/18001

[commit-bot: I haz the power, 2013-08-10 08:33:07]

Change committed as 216831