Re: Pushing forward on blink::initialize() and MessageLoop issue (https://codereview.chromium.org/2402983002/)

[Kentaro Hara]

I'm sorry for blocking you on this issue for a long time. Let me ask chromium-mojo@ for some advice.

Summary:

Leon is working on this CL. The problem is that:

- We want to initialize Mojo for TimeZoneMonitor in blink::initialize(). To initialize Mojo, we need a message loop.

- In production builds, we have a message loop when blink::initialize() is called. However, in unit tests, we don't have a message loop when blink::initialize() is called for the reason described in this comment. So we cannot initialize Mojo in blink::initialize().

- An ideal fix might be to fix unittests so that they initialize the message loop before calling blink::initialize(), but there're a lot of unittests.

Any idea?

On Tue, Oct 25, 2016 at 10:49 AM, Han, Leon <leon...@intel.com> wrote:

The current implementation of this CL is based on option (1), and, to enable those unit tests to initialize mojo stuff(currently only TimeZoneMonitorClient) lately after their own message loop is ready, introduced a new blink API:

initializeMojoForTest

Also, I added detailed comment along with its declaration at:

https://codereview.chromium.org/2402983002/diff/340001/third_party/WebKit/public/web/WebKit.h

 

Would you PTAnL and also give some suggestions about how to add some comments to clarify our situation well ;-) Thanks.

 

BR,

Han Leon

 

From: har...@google.com [mailto:har...@google.com] On Behalf Of Kentaro Hara
Sent: Tuesday, October 25, 2016 3:13 PM
To: Han, Leon <leon...@intel.com>
Cc: Colin Blundell <blun...@google.com>; siddhartha sivakumar <ss...@chromium.org>; Colin Blundell <blun...@chromium.org>

Subject: Re: Pushing forward on blink::initialize() and MessageLoop issue (https://codereview.chromium.org/2402983002/)

 

Thanks. Then colin's proposal makes sense.

 

I'm fine with either of (1) and (2). If we have a good comment, (1) sounds fine.

 

A long term fix would be to make TestBlinkWebUnitTestSupport create a message loop.

 

 

 

 

On Tue, Oct 25, 2016 at 4:21 AM, Han, Leon <leon...@intel.com> wrote:

From my investigation effort to try to create a message loop before calling blink::initialize() in TestBlinkWebUnitTestSupport, I think 'some tests' means all tests under the 4 test suites 'unit_tests, content_unittests, components_unittests and extensions_unittests' that would create their own message loop when they start to run, seems hundreds of them.
 
BR,
Han Leon

---

From: har...@google.com [har...@google.com] on behalf of Kentaro Hara [har...@chromium.org]
Sent: Tuesday, October 25, 2016 2:33 AM
To: Colin Blundell; siddhartha sivakumar
Cc: Colin Blundell; Han, Leon
Subject: Re: Pushing forward on blink::initialize() and MessageLoop issue (https://codereview.chromium.org/2402983002/)

My understanding from this comment is that it wouldn't work with the unittests that run in these suites.

 

ssid@: Do you know what the "some tests" in the comment refers to?

 

It looks weird to initialize with a dummy message loop and then switch to a real message loop...

 

 

On Mon, Oct 24, 2016 at 8:26 PM, Colin Blundell <blun...@google.com> wrote:

My understanding from this comment is that it wouldn't work with the unittests that run in these suites.

 

On Mon, Oct 24, 2016 at 11:23 AM Kentaro Hara <har...@chromium.org> wrote:

Is it hard to create a message loop in TestBlinkWebUnitTestSupport's constructor before calling blink::initialize()?

 

 

On Mon, Oct 24, 2016 at 7:56 PM, Colin Blundell <blun...@chromium.org> wrote:

Hi Kentaro and Han Leon,

 

I spent a little bit of time looking at this issue this morning. Here's the key fact from my perspective:

 

- In the codebase currently, the TimeZoneMonitor connection is set up in RenderThreadImpl::Init(), which also calls blink::initialize(). This codepath is not used by the test suites that use test_blink_web_unit_test_support.cc (since that class itself calls blink::initialize()).

 

This means that by definition the TimeZoneMonitor connection is not used in any of those tests.

 

Given (a) that fact and (b) the fact that it seems challenging to refactor all the unittests such that test_blink_web_unit_test_support.cc could itself set up a MessageLoop before calling blink::initialize(), I propose that we simply avoid creating TimeZoneMonitorClient in blink::initialize() if there is no current MessageLoop. There are two ways to do this that I see:

 

(1) Simply guard the creation in blink::initialize() behind a check for the current MessageLoop, with the comment that blink::initialize() is called without a current MessageLoop in some unitttest contexts.

 

(2) Add a blink::initializeWithoutMessageLoopForTesting() entrypoint. Have blink::initialize() and blink::initializeWithoutMessageLoopForTesting() both forward to an internal blink::initializeImpl() method that takes in a bool for whether it should expect there to be a MessageLoop or not.

 

The advantage of (1) is that it doesn't introduce any interface changes. The advantage of (2) is that it ensures that clients have to explicitly think about whether they're OK with initializing Blink without a current Message Loop, and in particular, any failure to initialize Blink with a MessageLoop in production code will be a loud failure instead of a silent failure. Personally, I prefer (2) for that reason.

 

Thoughts?

 

Thanks,

 

Colin

 

--

Kentaro Hara, Tokyo, Japan

 

--

Kentaro Hara, Tokyo, Japan

 

--

Kentaro Hara, Tokyo, Japan

--

Kentaro Hara, Tokyo, Japan

[Colin Blundell]

I think that at this time we can just avoid making these Mojo connections in unittests, i.e., not introduce initializeMojoForTest()). As I noted upthread, the TimeZoneMonitor connection was not previously made in these unittests, since it was done in RenderThreadImpl::Init(), which is not called in these unittests.

Any unittest that's testing specific Mojo functionality in Blink can just set up the specific Mojo connection that it needs after itself setting up a message loop (e.g., a TimeZoneMonitorClient test could explicitly initialize TimeZoneMonitorClient).

That seems like it should unblock the current migration of TimeZoneMonitor into Blink without losing any current functionality wrt these tests.

We can revisit if for some reason there's a need later to have the Mojo connections implicitly made across a broad range of unittests, but I'm not sure why that would be the case given that they're unittests.

[Kentaro Hara]

Colin: Ah, sorry! I was misunderstanding your suggestion.

On Tue, Oct 25, 2016 at 3:19 PM, Colin Blundell <blun...@chromium.org> wrote:

I think that at this time we can just avoid making these Mojo connections in unittests, i.e., not introduce initializeMojoForTest()). As I noted upthread, the TimeZoneMonitor connection was not previously made in these unittests, since it was done in RenderThreadImpl::Init(), which is not called in these unittests.

Yes, this will work at least in short term and unblock the CL.

 

Any unittest that's testing specific Mojo functionality in Blink can just set up the specific Mojo connection that it needs after itself setting up a message loop (e.g., a TimeZoneMonitorClient test could explicitly initialize TimeZoneMonitorClient).

That seems like it should unblock the current migration of TimeZoneMonitor into Blink without losing any current functionality wrt these tests.

We can revisit if for some reason there's a need later to have the Mojo connections implicitly made across a broad range of unittests, but I'm not sure why that would be the case given that they're unittests.

Totally agreed :)

 

--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-mojo+unsubscribe@chromium.org.
To post to this group, send email to chromi...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAMGE5NFNd7-_rWtwmJiOTCkQwr9%3DshzAUYwQH6ZXDbs_6VvbJg%40mail.gmail.com.