Initialize the message center as part of the testing browser process

Initialize the message center as part of the testing browser process

This allows us to remove a bunch of unnecessary initialize calls
More importantly it allows us to remove test specific logic
(see platform_notification_service_impl.cc) that is currently
preventing us from testing those methods.

This involves several other changes
  some (but not all) ash tests depend on message center but not on the
  testing browser process, the messsage center is initialized in the
  test suite for those tests.

  The browser process test uses both the testing and the real process..

  A few tests were failing now that the message center lingers for longer
  They were fixed by re-ordering the tear down sequence.

BUG=

[Miguel Garcia, 2016-11-24 16:21:59]

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

[commit-bot: I haz the power, 2016-11-24 16:22:31]

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

[Miguel Garcia, 2016-11-24 16:37:30]

miguelg@chromium.org changed reviewers:
+ peter@chromium.org, sky@chromium.org

[Miguel Garcia, 2016-11-24 16:37:30]

sky: You happen to be a main owner and also an ash/ owner so I thought you could
be the main reviewer and provide feedback on the overall approach.

I am especially interested in the following aspects
  - whether this change makes sense in the big scheme of things
  - whether the approach taken in browser_process_impl.cc to overcome the double
initialisation makes sense
  - whether the changes in ash/ are fine

Peter: Can you sign of on the notification changes?

Jochen: I cced you in case you can provide some early output since everyone in
the US is probably out. Just want a feeling on whether this is a good idea or
not.

[Miguel Garcia, 2016-11-24 16:38:29]

On 2016/11/24 16:37:30, Miguel Garcia wrote:
> sky: You happen to be a main owner and also an ash/ owner so I thought you
could
> be the main reviewer and provide feedback on the overall approach.
> 
> I am especially interested in the following aspects
>   - whether this change makes sense in the big scheme of things
>   - whether the approach taken in browser_process_impl.cc to overcome the
double
> initialisation makes sense
>   - whether the changes in ash/ are fine
> 
> Peter: Can you sign of on the notification changes?
> 
> Jochen: I cced you in case you can provide some early output since everyone in
> the US is probably out. Just want a feeling on whether this is a good idea or
> not.

And.. some of the android tests are broken due to the new way we return null for
notification ui manager if there is no browser process I will fix those once I
get my hands on an Android device.

[commit-bot: I haz the power, 2016-11-24 17:00:34]

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

[commit-bot: I haz the power, 2016-11-24 17:00:35]

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

[sky, 2016-11-26 17:10:53]

It doesn't look like the upload succeeded (some files show missing patch).
Please upload again.

[Miguel Garcia, 2016-11-28 10:47:23]

On 2016/11/26 17:10:53, sky wrote:
> It doesn't look like the upload succeeded (some files show missing patch).
> Please upload again.

The upload process keeps failing, I believe this is a known issue. I have tried
several times without success. Since it seems that most of the critical files
are there perhaps this is not blocking an initial review? In any case I'll ping
this review once I've managed to upload it.

[Miguel Garcia, 2016-11-28 10:59:49]

On 2016/11/28 10:47:23, Miguel Garcia wrote:
> On 2016/11/26 17:10:53, sky wrote:
> > It doesn't look like the upload succeeded (some files show missing patch).
> > Please upload again.
> 
> The upload process keeps failing, I believe this is a known issue. I have
tried
> several times without success. Since it seems that most of the critical files
> are there perhaps this is not blocking an initial review? In any case I'll
ping
> this review once I've managed to upload it.

https://bugs.chromium.org/p/chromium/issues/detail?id=668526 for context on the
upload issue

[sky, 2016-11-28 16:17:43]

I have a preference for keeping what is run by all tests to a minimum. In other
words, tests that need message center can initialize it. Is there a reason that
doesn't work well?

[Miguel Garcia, 2016-11-28 16:33:52]

On 2016/11/28 16:17:43, sky wrote:
> I have a preference for keeping what is run by all tests to a minimum. In
other
> words, tests that need message center can initialize it. Is there a reason
that
> doesn't work well?

So in general I agree with you but for better or worse the message center is a
global object so it's accessed from random places all over the code base, so
sometimes you are adding a completely unrelated change and suddenly test starts
failing because it's missing, or double initialized.

A concrete example I hit:

I would like to avoid checks like 
https://cs.chromium.org/chromium/src/chrome/browser/notifications/platform_no...
where in the middle of our production code we check if the profile is a testing
profile.

This is needed today because if we actually execute the method all the tests
that don't have a message center fail.

I am changing the logic of that method (in
https://codereview.chromium.org/2534443002/patch/40001/50003) and as such would
like to add a unit test there but in the unit test the profile will be a testing
profile so I would have to work around the work around. I just thought it would
be better to provide the message center the same life cycle expectations in the
testing browser process than in the real browser process.

[sky, 2016-11-28 17:36:25]

Just because an object is a global object doesn't mean we have to
initialize it for tests too. It just means code needs to be written to
deal with the possibility of the object being null. What is unique
about message center that means code can't check if it's null?

I do agree the profile check is awful. Should the code not assume
GetNotificationDisplayService() returns a non-null?

  -Scott

On Mon, Nov 28, 2016 at 8:33 AM,  <miguelg@chromium.org> wrote:
> On 2016/11/28 16:17:43, sky wrote:
>> I have a preference for keeping what is run by all tests to a minimum. In
> other
>> words, tests that need message center can initialize it. Is there a reason
> that
>> doesn't work well?
>
> So in general I agree with you but for better or worse the message center is
> a
> global object so it's accessed from random places all over the code base, so
> sometimes you are adding a completely unrelated change and suddenly test
> starts
> failing because it's missing, or double initialized.
>
> A concrete example I hit:
>
> I would like to avoid checks like
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/platform_no...
> where in the middle of our production code we check if the profile is a
> testing
> profile.
>
> This is needed today because if we actually execute the method all the tests
> that don't have a message center fail.
>
> I am changing the logic of that method (in
> https://codereview.chromium.org/2534443002/patch/40001/50003) and as such
> would
> like to add a unit test there but in the unit test the profile will be a
> testing
> profile so I would have to work around the work around. I just thought it
> would
> be better to provide the message center the same life cycle expectations in
> the
> testing browser process than in the real browser process.
>
> https://codereview.chromium.org/2528013002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miguel Garcia, 2016-11-29 13:14:10]

That's how I started but I found out so many places that assume the message
center exists (because they read it from g_browser_process->message_center()
which is in fact always available in production code) that I thought it was
assumed it should be.

I will try a more local change to get rid of this specific problem if this is
not acceptable.



On 2016/11/28 17:36:25, sky wrote:
> Just because an object is a global object doesn't mean we have to
> initialize it for tests too. It just means code needs to be written to
> deal with the possibility of the object being null. What is unique
> about message center that means code can't check if it's null?
> 
> I do agree the profile check is awful. Should the code not assume
> GetNotificationDisplayService() returns a non-null?
> 
>   -Scott
> 
> On Mon, Nov 28, 2016 at 8:33 AM,  <mailto:miguelg@chromium.org> wrote:
> > On 2016/11/28 16:17:43, sky wrote:
> >> I have a preference for keeping what is run by all tests to a minimum. In
> > other
> >> words, tests that need message center can initialize it. Is there a reason
> > that
> >> doesn't work well?
> >
> > So in general I agree with you but for better or worse the message center is
> > a
> > global object so it's accessed from random places all over the code base, so
> > sometimes you are adding a completely unrelated change and suddenly test
> > starts
> > failing because it's missing, or double initialized.
> >
> > A concrete example I hit:
> >
> > I would like to avoid checks like
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/platform_no...
> > where in the middle of our production code we check if the profile is a
> > testing
> > profile.
> >
> > This is needed today because if we actually execute the method all the tests
> > that don't have a message center fail.
> >
> > I am changing the logic of that method (in
> > https://codereview.chromium.org/2534443002/patch/40001/50003) and as such
> > would
> > like to add a unit test there but in the unit test the profile will be a
> > testing
> > profile so I would have to work around the work around. I just thought it
> > would
> > be better to provide the message center the same life cycle expectations in
> > the
> > testing browser process than in the real browser process.
> >
> > https://codereview.chromium.org/2528013002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-11-29 18:12:46]

Thank you!

On Tue, Nov 29, 2016 at 5:14 AM,  <miguelg@chromium.org> wrote:
> That's how I started but I found out so many places that assume the message
> center exists (because they read it from g_browser_process->message_center()
> which is in fact always available in production code) that I thought it was
> assumed it should be.
>
> I will try a more local change to get rid of this specific problem if this
> is
> not acceptable.
>
>
>
> On 2016/11/28 17:36:25, sky wrote:
>> Just because an object is a global object doesn't mean we have to
>> initialize it for tests too. It just means code needs to be written to
>> deal with the possibility of the object being null. What is unique
>> about message center that means code can't check if it's null?
>>
>> I do agree the profile check is awful. Should the code not assume
>> GetNotificationDisplayService() returns a non-null?
>>
>> -Scott
>>
>> On Mon, Nov 28, 2016 at 8:33 AM, <mailto:miguelg@chromium.org> wrote:
>> > On 2016/11/28 16:17:43, sky wrote:
>> >> I have a preference for keeping what is run by all tests to a minimum.
>> >> In
>> > other
>> >> words, tests that need message center can initialize it. Is there a
>> >> reason
>> > that
>> >> doesn't work well?
>> >
>> > So in general I agree with you but for better or worse the message
>> > center is
>> > a
>> > global object so it's accessed from random places all over the code
>> > base, so
>> > sometimes you are adding a completely unrelated change and suddenly test
>> > starts
>> > failing because it's missing, or double initialized.
>> >
>> > A concrete example I hit:
>> >
>> > I would like to avoid checks like
>> >
>>
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/platform_no...
>> > where in the middle of our production code we check if the profile is a
>> > testing
>> > profile.
>> >
>> > This is needed today because if we actually execute the method all the
>> > tests
>> > that don't have a message center fail.
>> >
>> > I am changing the logic of that method (in
>> > https://codereview.chromium.org/2534443002/patch/40001/50003) and as
>> > such
>> > would
>> > like to add a unit test there but in the unit test the profile will be a
>> > testing
>> > profile so I would have to work around the work around. I just thought
>> > it
>> > would
>> > be better to provide the message center the same life cycle expectations
>> > in
>> > the
>> > testing browser process than in the real browser process.
>> >
>> > https://codereview.chromium.org/2528013002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/2528013002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miguel Garcia, 2016-11-30 13:32:14]

One final thought that I forgot, right now  g_browser_process->message_center()
DCHECKs if the message center has not been initialized. Would it be ok to add a
IsInitialized() method to message_center to overcome this problem?


On 2016/11/29 18:12:46, sky wrote:
> Thank you!
> 
> On Tue, Nov 29, 2016 at 5:14 AM,  <mailto:miguelg@chromium.org> wrote:
> > That's how I started but I found out so many places that assume the message
> > center exists (because they read it from g_browser_process->message_center()
> > which is in fact always available in production code) that I thought it was
> > assumed it should be.
> >
> > I will try a more local change to get rid of this specific problem if this
> > is
> > not acceptable.
> >
> >
> >
> > On 2016/11/28 17:36:25, sky wrote:
> >> Just because an object is a global object doesn't mean we have to
> >> initialize it for tests too. It just means code needs to be written to
> >> deal with the possibility of the object being null. What is unique
> >> about message center that means code can't check if it's null?
> >>
> >> I do agree the profile check is awful. Should the code not assume
> >> GetNotificationDisplayService() returns a non-null?
> >>
> >> -Scott
> >>
> >> On Mon, Nov 28, 2016 at 8:33 AM, <mailto:miguelg@chromium.org> wrote:
> >> > On 2016/11/28 16:17:43, sky wrote:
> >> >> I have a preference for keeping what is run by all tests to a minimum.
> >> >> In
> >> > other
> >> >> words, tests that need message center can initialize it. Is there a
> >> >> reason
> >> > that
> >> >> doesn't work well?
> >> >
> >> > So in general I agree with you but for better or worse the message
> >> > center is
> >> > a
> >> > global object so it's accessed from random places all over the code
> >> > base, so
> >> > sometimes you are adding a completely unrelated change and suddenly test
> >> > starts
> >> > failing because it's missing, or double initialized.
> >> >
> >> > A concrete example I hit:
> >> >
> >> > I would like to avoid checks like
> >> >
> >>
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/platform_no...
> >> > where in the middle of our production code we check if the profile is a
> >> > testing
> >> > profile.
> >> >
> >> > This is needed today because if we actually execute the method all the
> >> > tests
> >> > that don't have a message center fail.
> >> >
> >> > I am changing the logic of that method (in
> >> > https://codereview.chromium.org/2534443002/patch/40001/50003) and as
> >> > such
> >> > would
> >> > like to add a unit test there but in the unit test the profile will be a
> >> > testing
> >> > profile so I would have to work around the work around. I just thought
> >> > it
> >> > would
> >> > be better to provide the message center the same life cycle expectations
> >> > in
> >> > the
> >> > testing browser process than in the real browser process.
> >> >
> >> > https://codereview.chromium.org/2528013002/
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups
> >> "Chromium-reviews" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an
> > email
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> >
> > https://codereview.chromium.org/2528013002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-11-30 16:28:33]

It's BrowserProcessImpl's a straight call through:

message_center::MessageCenter* BrowserProcessImpl::message_center() {
  DCHECK(CalledOnValidThread());
  return message_center::MessageCenter::Get();
}

Maybe you're saying you want MessageCenter::Get() to now have a
DCHECK? I'm not familiar enough with the message center code to know
the ramifications of that.

  -Scott

On Wed, Nov 30, 2016 at 5:32 AM,  <miguelg@chromium.org> wrote:
> One final thought that I forgot, right now
> g_browser_process->message_center()
> DCHECKs if the message center has not been initialized. Would it be ok to
> add a
> IsInitialized() method to message_center to overcome this problem?
>
>
> On 2016/11/29 18:12:46, sky wrote:
>> Thank you!
>>
>> On Tue, Nov 29, 2016 at 5:14 AM, <mailto:miguelg@chromium.org> wrote:
>> > That's how I started but I found out so many places that assume the
>> > message
>> > center exists (because they read it from
>> > g_browser_process->message_center()
>> > which is in fact always available in production code) that I thought it
>> > was
>> > assumed it should be.
>> >
>> > I will try a more local change to get rid of this specific problem if
>> > this
>> > is
>> > not acceptable.
>> >
>> >
>> >
>> > On 2016/11/28 17:36:25, sky wrote:
>> >> Just because an object is a global object doesn't mean we have to
>> >> initialize it for tests too. It just means code needs to be written to
>> >> deal with the possibility of the object being null. What is unique
>> >> about message center that means code can't check if it's null?
>> >>
>> >> I do agree the profile check is awful. Should the code not assume
>> >> GetNotificationDisplayService() returns a non-null?
>> >>
>> >> -Scott
>> >>
>> >> On Mon, Nov 28, 2016 at 8:33 AM, <mailto:miguelg@chromium.org> wrote:
>> >> > On 2016/11/28 16:17:43, sky wrote:
>> >> >> I have a preference for keeping what is run by all tests to a
>> >> >> minimum.
>> >> >> In
>> >> > other
>> >> >> words, tests that need message center can initialize it. Is there a
>> >> >> reason
>> >> > that
>> >> >> doesn't work well?
>> >> >
>> >> > So in general I agree with you but for better or worse the message
>> >> > center is
>> >> > a
>> >> > global object so it's accessed from random places all over the code
>> >> > base, so
>> >> > sometimes you are adding a completely unrelated change and suddenly
>> >> > test
>> >> > starts
>> >> > failing because it's missing, or double initialized.
>> >> >
>> >> > A concrete example I hit:
>> >> >
>> >> > I would like to avoid checks like
>> >> >
>> >>
>> >
>>
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/platform_no...
>> >> > where in the middle of our production code we check if the profile is
>> >> > a
>> >> > testing
>> >> > profile.
>> >> >
>> >> > This is needed today because if we actually execute the method all
>> >> > the
>> >> > tests
>> >> > that don't have a message center fail.
>> >> >
>> >> > I am changing the logic of that method (in
>> >> > https://codereview.chromium.org/2534443002/patch/40001/50003) and as
>> >> > such
>> >> > would
>> >> > like to add a unit test there but in the unit test the profile will
>> >> > be a
>> >> > testing
>> >> > profile so I would have to work around the work around. I just
>> >> > thought
>> >> > it
>> >> > would
>> >> > be better to provide the message center the same life cycle
>> >> > expectations
>> >> > in
>> >> > the
>> >> > testing browser process than in the real browser process.
>> >> >
>> >> > https://codereview.chromium.org/2528013002/
>> >>
>> >> --
>> >> You received this message because you are subscribed to the Google
>> >> Groups
>> >> "Chromium-reviews" group.
>> >> To unsubscribe from this group and stop receiving emails from it, send
>> >> an
>> > email
>> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >>
>> >
>> >
>> >
>> > https://codereview.chromium.org/2528013002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/2528013002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.