Allow ObserverListThreadSafe to be used from sequenced tasks.

Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org,melandory@chromium.org,bengr@chromium.org,sfiera@chromium.org

Review-Url: https://codereview.chromium.org/2592143003
Cr-Commit-Position: refs/heads/master@{#447311}
Committed: https://chromium.googlesource.com/chromium/src/+/5062447d399be019735314f765d8d21b9433d995

[fdoray, 2016-12-21 18:52:04]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

This CL does not change the behavior of ObserverListThreadSafe when
used from single-threaded tasks.

BUG=675631
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks.
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- There is one notification task per observer rather than one
  notification task for all observers registered from the same thread.
- Notifications are not dispatched in nested loops.

BUG=675631
==========

[fdoray, 2016-12-21 19:08:59]

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

[commit-bot: I haz the power, 2016-12-21 19:09:41]

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, 2016-12-21 19:17:49]

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

[commit-bot: I haz the power, 2016-12-21 19:17:50]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fdoray, 2016-12-21 19:27:24]

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

[commit-bot: I haz the power, 2016-12-21 19:28:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2016-12-21 19:30:33]

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

[commit-bot: I haz the power, 2016-12-21 19:31:22]

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, 2016-12-21 19:40:43]

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

[commit-bot: I haz the power, 2016-12-21 19:40:44]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[fdoray, 2016-12-23 20:00:44]

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

[commit-bot: I haz the power, 2016-12-23 20:00:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2016-12-23 20:02:06]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks.
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- There is one notification task per observer rather than one
  notification task for all observers registered from the same thread.
- Notifications are not dispatched in nested loops.

BUG=675631
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- There is one notification task per observer rather than one
  notification task for all observers registered from the same thread.
- Notifications are not dispatched in nested loops.

BUG=675631
==========

[fdoray, 2016-12-23 20:02:51]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- There is one notification task per observer rather than one
  notification task for all observers registered from the same thread.
- Notifications are not dispatched in nested loops.

BUG=675631
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- Notifications are not dispatched in nested loops.

BUG=675631
==========

[fdoray, 2016-12-23 20:03:14]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- Notifications are not dispatched in nested loops.

BUG=675631
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- Notifications can no longer be dispatched in nested loops.

BUG=675631
==========

[fdoray, 2016-12-23 20:04:09]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org

[fdoray, 2016-12-23 20:04:09]

PTAL

[commit-bot: I haz the power, 2016-12-23 20:11:53]

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

[commit-bot: I haz the power, 2016-12-23 20:11:54]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[gab, 2017-01-05 18:45:51]

Makes browser_tests unhappy, should I review or wait for fix?

[fdoray, 2017-01-05 19:37:21]

On 2017/01/05 18:45:51, gab wrote:
> Makes browser_tests unhappy, should I review or wait for fix?

You should wait. I'll take a look at test failures.

[fdoray, 2017-01-05 19:37:49]

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

[commit-bot: I haz the power, 2017-01-05 19:38: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-01-05 19:46:21]

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

[commit-bot: I haz the power, 2017-01-05 19:46:21]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[fdoray, 2017-01-06 18:28:59]

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

[commit-bot: I haz the power, 2017-01-06 18:29:22]

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-01-06 18:39:51]

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

[commit-bot: I haz the power, 2017-01-06 18:39:52]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[fdoray, 2017-01-06 22:30:25]

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

[commit-bot: I haz the power, 2017-01-06 22:31:13]

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-01-06 22:45:32]

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

[commit-bot: I haz the power, 2017-01-06 22:45:33]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[fdoray, 2017-01-09 21:52:36]

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

[commit-bot: I haz the power, 2017-01-09 21:53:20]

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-01-09 22:05:10]

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

[commit-bot: I haz the power, 2017-01-09 22:05:11]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[fdoray, 2017-01-10 13:40:49]

gab@: PTAL

I have pending CLs to fix test failures:
- https://codereview.chromium.org/2592143003/
- https://codereview.chromium.org/2627523003/

[gab, 2017-01-10 19:27:50]

On 2017/01/10 13:40:49, fdoray wrote:
> gab@: PTAL

Looking

> 
> I have pending CLs to fix test failures:
> - https://codereview.chromium.org/2592143003/

That link points to this CL :)

> - https://codereview.chromium.org/2627523003/

Cool, found some real bugs :)!

[fdoray, 2017-01-10 19:36:30]

On 2017/01/10 19:27:50, gab wrote:
> On 2017/01/10 13:40:49, fdoray wrote:
> > gab@: PTAL
> 
> Looking
> 
> > 
> > I have pending CLs to fix test failures:
> > - https://codereview.chromium.org/2592143003/

oops https://codereview.chromium.org/2621743004/

> 
> That link points to this CL :)
> 
> > - https://codereview.chromium.org/2627523003/
> 
> Cool, found some real bugs :)!

[gab, 2017-01-10 21:14:50]

Top-level comment on |current_notifications_|, I don't think it works as-is.

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:19: // depend on its presence here).
Move it out of regular include block with comment like
https://codereview.chromium.org/2597773002/diff/60001/base/supports_user_data...

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:92: // NOTIFY_ALL, |observer| must be notified.
Hmmm, see comment on |current_notifications_| but I don't think this is what you
want. What you want is to make sure the added observer will be notified if its
addition has an happens-before relation with the notify.

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:139: observer.second->PostNonNestableTask(
Why add non-nestable?

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:202: std::unordered_map<PlatformThreadId,
NotificationData> current_notifications_;
Seems like this should use TLS? (and as such not require synchronization to
access)

Which then makes me think I don't think it can trigger (i.e. it's
inserted/removed in a scope on the same thread on which it's latter checked to
be non-empty?)

https://codereview.chromium.org/2592143003/diff/140001/net/spdy/spdy_session_...
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/2592143003/diff/140001/net/spdy/spdy_session_...
net/spdy/spdy_session_pool.cc:10: #include "base/memory/ptr_util.h"
Fix includes in another CL?

[fdoray, 2017-01-11 13:58:36]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- Notifications can no longer be dispatched in nested loops.

BUG=675631
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
==========

[fdoray, 2017-01-11 14:09:06]

PTAnL

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:19: // depend on its presence here).
On 2017/01/10 21:14:49, gab wrote:
> Move it out of regular include block with comment like
>
https://codereview.chromium.org/2597773002/diff/60001/base/supports_user_data...

Done.

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:92: // NOTIFY_ALL, |observer| must be notified.
On 2017/01/10 21:14:49, gab wrote:
> Hmmm, see comment on |current_notifications_| but I don't think this is what
you
> want. What you want is to make sure the added observer will be notified if its
> addition has an happens-before relation with the notify.

https://cs.chromium.org/chromium/src/base/observer_list.h?l=77
// Specifies that any observers added ***during notification*** are notified.
// This is the default type if non type is provided to the constructor.
NOTIFY_ALL,

If AddObserver() is called on a NOTIFY_ALL ObserverListThreadSafe from a
notification, the new observer must be notified.

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:139: observer.second->PostNonNestableTask(
On 2017/01/10 21:14:49, gab wrote:
> Why add non-nestable?

When |type_| is NOTIFY_ALL, an observer added during a notification must receive
the notification itself. I used PostNonNestableTask() because I wasn't
particularly interested in handling adding an observer from a notification
dispatched from within a notification dispatched from within a notification,
etc.

In the latest patch set, I changed PostNonNestableTask to PostTask. When an
observer is added from a stack of notifications, it only receives the
notification on top of the stack.

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:202: std::unordered_map<PlatformThreadId,
NotificationData> current_notifications_;
On 2017/01/10 21:14:49, gab wrote:
> Seems like this should use TLS? (and as such not require synchronization to
> access)
> 
> Which then makes me think I don't think it can trigger (i.e. it's
> inserted/removed in a scope on the same thread on which it's latter checked to
> be non-empty?)

TLS works. Done.

https://codereview.chromium.org/2592143003/diff/140001/net/spdy/spdy_session_...
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/2592143003/diff/140001/net/spdy/spdy_session_...
net/spdy/spdy_session_pool.cc:10: #include "base/memory/ptr_util.h"
On 2017/01/10 21:14:50, gab wrote:
> Fix includes in another CL?

Done.

[gab, 2017-01-12 16:24:06]

lg thanks

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:139: observer.second->PostNonNestableTask(
On 2017/01/11 14:09:06, fdoray wrote:
> On 2017/01/10 21:14:49, gab wrote:
> > Why add non-nestable?
> 
> When |type_| is NOTIFY_ALL, an observer added during a notification must
receive
> the notification itself. I used PostNonNestableTask() because I wasn't
> particularly interested in handling adding an observer from a notification
> dispatched from within a notification dispatched from within a notification,
> etc.

Notify merely posts anyways so I don't think you can get nested notification
dispatches.

> 
> In the latest patch set, I changed PostNonNestableTask to PostTask. When an
> observer is added from a stack of notifications, it only receives the
> notification on top of the stack.

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:48: //   immediately.
- "and immediately" ? (synchronously implies immediately)

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:80: if (!SequencedTaskRunnerHandle::IsSet())
Seems this should be a DCHECK instead of handling it, doesn't make sense to
AddObserver if we can't handle it... (might break existing tests that don't use
a class' observer and never bothered to add a MessageLoop though... up to you if
you want to try it out but probably not in this CL)

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:91: // If this is called while a notification is
being dispatched and |type_| is
s/being dispatched/being dispatched on this thread/

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:92: // NOTIFY_ALL, |observer| must be notified.
+ "(if a notification is being dispatched on another thread in parallel, the
notification may or may not make it to |observer| depending on the outcome of
the race to |lock_|)"

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:176: tls_current_notification_.Get();
As mentioned in another comment, I don't think nesting is possible (the best
Run() can do in its scope is Notify or AddObserver which both generate a
PostTask, it won't synchronously get into NotifyWrapper()).

As such this is DCHECK(!tls_current_notification_.Get());

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_uni...
File base/observer_list_unittest.cc (left):

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_uni...
base/observer_list_unittest.cc:6: #include "base/observer_list_threadsafe.h"
Keep it up here I'd say (though it's weird to conflate testing of two cc's in
one unittest.cc but that's not worth changing in this CL)

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_uni...
File base/observer_list_unittest.cc (right):

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_uni...
base/observer_list_unittest.cc:557: using ObserverListType =
ObserverListThreadSafe<Foo>;
Don't think the using is worth it for two sites, it merely obscures types used
(same above)

[fdoray, 2017-01-12 17:17:57]

PTAnL

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:139: observer.second->PostNonNestableTask(
On 2017/01/12 16:24:05, gab wrote:
> On 2017/01/11 14:09:06, fdoray wrote:
> > On 2017/01/10 21:14:49, gab wrote:
> > > Why add non-nestable?
> > 
> > When |type_| is NOTIFY_ALL, an observer added during a notification must
> receive
> > the notification itself. I used PostNonNestableTask() because I wasn't
> > particularly interested in handling adding an observer from a notification
> > dispatched from within a notification dispatched from within a notification,
> > etc.
> 
> Notify merely posts anyways so I don't think you can get nested notification
> dispatches.

You can RunLoop().RunUntilIdle() from a notification. Another notification could
be dispatched within this RunLoop...

> 
> > 
> > In the latest patch set, I changed PostNonNestableTask to PostTask. When an
> > observer is added from a stack of notifications, it only receives the
> > notification on top of the stack.
>

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:48: //   immediately.
On 2017/01/12 16:24:05, gab wrote:
> - "and immediately" ? (synchronously implies immediately)

Done.

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:80: if (!SequencedTaskRunnerHandle::IsSet())
On 2017/01/12 16:24:06, gab wrote:
> Seems this should be a DCHECK instead of handling it, doesn't make sense to
> AddObserver if we can't handle it... (might break existing tests that don't
use
> a class' observer and never bothered to add a MessageLoop though... up to you
if
> you want to try it out but probably not in this CL)

Will do this in a separate CL since it breaks tests. Added TODO.

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:91: // If this is called while a notification is
being dispatched and |type_| is
On 2017/01/12 16:24:06, gab wrote:
> s/being dispatched/being dispatched on this thread/

Done.

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:92: // NOTIFY_ALL, |observer| must be notified.
On 2017/01/12 16:24:06, gab wrote:
> + "(if a notification is being dispatched on another thread in parallel, the
> notification may or may not make it to |observer| depending on the outcome of
> the race to |lock_|)"

Not sure this is true.

Thread / Action
1      / Calls Notify()
1      / Acquires |lock_|
1      / Starts posting notification tasks
2      / Calls AddObserver() to add |new_observer|
2      / Blocks while trying to acquire |lock_|
1      / Releases |lock_|
2      / Acquires |lock_|
2      / Adds |new_observer|
|new_observer| never gets the notification that was being dispatched while it
was added.

Other scenario:
Thread / Action
1      / Calls Notify()
1      / Acquires |lock_|
1      / Posts notification tasks
1      / Releases |lock_|
2      / Starts running a notification task
3      / Calls AddObserver() to add |new_observer|
3      / Acquires |lock_|
3      / Adds the observer
3      / Releases |lock_|
2      / Finishes to run the notification task
|new_observer| never gets the notification that was being dispatched while it
was added.

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:176: tls_current_notification_.Get();
On 2017/01/12 16:24:06, gab wrote:
> As mentioned in another comment, I don't think nesting is possible (the best
> Run() can do in its scope is Notify or AddObserver which both generate a
> PostTask, it won't synchronously get into NotifyWrapper()).
> 
> As such this is DCHECK(!tls_current_notification_.Get());

See previous comment in this file (a notification can create a RunLoop to run
nested tasks).

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_uni...
File base/observer_list_unittest.cc (left):

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_uni...
base/observer_list_unittest.cc:6: #include "base/observer_list_threadsafe.h"
On 2017/01/12 16:24:06, gab wrote:
> Keep it up here I'd say (though it's weird to conflate testing of two cc's in
> one unittest.cc but that's not worth changing in this CL)

Done.

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_uni...
File base/observer_list_unittest.cc (right):

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_uni...
base/observer_list_unittest.cc:557: using ObserverListType =
ObserverListThreadSafe<Foo>;
On 2017/01/12 16:24:06, gab wrote:
> Don't think the using is worth it for two sites, it merely obscures types used
> (same above)

Done.

[gab, 2017-01-12 20:12:15]

lgtm w/ comment, thanks!

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/140001/base/observer_list_thr...
base/observer_list_threadsafe.h:139: observer.second->PostNonNestableTask(
On 2017/01/12 17:17:57, fdoray wrote:
> On 2017/01/12 16:24:05, gab wrote:
> > On 2017/01/11 14:09:06, fdoray wrote:
> > > On 2017/01/10 21:14:49, gab wrote:
> > > > Why add non-nestable?
> > > 
> > > When |type_| is NOTIFY_ALL, an observer added during a notification must
> > receive
> > > the notification itself. I used PostNonNestableTask() because I wasn't
> > > particularly interested in handling adding an observer from a notification
> > > dispatched from within a notification dispatched from within a
notification,
> > > etc.
> > 
> > Notify merely posts anyways so I don't think you can get nested notification
> > dispatches.
> 
> You can RunLoop().RunUntilIdle() from a notification. Another notification
could
> be dispatched within this RunLoop...

Ewww, hadn't understood your use case for PostNonNestableTask before, now I
see... This should be very rare but we also don't have a good reason to force
non-nesting IMO and the solution is fairly self-contained so I guess it's okay.


> 
> > 
> > > 
> > > In the latest patch set, I changed PostNonNestableTask to PostTask. When
an
> > > observer is added from a stack of notifications, it only receives the
> > > notification on top of the stack.
> >

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:92: // NOTIFY_ALL, |observer| must be notified.
On 2017/01/12 17:17:57, fdoray wrote:
> On 2017/01/12 16:24:06, gab wrote:
> > + "(if a notification is being dispatched on another thread in parallel, the
> > notification may or may not make it to |observer| depending on the outcome
of
> > the race to |lock_|)"
> 
> Not sure this is true.
> 
> Thread / Action
> 1      / Calls Notify()
> 1      / Acquires |lock_|
> 1      / Starts posting notification tasks
> 2      / Calls AddObserver() to add |new_observer|
> 2      / Blocks while trying to acquire |lock_|
> 1      / Releases |lock_|
> 2      / Acquires |lock_|
> 2      / Adds |new_observer|
> |new_observer| never gets the notification that was being dispatched while it
> was added.
> 
> Other scenario:
> Thread / Action
> 1      / Calls Notify()
> 1      / Acquires |lock_|
> 1      / Posts notification tasks
> 1      / Releases |lock_|
> 2      / Starts running a notification task
> 3      / Calls AddObserver() to add |new_observer|
> 3      / Acquires |lock_|
> 3      / Adds the observer
> 3      / Releases |lock_|
> 2      / Finishes to run the notification task
> |new_observer| never gets the notification that was being dispatched while it
> was added.

But you also have:
1      / Calls Notify()
2      / Calls AddObserver() to add |new_observer|
2      / Acquires |lock_|
1      / Blocks while trying to acquire |lock_|
2      / Adds |new_observer|
2      / Releases |lock_|
1      / Acquires |lock_|
1      / Starts posting notification tasks
1      / Releases |lock_|
|new_observer| gets the notification that was being dispatched while it was
added

https://codereview.chromium.org/2592143003/diff/200001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/200001/base/observer_list_thr...
base/observer_list_threadsafe.h:175: // This will be used if the callback below
calls AddObserver().
Expand comment to explain the specific use case (nesting) in which this matters.

[fdoray, 2017-01-12 20:57:44]

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/180001/base/observer_list_thr...
base/observer_list_threadsafe.h:92: // NOTIFY_ALL, |observer| must be notified.
On 2017/01/12 20:12:14, gab wrote:
> On 2017/01/12 17:17:57, fdoray wrote:
> > On 2017/01/12 16:24:06, gab wrote:
> > > + "(if a notification is being dispatched on another thread in parallel,
the
> > > notification may or may not make it to |observer| depending on the outcome
> of
> > > the race to |lock_|)"
> > 
> > Not sure this is true.
> > 
> > Thread / Action
> > 1      / Calls Notify()
> > 1      / Acquires |lock_|
> > 1      / Starts posting notification tasks
> > 2      / Calls AddObserver() to add |new_observer|
> > 2      / Blocks while trying to acquire |lock_|
> > 1      / Releases |lock_|
> > 2      / Acquires |lock_|
> > 2      / Adds |new_observer|
> > |new_observer| never gets the notification that was being dispatched while
it
> > was added.
> > 
> > Other scenario:
> > Thread / Action
> > 1      / Calls Notify()
> > 1      / Acquires |lock_|
> > 1      / Posts notification tasks
> > 1      / Releases |lock_|
> > 2      / Starts running a notification task
> > 3      / Calls AddObserver() to add |new_observer|
> > 3      / Acquires |lock_|
> > 3      / Adds the observer
> > 3      / Releases |lock_|
> > 2      / Finishes to run the notification task
> > |new_observer| never gets the notification that was being dispatched while
it
> > was added.
> 
> But you also have:
> 1      / Calls Notify()
> 2      / Calls AddObserver() to add |new_observer|
> 2      / Acquires |lock_|
> 1      / Blocks while trying to acquire |lock_|
> 2      / Adds |new_observer|
> 2      / Releases |lock_|
> 1      / Acquires |lock_|
> 1      / Starts posting notification tasks
> 1      / Releases |lock_|
> |new_observer| gets the notification that was being dispatched while it was
> added

Done.

https://codereview.chromium.org/2592143003/diff/200001/base/observer_list_thr...
File base/observer_list_threadsafe.h (right):

https://codereview.chromium.org/2592143003/diff/200001/base/observer_list_thr...
base/observer_list_threadsafe.h:175: // This will be used if the callback below
calls AddObserver().
On 2017/01/12 20:12:14, gab wrote:
> Expand comment to explain the specific use case (nesting) in which this
matters.

Done.

[gab, 2017-01-13 16:52:15]

lgtm++ :)!

[fdoray, 2017-01-19 18:39:51]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-01-19 18:40:17]

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-01-19 18:49:31]

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

[commit-bot: I haz the power, 2017-01-19 18:49:32]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[fdoray, 2017-01-19 19:03:15]

fdoray@chromium.org changed reviewers:
+ rkaplow@chromium.org

[fdoray, 2017-01-19 19:03:16]

TBR rkaplow@ for adding missing include in network_metrics_provider.cc

[fdoray, 2017-01-19 19:03:30]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org
==========

[fdoray, 2017-01-19 19:03:37]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-19 19:03:38]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2592143003/#ps240001
(title: "add missing include")

[commit-bot: I haz the power, 2017-01-19 19:04:28]

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-01-19 19:16:04]

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

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

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fdoray, 2017-01-19 23:12:52]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-19 23:12:53]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2592143003/#ps260001
(title: "add missing include")

[commit-bot: I haz the power, 2017-01-19 23:13:55]

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-01-19 23:27:36]

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

[commit-bot: I haz the power, 2017-01-19 23:27:38]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fdoray, 2017-01-19 23:39:30]

fdoray@chromium.org changed reviewers:
+ melandory@chromium.org

[fdoray, 2017-01-19 23:39:31]

TBR melandory@chromium.org for missing include added to
components/password_manager/core/browser/form_fetcher_impl.cc

[fdoray, 2017-01-19 23:39:41]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org,melandory@chromium.org
==========

[fdoray, 2017-01-19 23:39:47]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-19 23:39:48]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2592143003/#ps280001
(title: "add missing include")

[commit-bot: I haz the power, 2017-01-19 23:40:48]

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-01-19 23:51:47]

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

[commit-bot: I haz the power, 2017-01-19 23:51:49]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[fdoray, 2017-01-20 00:03:21]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org,melandory@chromium.org
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org,melandory@chromium.org,bengr@chromium.org
==========

[fdoray, 2017-01-20 00:03:21]

fdoray@chromium.org changed reviewers:
+ bengr@chromium.org

[fdoray, 2017-01-20 00:03:57]

TBR bengr@chromium.org for missing include in
components/precache/content/precache_manager_unittest.cc

[fdoray, 2017-01-20 00:04:03]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-20 00:04:03]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2592143003/#ps300001
(title: "add missing include")

[commit-bot: I haz the power, 2017-01-20 00:05:22]

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-01-20 00:15:50]

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

[commit-bot: I haz the power, 2017-01-20 00:15:51]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[bengr, 2017-01-20 18:13:55]

lgtm

[rkaplow, 2017-01-23 15:25:27]

lgtm

[melandory, 2017-01-25 12:28:19]

lgtm

[melandory, 2017-01-25 12:28:20]

lgtm

[fdoray, 2017-01-25 15:19:35]

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

[commit-bot: I haz the power, 2017-01-25 15:19:55]

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-01-25 15:29:04]

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

[commit-bot: I haz the power, 2017-01-25 15:29:05]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fdoray, 2017-01-27 18:37:41]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-27 18:37:42]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
rkaplow@chromium.org, bengr@chromium.org, melandory@chromium.org
Link to the patchset: https://codereview.chromium.org/2592143003/#ps380001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-27 18:38:01]

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-01-27 18:49:43]

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

[commit-bot: I haz the power, 2017-01-27 18:49:45]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[fdoray, 2017-01-27 20:16:39]

fdoray@chromium.org changed reviewers:
+ sfiera@chromium.org

[fdoray, 2017-01-27 20:16:40]

TBR sfiera@chromium.org for missing include

[fdoray, 2017-01-27 20:16:51]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org,melandory@chromium.org,bengr@chromium.org
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org,melandory@chromium.org,bengr@chromium.org,sfiera@chr...
==========

[fdoray, 2017-01-27 20:16:58]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-27 20:16:58]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
melandory@chromium.org, rkaplow@chromium.org, bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2592143003/#ps400001
(title: "add missing include")

[commit-bot: I haz the power, 2017-01-27 20:17:44]

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-01-27 22:17:59]

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

[commit-bot: I haz the power, 2017-01-27 22:18:01]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sfiera, 2017-01-30 09:34:58]

LGTM for //components/ntp_tiles/...

[fdoray, 2017-01-30 16:01:20]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-30 16:01:20]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
melandory@chromium.org, rkaplow@chromium.org, bengr@chromium.org,
sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2592143003/#ps420001
(title: "exception for Android")

[commit-bot: I haz the power, 2017-01-30 16:01:34]

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-01-30 18:21:28]

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

[commit-bot: I haz the power, 2017-01-30 18:21:30]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fdoray, 2017-01-31 16:12:08]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-01-31 16:12:32]

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-01-31 18:35:21]

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

[commit-bot: I haz the power, 2017-01-31 18:35:23]

Failed to apply patch for components/ntp_tiles/most_visited_sites_unittest.cc:
While running git apply --index -p1;
  error: patch failed: components/ntp_tiles/most_visited_sites_unittest.cc:21
  error: components/ntp_tiles/most_visited_sites_unittest.cc: patch does not
apply

Patch:       components/ntp_tiles/most_visited_sites_unittest.cc
Index: components/ntp_tiles/most_visited_sites_unittest.cc
diff --git a/components/ntp_tiles/most_visited_sites_unittest.cc
b/components/ntp_tiles/most_visited_sites_unittest.cc
index
56f737de4d1fbded8b8bc42374d8afa88cfe43df..0e0fc6e2791830f9f70a90f21fff5443a26c91b4
100644
--- a/components/ntp_tiles/most_visited_sites_unittest.cc
+++ b/components/ntp_tiles/most_visited_sites_unittest.cc
@@ -21,6 +21,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/test/sequenced_worker_pool_owner.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "components/history/core/browser/top_sites.h"
 #include "components/ntp_tiles/icon_cacher.h"
 #include "components/ntp_tiles/json_unsafe_parser.h"

[fdoray, 2017-01-31 18:42:32]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-31 18:42:32]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
melandory@chromium.org, rkaplow@chromium.org, bengr@chromium.org,
sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2592143003/#ps440001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-31 18:43:06]

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-01-31 20:26:35]

CQ is committing da patch.

Bot data: {"patchset_id": 440001, "attempt_start_ts": 1485888152155870,
"parent_rev": "a2176d0c08e0a2367c91c2edb8ae255f185130ae", "commit_rev":
"5062447d399be019735314f765d8d21b9433d995"}

[commit-bot: I haz the power, 2017-01-31 20:27:13]

Description was changed from

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org,melandory@chromium.org,bengr@chromium.org,sfiera@chr...
==========

to

==========
Allow ObserverListThreadSafe to be used from sequenced tasks.

Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.

With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.

ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
  are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
  notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
  a stack of notification dispatches, only the notification on top of
  the stack is sent to the newly added observer.

BUG=675631
TBR=rkaplow@chromium.org,melandory@chromium.org,bengr@chromium.org,sfiera@chr...

Review-Url: https://codereview.chromium.org/2592143003
Cr-Commit-Position: refs/heads/master@{#447311}
Committed:
https://chromium.googlesource.com/chromium/src/+/5062447d399be019735314f765d8...
==========

[commit-bot: I haz the power, 2017-01-31 20:27:15]

Committed patchset #23 (id:440001) as
https://chromium.googlesource.com/chromium/src/+/5062447d399be019735314f765d8...

[tsergeant, 2017-02-01 00:46:26]

A revert of this CL (patchset #23 id:440001) has been created in
https://codereview.chromium.org/2669673002/ by tsergeant@chromium.org.

The reason for reverting is: This CL is causing tests to fail on Linux
ChromiumOS Tests (dbg)(1):

- ProfileBrowserTest.OffTheRecordURLRequestContextIsolation
- ProfileBrowserTest.URLRequestContextIsolation

See example failed build:

https://luci-milo.appspot.com/buildbot/chromium.chromiumos/Linux%20ChromiumOS....