Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2402983002:
[TimeZoneMonitor] Decouple renderer side impl from content to blink. (Closed)
|
Created:
4 years, 2 months ago by leonhsl(Using Gerrit) Modified:
4 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, shimazu+worker_chromium.org, falken, kinuko+worker_chromium.org, darin-cc_chromium.org, blink-reviews, horo+watch_chromium.org, abarth-chromium, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[TimeZoneMonitor] Decouple renderer side impl from content to blink.
In addition to the decoupling work, this CL:
* notifies time zone change to all the workerBackingThread, no matter they
do OWN their platform thread or not, while the old behavior is that only
those workerBackingThread OWNING a platform thread could get the
notifications, which is incorrect.
* will initialize TimeZoneMonitorClient in blink::initialize() if a message
loop has already been created, this is always true for production code,
while some unit tests may have not prepared the message loop when
calling blink::initialize(), so TimeZoneMonitorClient won't be initialized.
TBR=xhwang@chromium.org,rockot@chromium.org
BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
and then click "recheck" to ensure that the renderer picks up the new
time zone. Don't reload the page, which is likely to give you a new
renderer process, use the "recheck" link on the page.
Committed: https://crrev.com/7a219524da03c684d4ca6367caca55bbcf79dbab
Cr-Commit-Position: refs/heads/master@{#430857}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use blink variant mojom #Patch Set 3 : Rebase only #
Total comments: 22
Patch Set 4 : Create blink::WorkerThread::PostTaskToAllWorkerThreads helper function #
Total comments: 4
Patch Set 5 : Use blink::toIsolate(ExecutionContext*) instead #Patch Set 6 : Initialize mojo before initializing blink #
Total comments: 17
Patch Set 7 : Expose blink::WorkerThread::workerThreads() #Patch Set 8 : Inside modules use V8PerIsolateData::mainThreadIsolate() instead of blink::mainThreadIsolate() #
Total comments: 6
Patch Set 9 : Introduce blink::initializeMojo() API #Patch Set 10 : webkit_unit_tests does not need to init mojo edk any more by itself #
Total comments: 1
Patch Set 11 : content_unittests does not need to init mojo edk any more by itself #
Total comments: 32
Patch Set 12 : Address comments from blundell@ and haraken@ #Patch Set 13 : Rebase only #Patch Set 14 : Remove all duplicated calls to mojo::edk::init() #Patch Set 15 : Remove blink::initializeMojoForTest() #Patch Set 16 : Rebase only #
Total comments: 17
Patch Set 17 : Address nits from haraken@ #Patch Set 18 : Only changes for time zone monitor #Patch Set 19 : Fix blink_heap_unittests #
Total comments: 12
Patch Set 20 : Address nits and revert blink_heap_unittests changes because https://codereview.chromium.org/2453933003/ la… #Patch Set 21 : Remove TimeZoneMonitorClient::Bind() #
Total comments: 4
Patch Set 22 : Rebase only #Patch Set 23 : Address comments from kinuko@ and blundell@ #
Total comments: 2
Patch Set 24 : Address nit and rebase #Patch Set 25 : Rebase only #Messages
Total messages: 219 (125 generated)
leon.han@intel.com changed reviewers: + blundell@chromium.org
This CL can build well and test OK locally, but as I'm not familiar with blink code,, it still has presubmit errors. And seems these errors are showing some architectural problems.. Instead of myself checking around blink code here and there, I think your suggestions would be very helpful to speedy my work to improve this CL further. Thanks in advance;-) https://codereview.chromium.org/2402983002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/2402983002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:50: using IDToTaskRunnerMap = std::map<base::PlatformThreadId, base::TaskRunner*>; Presubmit ERRORS: Do not use Chromium class from namespace base inside Blink core. https://codereview.chromium.org/2402983002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:54: base::Bind(&NotifyTimezoneChangeOnThisThread)); Presubmit ERRORS: Do not use Chromium class from namespace base inside Blink core.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
blundell@chromium.org changed reviewers: + haraken@chromium.org
This looks like a good start! I'm not familiar enough with Blink to be able to answer either your questions about the WTF equivalents of //base primitives or the questions that I left in comments :). Kentaro, could you either review this CL or forward it on to the best reviewer? It's part of Blink Onion Soup. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:72: static void PostTaskToAllThreads(const base::Closure& closure); Do the WorkerBackingThreads correspond exactly to workers in content::PostTaskToAllWebWorkers()? https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:72: static void PostTaskToAllThreads(const base::Closure& closure); Could we reuse WorkerThread::workerThread() instead of creating parallel functionality? https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/ModulesInitializer.cpp:51: TimeZoneMonitorClient::Init(); Does this match 1:1 with the lifetime of RenderThreadImpl?
haraken@chromium.org changed reviewers: + nhiroki@chromium.org
+nhiroki https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:21: #include <map> Don't use std::map in Blink. You can use HashMap. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:51: static IDToTaskRunnerMap& threadRunners() { Yeah, this looks nasty. nhiroki@: What's an expected way to get a set of all workers in Blink? https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/ModulesInitializer.cpp:51: TimeZoneMonitorClient::Init(); On 2016/10/11 11:29:06, blundell wrote: > Does this match 1:1 with the lifetime of RenderThreadImpl? Yes. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:18: v8::Isolate* isolate = v8::Isolate::GetCurrent(); v8::Isolate::GetCurrent() is deprecated and should be avoided. It's better to pass in the isolate from WorkerGlobalScope or something. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:44: void TimeZoneMonitorClient::OnTimeZoneChange(const WTF::String& tzInfo) { timeZoneInfo Blink prefers a fully qualified name.
Thanks, Kentaro! Another question. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:54: base::Bind(&NotifyTimezoneChangeOnThisThread)); What's the WTF equivalent of the base::Bind infrastructure?
https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:54: base::Bind(&NotifyTimezoneChangeOnThisThread)); On 2016/10/11 11:47:56, blundell wrote: > What's the WTF equivalent of the base::Bind infrastructure? WTF::bind.
Thanks all for kindly help! Somehow becoming confident.. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:21: #include <map> On 2016/10/11 11:45:06, haraken wrote: > > Don't use std::map in Blink. You can use HashMap. Acknowledged. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:51: static IDToTaskRunnerMap& threadRunners() { On 2016/10/11 11:45:06, haraken wrote: > > Yeah, this looks nasty. > > nhiroki@: What's an expected way to get a set of all workers in Blink? Yeah the purpose is to post a task to all running worker threads in Blink. Thanks for help about this and eager to comments from nhiroki@. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:72: static void PostTaskToAllThreads(const base::Closure& closure); On 2016/10/11 11:29:06, blundell wrote: > Do the WorkerBackingThreads correspond exactly to workers in > content::PostTaskToAllWebWorkers()? Yeah this has exactly the same effect with content::WorkerThreadRegistry::PostTaskToAllThreads(), which is posting task to all threads registered via blink::Platform API at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/workers/W... . As we want to do the same work inside blink, I maintains the worker threads via a static blink::threadRunners(), maybe this is not proper way but I did not find out how to enumerate all worker threads inside blink. #I noticed haraken@ has helped asking nhiroki@ about this. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:72: static void PostTaskToAllThreads(const base::Closure& closure); On 2016/10/11 11:29:06, blundell wrote: > Could we reuse WorkerThread::workerThread() instead of creating parallel > functionality? Seems multiple blink::WorkerThread correspond with 1 blink::WorkerBackingThread, and content::WorkerThreadRegistry is tracking blink::WorkerBackingThread, so I'm focusing on the latter one. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/ModulesInitializer.cpp:51: TimeZoneMonitorClient::Init(); On 2016/10/11 11:45:06, haraken wrote: > On 2016/10/11 11:29:06, blundell wrote: > > Does this match 1:1 with the lifetime of RenderThreadImpl? > > Yes. Actually I have no enough knowledge about rules in Blink so I'm not sure my solution here is correct or not. Maybe I should impl TimeZoneMonitorClient in third_party/Webkit/Source/platform rather than third_party/Webkit/Source/modules? And I'm somehow confused about object life-cycle management for my TimeZoneMonitorClient following with rules in Blink. Would you please suggest more about this? Thanks ;-) https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:18: v8::Isolate* isolate = v8::Isolate::GetCurrent(); On 2016/10/11 11:45:06, haraken wrote: > > v8::Isolate::GetCurrent() is deprecated and should be avoided. It's better to > pass in the isolate from WorkerGlobalScope or something. Acknowledged and will investigate around how to do so. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:44: void TimeZoneMonitorClient::OnTimeZoneChange(const WTF::String& tzInfo) { On 2016/10/11 11:45:06, haraken wrote: > > timeZoneInfo > > Blink prefers a fully qualified name. Acknowledged. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:54: base::Bind(&NotifyTimezoneChangeOnThisThread)); On 2016/10/11 11:50:47, haraken wrote: > On 2016/10/11 11:47:56, blundell wrote: > > What's the WTF equivalent of the base::Bind infrastructure? > > WTF::bind. Acknowledged.
https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:51: static IDToTaskRunnerMap& threadRunners() { On 2016/10/11 11:45:06, haraken wrote: > > Yeah, this looks nasty. > > nhiroki@: What's an expected way to get a set of all workers in Blink? As blundell@'s comment (and an email thread[1]), WorkerThread::workerThreads() is an expected way to communicate with all workers. The function is private, so we need to make it public or create a static helper function like WorkerThread::PostTaskToAllWorkerThreads(). [1] https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/G0qnCE5A67o/ leonhsl: As you commented, multiple blink::WorkerThread can correspond with 1 blink::WorkerBackingThread. If you'd like to avoid posting duplicate tasks to the same WorkerBackingThread, you could filter out the set of WorkerThread returned by workerThreads() in some way, for example, memoize Isolate and skip if it's duplicate. https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:72: static void PostTaskToAllThreads(const base::Closure& closure); On 2016/10/11 11:29:06, blundell wrote: > Do the WorkerBackingThreads correspond exactly to workers in > content::PostTaskToAllWebWorkers()? (This is not related to this CL and this looks like an issue of worker infra, but let me leave a comment for the future) Apparently, PostTaskToAllWebWorkers() doesn't post a task to WorkerBackingThreads when they don't own a platform thread. AnimationWorklet/AudioWorklet/CompositorWorker are such cases and these haven't been shipped yet, so this is not a real problem. We may need to fix it before shipping them. === Details === WorkerBackingThread creates and owns a dedicated platform thread for typical workers, but it doesn't create a thread and instead borrows an existing platform thread for some special workers (e.g. CompositorWorker is hosted on the existing compositor thread[1]). Note that these special workers have NOT been shipped yet. WorkerThreadRegistry::PostTaskToAllThreads() posts a task to |task_runner_map_|. This map is updated on WorkerThreadRegistry::DidStartCurrentWorkerThread()[2]. This is called from WorkerBackingThread::initialize() only when WorkerBackingThread owns a platform thread[3]. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/compos... [2] https://cs.chromium.org/chromium/src/content/child/worker_thread_registry.cc?... [3] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/workers/W...
https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:72: static void PostTaskToAllThreads(const base::Closure& closure); On 2016/10/12 06:20:20, nhiroki wrote: > On 2016/10/11 11:29:06, blundell wrote: > > Do the WorkerBackingThreads correspond exactly to workers in > > content::PostTaskToAllWebWorkers()? > > (This is not related to this CL and this looks like an issue of worker infra, > but let me leave a comment for the future) > > Apparently, PostTaskToAllWebWorkers() doesn't post a task to > WorkerBackingThreads when they don't own a platform thread. > AnimationWorklet/AudioWorklet/CompositorWorker are such cases and these haven't > been shipped yet, so this is not a real problem. We may need to fix it before > shipping them. > > === Details === > > WorkerBackingThread creates and owns a dedicated platform thread for typical > workers, but it doesn't create a thread and instead borrows an existing platform > thread for some special workers (e.g. CompositorWorker is hosted on the existing > compositor thread[1]). Note that these special workers have NOT been shipped > yet. > > WorkerThreadRegistry::PostTaskToAllThreads() posts a task to |task_runner_map_|. > This map is updated on WorkerThreadRegistry::DidStartCurrentWorkerThread()[2]. > This is called from WorkerBackingThread::initialize() only when > WorkerBackingThread owns a platform thread[3]. > > [1] > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/compos... > [2] > https://cs.chromium.org/chromium/src/content/child/worker_thread_registry.cc?... > [3] > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/workers/W... Filed an issue: https://bugs.chromium.org/p/chromium/issues/detail?id=655028
Thanks, everyone! Han Leon, it looks to me like you have all the info you need now to move forward on this CL.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Yeah thanks everyone for help! I just uploaded ps#4, at least presubmit errors disappeared. Would you PTAnL at this with my additional questions ;-) https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:51: static IDToTaskRunnerMap& threadRunners() { On 2016/10/12 06:20:20, nhiroki wrote: > On 2016/10/11 11:45:06, haraken wrote: > > > > Yeah, this looks nasty. > > > > nhiroki@: What's an expected way to get a set of all workers in Blink? > > As blundell@'s comment (and an email thread[1]), WorkerThread::workerThreads() > is an expected way to communicate with all workers. The function is private, so > we need to make it public or create a static helper function like > WorkerThread::PostTaskToAllWorkerThreads(). > > [1] https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/G0qnCE5A67o/ > > leonhsl: As you commented, multiple blink::WorkerThread can correspond with 1 > blink::WorkerBackingThread. If you'd like to avoid posting duplicate tasks to > the same WorkerBackingThread, you could filter out the set of WorkerThread > returned by workerThreads() in some way, for example, memoize Isolate and skip > if it's duplicate. Thanks a lot! I suppose WorkerThread::isOwningBackingThread() could be used to avoid possible duplication, is it OK? https://codereview.chromium.org/2402983002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:21: v8::Isolate* isolate = v8::Isolate::GetCurrent(); As haraken@ commented, v8::Isolate::GetCurrent() is deprecated and should be avoided, I've investigated around but can't find out a proper way. Is it possible to get the v8::Isolate instance from ExecutionContext? Thanks. https://codereview.chromium.org/2402983002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:53: NotifyTimezoneChange(nullptr); For the main thread, how could we get the v8::Isolate instance? Thanks.
https://codereview.chromium.org/2402983002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:21: v8::Isolate* isolate = v8::Isolate::GetCurrent(); On 2016/10/12 13:54:44, leonhsl wrote: > As haraken@ commented, v8::Isolate::GetCurrent() is deprecated and should be > avoided, I've investigated around but can't find out a proper way. Is it > possible to get the v8::Isolate instance from ExecutionContext? Thanks. We have toIsolate(ExecutionContext*).
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:120001) has been deleted
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Uploaded ps#6, although trybots are still red(see bellowing discussion), I suppose codes are ready for an overall look now ;-) About trybots failures: I think all the troubles come from the newly emerged fact that now the execution of blink::initialize() needs mojo environment ready, because it is calling TimeZoneMonitorClient::Init(), which will try to establish mojo connection with peer. This means whenever before you call blink::initialize(), you must ensure: 1, Mojo system has been initialized. #I've achieved this at ps#6. 2, The message loop for current thread is ready. #Sadly this is hard to achieve and is causing trybots failures. #Details: content::UnitTestTestSuite constructor constructs content::TestBlinkWebUnitTestSupport, whose constructor is calling blink::initialize(), while the message loop is created lately at https://cs.chromium.org/chromium/src/base/test/launcher/unit_test_launcher.cc... when launching the test suite. Any thoughts about this? Thanks in advance. https://codereview.chromium.org/2402983002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:21: v8::Isolate* isolate = v8::Isolate::GetCurrent(); On 2016/10/12 14:13:19, haraken wrote: > On 2016/10/12 13:54:44, leonhsl wrote: > > As haraken@ commented, v8::Isolate::GetCurrent() is deprecated and should be > > avoided, I've investigated around but can't find out a proper way. Is it > > possible to get the v8::Isolate instance from ExecutionContext? Thanks. > > We have toIsolate(ExecutionContext*). Got it and thanks!
https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:330: if (thread->isOwningBackingThread()) isOwningBackingThread() returns true when this WorkerThread exclusively owns WorkerBackingThread. In other words, if WorkerThread shares a WorkerBackingThread with other WorkerThreads, this returns false. Therefore, a task is never posted to WorkerBackingThread that is shared among multiple WorkerThreads. I guess this behavior is different from your expectation. Instead of isOwningBackingThread(), how about recording a pointer to WorkerBackingThread and check it on each iteration like this? WTF::HashSet<WorkerBackingThread*> posted; for (WorkerThread* thread : threads) { if (posted.contains(&thread->workerBackingThread())) continue; thread->postTask(...); posted.add(&thread->workerBackingThread()); } (NOTE: I have't checked this code snippet with a compiler, so this may need some modification) In addition, this filter seems to be specific to your usecase. It'd be more common that a caller of postTask() doesn't take care of whether target WorkerThreads share a WorkerBackingThread. So, how about making workerThreads() public and filtering in TimeZoneMonitorClient::OnTimeZoneChange() instead? This also enables to duplicate a task (see my other comment) https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:331: thread->postTask(location, std::move(task)); |task| is moved here, so you can't use it on following iterations. https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:46: void TimeZoneMonitorClient::OnTimeZoneChange(const WTF::String& timeZoneInfo) { Is this called only from the main thread? I'm concerned that posting a task to all worker threads from non-main thread may cause race conditions with worker thread destruction that happens on the main thread. https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:54: NotifyTimezoneChange(nullptr); Does this call make sense?
nhiroki@, Thanks a lot for review! https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:330: if (thread->isOwningBackingThread()) On 2016/10/13 10:24:12, nhiroki wrote: > isOwningBackingThread() returns true when this WorkerThread exclusively owns > WorkerBackingThread. In other words, if WorkerThread shares a > WorkerBackingThread with other WorkerThreads, this returns false. Therefore, a > task is never posted to WorkerBackingThread that is shared among multiple > WorkerThreads. I guess this behavior is different from your expectation. > > Instead of isOwningBackingThread(), how about recording a pointer to > WorkerBackingThread and check it on each iteration like this? > > WTF::HashSet<WorkerBackingThread*> posted; > for (WorkerThread* thread : threads) { > if (posted.contains(&thread->workerBackingThread())) > continue; > thread->postTask(...); > posted.add(&thread->workerBackingThread()); > } > > (NOTE: I have't checked this code snippet with a compiler, so this may need some > modification) > > In addition, this filter seems to be specific to your usecase. It'd be more > common that a caller of postTask() doesn't take care of whether target > WorkerThreads share a WorkerBackingThread. So, how about making workerThreads() > public and filtering in TimeZoneMonitorClient::OnTimeZoneChange() instead? This > also enables to duplicate a task (see my other comment) Oh,, I misunderstood that isOwningBackingThread() means the WorkerThread is the *main* worker thread for a WorkerBackingThread. Thanks for the new filter suggestion. A question: If we'll make workerThreads() public, then we must also make the mutext threadSetMutex() public and enforce workerThreads()'s callers always lock the mutex until they finished using the returned threads. This looks like somehow dangerous.. https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:331: thread->postTask(location, std::move(task)); On 2016/10/13 10:24:12, nhiroki wrote: > |task| is moved here, so you can't use it on following iterations. My bad.. Thanks! https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:46: void TimeZoneMonitorClient::OnTimeZoneChange(const WTF::String& timeZoneInfo) { On 2016/10/13 10:24:12, nhiroki wrote: > Is this called only from the main thread? > > I'm concerned that posting a task to all worker threads from non-main thread may > cause race conditions with worker thread destruction that happens on the main > thread. Yeah mojo binding is thread hostile and the mojo methods are always called on the thread on which the mojo impl has been bound. We bind our TimeZoneMonitorClient impl at line#41 of this file, which executes on the main thread. https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:54: NotifyTimezoneChange(nullptr); On 2016/10/13 10:24:12, nhiroki wrote: > Does this call make sense? This is to notify the V8 isolate for current thread(main thread), I noticed that toIsolate(nullptr) will lead to the isolate of current thread. Do you have any suggestion here? Thanks.
On 2016/10/13 09:28:53, leonhsl wrote: > Uploaded ps#6, although trybots are still red(see bellowing discussion), I > suppose codes are ready for an overall look now ;-) > > About trybots failures: > I think all the troubles come from the newly emerged fact that now the execution > of blink::initialize() needs mojo environment ready, because it is calling > TimeZoneMonitorClient::Init(), which will try to establish mojo connection with > peer. > This means whenever before you call blink::initialize(), you must ensure: > 1, Mojo system has been initialized. > #I've achieved this at ps#6. > 2, The message loop for current thread is ready. > #Sadly this is hard to achieve and is causing trybots failures. > #Details: content::UnitTestTestSuite constructor constructs > content::TestBlinkWebUnitTestSupport, whose constructor is calling > blink::initialize(), while the message loop is created lately at > https://cs.chromium.org/chromium/src/base/test/launcher/unit_test_launcher.cc... > when launching the test suite. > Any thoughts about this? Thanks in advance. > Were you able to figure out why this wasn't a problem when the TimeZoneMonitor connection setup was executing in the constructor of RenderThreadImpl()?
On 2016/10/14 07:43:13, blundell wrote: > On 2016/10/13 09:28:53, leonhsl wrote: > > Uploaded ps#6, although trybots are still red(see bellowing discussion), I > > suppose codes are ready for an overall look now ;-) > > > > About trybots failures: > > I think all the troubles come from the newly emerged fact that now the > execution > > of blink::initialize() needs mojo environment ready, because it is calling > > TimeZoneMonitorClient::Init(), which will try to establish mojo connection > with > > peer. > > This means whenever before you call blink::initialize(), you must ensure: > > 1, Mojo system has been initialized. > > #I've achieved this at ps#6. > > 2, The message loop for current thread is ready. > > #Sadly this is hard to achieve and is causing trybots failures. > > #Details: content::UnitTestTestSuite constructor constructs > > content::TestBlinkWebUnitTestSupport, whose constructor is calling > > blink::initialize(), while the message loop is created lately at > > > https://cs.chromium.org/chromium/src/base/test/launcher/unit_test_launcher.cc... > > when launching the test suite. > > Any thoughts about this? Thanks in advance. > > > > Were you able to figure out why this wasn't a problem when the TimeZoneMonitor > connection setup was executing in the constructor of RenderThreadImpl()? The main message loop is created at [1], while RenderThreadImpl is constructed after that, at [2], so RenderThreadImpl constructor has become capable to run all mojo related operations. Actually chrome itself built from this CL can run well because Blink is initialized inside RenderThreadImpl constructor, while all of our troubles are from test codes, which initialize Blink by themselves. [1] https://cs.chromium.org/chromium/src/content/renderer/renderer_main.cc?rcl=14... [2] https://cs.chromium.org/chromium/src/content/renderer/renderer_main.cc?rcl=14...
On 2016/10/14 08:01:38, leonhsl wrote: > On 2016/10/14 07:43:13, blundell wrote: > > On 2016/10/13 09:28:53, leonhsl wrote: > > > Uploaded ps#6, although trybots are still red(see bellowing discussion), I > > > suppose codes are ready for an overall look now ;-) > > > > > > About trybots failures: > > > I think all the troubles come from the newly emerged fact that now the > > execution > > > of blink::initialize() needs mojo environment ready, because it is calling > > > TimeZoneMonitorClient::Init(), which will try to establish mojo connection > > with > > > peer. > > > This means whenever before you call blink::initialize(), you must ensure: > > > 1, Mojo system has been initialized. > > > #I've achieved this at ps#6. > > > 2, The message loop for current thread is ready. > > > #Sadly this is hard to achieve and is causing trybots failures. > > > #Details: content::UnitTestTestSuite constructor constructs > > > content::TestBlinkWebUnitTestSupport, whose constructor is calling > > > blink::initialize(), while the message loop is created lately at > > > > > > https://cs.chromium.org/chromium/src/base/test/launcher/unit_test_launcher.cc... > > > when launching the test suite. > > > Any thoughts about this? Thanks in advance. > > > > > > > Were you able to figure out why this wasn't a problem when the TimeZoneMonitor > > connection setup was executing in the constructor of RenderThreadImpl()? > > The main message loop is created at [1], while RenderThreadImpl is constructed > after that, at [2], so RenderThreadImpl constructor has become capable to run > all mojo related operations. Actually chrome itself built from this CL can run > well because Blink is initialized inside RenderThreadImpl constructor, while all > of our troubles are from test codes, which initialize Blink by themselves. > [1] > https://cs.chromium.org/chromium/src/content/renderer/renderer_main.cc?rcl=14... > [2] > https://cs.chromium.org/chromium/src/content/renderer/renderer_main.cc?rcl=14... There are a couple possible solutions that I see: (1) Exploit the fact that Blink can tell whether there's a current MessageLoop and avoid initializing Mojo stuff in blink::initialize() if there's no current message loop. Could make this explicit by adding a blink::initializeWithoutMessageLoopForTesting() interface and only supporting the lack of a current MessageLoop in that context. Of course, then the test suites that utilize that won't have the Mojo stuff initialized. (2) Add a blink::initializeMojo() method that the client is responsible for calling at a time when there *is* a current MessageLoop. Neither of these solutions is particularly appealing, but I don't see anything else obvious.
Sorry for my delayed reply. https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:330: if (thread->isOwningBackingThread()) On 2016/10/14 03:12:00, leonhsl wrote: > On 2016/10/13 10:24:12, nhiroki wrote: > > isOwningBackingThread() returns true when this WorkerThread exclusively owns > > WorkerBackingThread. In other words, if WorkerThread shares a > > WorkerBackingThread with other WorkerThreads, this returns false. Therefore, a > > task is never posted to WorkerBackingThread that is shared among multiple > > WorkerThreads. I guess this behavior is different from your expectation. > > > > Instead of isOwningBackingThread(), how about recording a pointer to > > WorkerBackingThread and check it on each iteration like this? > > > > WTF::HashSet<WorkerBackingThread*> posted; > > for (WorkerThread* thread : threads) { > > if (posted.contains(&thread->workerBackingThread())) > > continue; > > thread->postTask(...); > > posted.add(&thread->workerBackingThread()); > > } > > > > (NOTE: I have't checked this code snippet with a compiler, so this may need > some > > modification) > > > > In addition, this filter seems to be specific to your usecase. It'd be more > > common that a caller of postTask() doesn't take care of whether target > > WorkerThreads share a WorkerBackingThread. So, how about making > workerThreads() > > public and filtering in TimeZoneMonitorClient::OnTimeZoneChange() instead? > This > > also enables to duplicate a task (see my other comment) > > Oh,, I misunderstood that isOwningBackingThread() means the WorkerThread is the > *main* worker thread for a WorkerBackingThread. Thanks for the new filter > suggestion. Yeah..., this is so confusing. To be more precise, "BackingThread" means a platform thread (e.g. Pthreads) here and isOwningBackingThread() is used for checking whether it's necessary to initialize/finalize the platform thread when WorkerBackingThread is constructed/destructed. As my other comment[1], WorkerBackingThread can borrows an existing platform thread (e.g. compositor thread, audio thread) instead of creating a dedicated one. In the case, we don't have to initialize/finalize a platform thread on construction/destruction, so the function returns false. [1] https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... > A question: > If we'll make workerThreads() public, then we must also make the mutext > threadSetMutex() public and enforce workerThreads()'s callers always lock the > mutex until they finished using the returned threads. This looks like somehow > dangerous.. Good point. I think the caller can use the returned threads without a lock if they are synchronously used on the main thread, because the thread set is updated only on the main thread. To clarify that, we may want to have a comment on workerThreads() like this: // Returns a set of all worker threads. This must be called only on the main thread and the returned set must not be stored for future use. HashSet<WorkerThread*>& WorkerThread::workerThreads();
https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:46: void TimeZoneMonitorClient::OnTimeZoneChange(const WTF::String& timeZoneInfo) { On 2016/10/14 03:12:00, leonhsl wrote: > On 2016/10/13 10:24:12, nhiroki wrote: > > Is this called only from the main thread? > > > > I'm concerned that posting a task to all worker threads from non-main thread > may > > cause race conditions with worker thread destruction that happens on the main > > thread. > > Yeah mojo binding is thread hostile and the mojo methods are always called on > the thread on which the mojo impl has been bound. We bind our > TimeZoneMonitorClient impl at line#41 of this file, which executes on the main > thread. OK, then we can safely access WorkerThread::workerThreads() from here (see my other comments on WorkerThread.cpp). https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:54: NotifyTimezoneChange(nullptr); On 2016/10/14 03:12:00, leonhsl wrote: > On 2016/10/13 10:24:12, nhiroki wrote: > > Does this call make sense? > > This is to notify the V8 isolate for current thread(main thread), I noticed that > toIsolate(nullptr) will lead to the isolate of current thread. Do you have any > suggestion here? Thanks. Thank you for the explanation. I don't know whether there is a better way. haraken@ may know it.
https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:23: if (!isolate) This shouldn't happen. https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:54: NotifyTimezoneChange(nullptr); On 2016/10/17 03:56:37, nhiroki (slow) wrote: > On 2016/10/14 03:12:00, leonhsl wrote: > > On 2016/10/13 10:24:12, nhiroki wrote: > > > Does this call make sense? > > > > This is to notify the V8 isolate for current thread(main thread), I noticed > that > > toIsolate(nullptr) will lead to the isolate of current thread. Do you have any > > suggestion here? Thanks. > > Thank you for the explanation. I don't know whether there is a better way. > haraken@ may know it. Can you make NotifyTimezoneChange take an Isolate parameter and then pass in mainThreadIsolate()?
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded ps#7 to address comments from nhiroki@ and haraken@, would you PTAnL? Thanks. I'm still investigating around the issue about blink initialization without a message loop in case of some test codes, according blundell@'s suggestions. https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:330: if (thread->isOwningBackingThread()) On 2016/10/17 03:49:11, nhiroki (slow) wrote: > On 2016/10/14 03:12:00, leonhsl wrote: > > On 2016/10/13 10:24:12, nhiroki wrote: > > > isOwningBackingThread() returns true when this WorkerThread exclusively owns > > > WorkerBackingThread. In other words, if WorkerThread shares a > > > WorkerBackingThread with other WorkerThreads, this returns false. Therefore, > a > > > task is never posted to WorkerBackingThread that is shared among multiple > > > WorkerThreads. I guess this behavior is different from your expectation. > > > > > > Instead of isOwningBackingThread(), how about recording a pointer to > > > WorkerBackingThread and check it on each iteration like this? > > > > > > WTF::HashSet<WorkerBackingThread*> posted; > > > for (WorkerThread* thread : threads) { > > > if (posted.contains(&thread->workerBackingThread())) > > > continue; > > > thread->postTask(...); > > > posted.add(&thread->workerBackingThread()); > > > } > > > > > > (NOTE: I have't checked this code snippet with a compiler, so this may need > > some > > > modification) > > > > > > In addition, this filter seems to be specific to your usecase. It'd be more > > > common that a caller of postTask() doesn't take care of whether target > > > WorkerThreads share a WorkerBackingThread. So, how about making > > workerThreads() > > > public and filtering in TimeZoneMonitorClient::OnTimeZoneChange() instead? > > This > > > also enables to duplicate a task (see my other comment) > > > > Oh,, I misunderstood that isOwningBackingThread() means the WorkerThread is > the > > *main* worker thread for a WorkerBackingThread. Thanks for the new filter > > suggestion. > > Yeah..., this is so confusing. > > To be more precise, "BackingThread" means a platform thread (e.g. Pthreads) here > and isOwningBackingThread() is used for checking whether it's necessary to > initialize/finalize the platform thread when WorkerBackingThread is > constructed/destructed. As my other comment[1], WorkerBackingThread can borrows > an existing platform thread (e.g. compositor thread, audio thread) instead of > creating a dedicated one. In the case, we don't have to initialize/finalize a > platform thread on construction/destruction, so the function returns false. > > [1] > https://codereview.chromium.org/2402983002/diff/40001/third_party/WebKit/Sour... > > > A question: > > If we'll make workerThreads() public, then we must also make the mutext > > threadSetMutex() public and enforce workerThreads()'s callers always lock the > > mutex until they finished using the returned threads. This looks like somehow > > dangerous.. > > Good point. I think the caller can use the returned threads without a lock if > they are synchronously used on the main thread, because the thread set is > updated only on the main thread. > > To clarify that, we may want to have a comment on workerThreads() like this: > > // Returns a set of all worker threads. This must be called only on the main > thread and the returned set must not be stored for future use. > HashSet<WorkerThread*>& WorkerThread::workerThreads(); Done. Thank you very much for sharing the details and the solution suggestion! https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:23: if (!isolate) On 2016/10/17 03:59:49, haraken wrote: > > This shouldn't happen. Got it, changed to DCHECK(isolate). https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:46: void TimeZoneMonitorClient::OnTimeZoneChange(const WTF::String& timeZoneInfo) { On 2016/10/17 03:56:37, nhiroki (slow) wrote: > On 2016/10/14 03:12:00, leonhsl wrote: > > On 2016/10/13 10:24:12, nhiroki wrote: > > > Is this called only from the main thread? > > > > > > I'm concerned that posting a task to all worker threads from non-main thread > > may > > > cause race conditions with worker thread destruction that happens on the > main > > > thread. > > > > Yeah mojo binding is thread hostile and the mojo methods are always called on > > the thread on which the mojo impl has been bound. We bind our > > TimeZoneMonitorClient impl at line#41 of this file, which executes on the main > > thread. > > OK, then we can safely access WorkerThread::workerThreads() from here (see my > other comments on WorkerThread.cpp). Got it. https://codereview.chromium.org/2402983002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:54: NotifyTimezoneChange(nullptr); On 2016/10/17 03:59:49, haraken wrote: > On 2016/10/17 03:56:37, nhiroki (slow) wrote: > > On 2016/10/14 03:12:00, leonhsl wrote: > > > On 2016/10/13 10:24:12, nhiroki wrote: > > > > Does this call make sense? > > > > > > This is to notify the V8 isolate for current thread(main thread), I noticed > > that > > > toIsolate(nullptr) will lead to the isolate of current thread. Do you have > any > > > suggestion here? Thanks. > > > > Thank you for the explanation. I don't know whether there is a better way. > > haraken@ may know it. > > Can you make NotifyTimezoneChange take an Isolate parameter and then pass in > mainThreadIsolate()? Done and thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Uploaded ps#8 to use V8PerIsolateData::mainThreadIsolate() instead of blink::mainThreadIsolate(), because codes in third_party/WebKit/Source/modules/ should not depend on third_party/WebKit/Source/public/web/.
LGTM from the point of view of worker code. https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:318: HashSet<WorkerThread*>& WorkerThread::workerThreads() { DCHECK(isMainThread());
WebKit LGTM https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:36: static TimeZoneMonitorClient* instance = nullptr; Use DEFINE_STATIC_LOCAL https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:67: if (posted.contains(&thread->workerBackingThread())) Add a comment about why we need this check.
Hi Han Leon, I'm assuming that you're still looking at fixing initialization stuff in order to get green bots. Let me know if that's not the case!
On 2016/10/17 15:19:34, blundell wrote: > Hi Han Leon, > > I'm assuming that you're still looking at fixing initialization stuff in order > to get green bots. Let me know if that's not the case! Yeah I'm preparing the new patch set and have updated the status at https://codereview.chromium.org/2402983002/#msg53. Will upload the new patch set soon and wish we can get all bots green ;-)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Hi, haraken@, blundell@, would you PTAnL at changes in ps#9 and ps#10? All bots are green now except one android bot,, I'm investigating the root cause. Thanks. https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:318: HashSet<WorkerThread*>& WorkerThread::workerThreads() { On 2016/10/17 12:01:08, nhiroki (slow) wrote: > DCHECK(isMainThread()); Done. https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:36: static TimeZoneMonitorClient* instance = nullptr; On 2016/10/17 13:02:55, haraken wrote: > > Use DEFINE_STATIC_LOCAL Done. https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:67: if (posted.contains(&thread->workerBackingThread())) On 2016/10/17 13:02:54, haraken wrote: > > Add a comment about why we need this check. Done. https://codereview.chromium.org/2402983002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/RunAllTests.cpp (left): https://codereview.chromium.org/2402983002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/RunAllTests.cpp:70: mojo::edk::Init(); No need to init mojo edk any more, content::SetUpBlinkTestEnvironment() will do this. #When constructing content::TestBlinkWebUnitTestSupport
Patchset #11 (id:240001) has been deleted
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded ps#11 to fix android trybot failure. https://codereview.chromium.org/2402983002/diff/260001/content/test/run_all_u... File content/test/run_all_unittests.cc (left): https://codereview.chromium.org/2402983002/diff/260001/content/test/run_all_u... content/test/run_all_unittests.cc:18: content::InitializeMojo(); No need to init mojo edk any more, content::UnitTestTestSuite constructor will do this. #When constructing content::TestBlinkWebUnitTestSupport
Thanks! Looks good with a bunch of nits plus one question for nhiroki@. nhiroki@, could you look at my comment on TimeZoneMonitorClient.cpp:71? Thanks! https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... content/test/test_blink_web_unit_test_support.cc:128: // Initialize mojo firstly to enable Blink initialization use it. nit: s/use/to use https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... File media/blink/run_all_unittests.cc (right): https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... media/blink/run_all_unittests.cc:89: // Initialize mojo firstly to enable Blink initialization use it. ("to use" on all of these comments throughout this CL) https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.cpp:81: if (m_isMojoInitialized) Can this just be an assert(!m_isMojoInitialized) instead? If not, why not? https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.h (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.h:17: void initializeMojo(); I'm not sure on Blink conventions re: comments on methods. I would ordinarily say that this deserves a client on why it's a separate method and what its preconditions are (that the platform thread needs to exist). https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/DEPS (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/DEPS:2: "+device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom-blink.h", any reason not to leave these at the directory level rather than individual file level? https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/OWNERS (left): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/OWNERS:1: blundell@chromium.org Add mark@chromium.org please. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:71: thread->postTask(BLINK_FROM_HERE, createCrossThreadTask( To be totally explicit about this issue: This behavior is different from what it was previously, as now this task will be posted to workerBackingThreads that don't own their platform thread. nhiroki's comment upthread (and crbug.com/655028) seem to indicate that the *new* behavior is the correct one, and that the old one was incorrect. nhiroki@, am I understanding you correctly there? If that is true, then this behavioral change should be called out in the CL description. I would also call out the changes that you're needing to make to support initialization of Mojo in Blink now. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:85: if (Platform::isMessageLoopReady()) I see, this is why you have initializeMojo() be safe for multiple calls. Did you do this so that production code continues to Just Work? If so, maybe it makes sense to rename the public-facing method initializeMojoForTesting() to make it clear that it should only need to be called in a testing context where blink::initialize() is called without the platform having a current thread? https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:85: if (Platform::isMessageLoopReady()) Nit: It seems like you don't need to add Platform::isMessageLoopReady() because you can just use if (platform->currentThread()) as is done on line 89 below. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebKit.h (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/web/WebKit.h:47: // Blink's mojo stuff must be initialized with 2 prerequisites: Ah I see, this is where you added the comment. Great, exactly what I was envisioning. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/web/WebKit.h:50: // For production code it's sure that above 2 are already fullfilled before This comment strengthens my belief that we should name this method initializeMojoForTesting().
Hi blundell@, Thanks a lot for the review! I'll upload new patch set to address comments and update cl description tomorrow ;-) https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... content/test/test_blink_web_unit_test_support.cc:128: // Initialize mojo firstly to enable Blink initialization use it. On 2016/10/18 12:32:11, blundell wrote: > nit: s/use/to use Acknowledged. https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... File media/blink/run_all_unittests.cc (right): https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... media/blink/run_all_unittests.cc:89: // Initialize mojo firstly to enable Blink initialization use it. On 2016/10/18 12:32:11, blundell wrote: > ("to use" on all of these comments throughout this CL) Acknowledged. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/DEPS (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/DEPS:2: "+device/time_zone_monitor/public/interfaces/time_zone_monitor.mojom-blink.h", On 2016/10/18 12:32:11, blundell wrote: > any reason not to leave these at the directory level rather than individual file > level? Seems Webkit prefers such style, I found many similar places,, such as https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/batter... . Maybe haraken@ could help to share more information about this. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/OWNERS (left): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/OWNERS:1: blundell@chromium.org On 2016/10/18 12:32:11, blundell wrote: > Add mailto:mark@chromium.org please. Acknowledged. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:71: thread->postTask(BLINK_FROM_HERE, createCrossThreadTask( On 2016/10/18 12:32:11, blundell wrote: > To be totally explicit about this issue: > > This behavior is different from what it was previously, as now this task will be > posted to workerBackingThreads that don't own their platform thread. nhiroki's > comment upthread (and crbug.com/655028) seem to indicate that the *new* behavior > is the correct one, and that the old one was incorrect. nhiroki@, am I > understanding you correctly there? > > If that is true, then this behavioral change should be called out in the CL > description. > > I would also call out the changes that you're needing to make to support > initialization of Mojo in Blink now. Acknowledged. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:85: if (Platform::isMessageLoopReady()) On 2016/10/18 12:32:11, blundell wrote: > Nit: It seems like you don't need to add Platform::isMessageLoopReady() because > you can just use if (platform->currentThread()) as is done on line 89 below. I tried (platform->currentThread()) before, and turned out that it does not really indicate the existence of a message loop for current thread. I also tried to plumbed down into its implementation but did not find any hints leading to base::MessageLoop::current(). Maybe haraken@ could help to comment about this. Thanks. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:85: if (Platform::isMessageLoopReady()) On 2016/10/18 12:32:11, blundell wrote: > I see, this is why you have initializeMojo() be safe for multiple calls. Did you > do this so that production code continues to Just Work? If so, maybe it makes > sense to rename the public-facing method initializeMojoForTesting() to make it > clear that it should only need to be called in a testing context where > blink::initialize() is called without the platform having a current thread? Acknowledged. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebKit.h (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/web/WebKit.h:50: // For production code it's sure that above 2 are already fullfilled before On 2016/10/18 12:32:11, blundell wrote: > This comment strengthens my belief that we should name this method > initializeMojoForTesting(). Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:85: if (Platform::isMessageLoopReady()) On 2016/10/18 14:34:48, leonhsl wrote: > On 2016/10/18 12:32:11, blundell wrote: > > Nit: It seems like you don't need to add Platform::isMessageLoopReady() > because > > you can just use if (platform->currentThread()) as is done on line 89 below. > > I tried (platform->currentThread()) before, and turned out that it does not > really indicate the existence of a message loop for current thread. I also tried > to plumbed down into its implementation but did not find any hints leading to > base::MessageLoop::current(). Maybe haraken@ could help to comment about this. > Thanks. I think Platform::currentThread() should work. Would you investigate a bit more why it doesn't work? https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:146: static bool isMessageLoopReady(); I want to avoid introducing this if possible... https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebKit.h (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/web/WebKit.h:57: // stuff, which has been skipped by blink::initialize(). Hmm, would you summarize why you cannot just use initialize() to initialize Mojo? We really want to centralize all initialization paths to WebKit::initialize(). (I've done a lot of refactoring to centralize distributed initialization paths before.)
https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:71: thread->postTask(BLINK_FROM_HERE, createCrossThreadTask( On 2016/10/18 14:34:48, leonhsl wrote: > On 2016/10/18 12:32:11, blundell wrote: > > To be totally explicit about this issue: > > > > This behavior is different from what it was previously, as now this task will > be > > posted to workerBackingThreads that don't own their platform thread. nhiroki's > > comment upthread (and crbug.com/655028) seem to indicate that the *new* > behavior > > is the correct one, and that the old one was incorrect. nhiroki@, am I > > understanding you correctly there? Yes, that's right. > > If that is true, then this behavioral change should be called out in the CL > > description. > > > > I would also call out the changes that you're needing to make to support > > initialization of Mojo in Blink now. > > Acknowledged.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:300001) has been deleted
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
[TimeZoneMonitor] Decouple renderer side impl from content to blink.
BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
and then click "recheck" to ensure that the renderer picks up the new
time zone. Don't reload the page, which is likely to give you a new
renderer process, use the "recheck" link on the page.
==========
to
==========
[TimeZoneMonitor] Decouple renderer side impl from content to blink.
In addition to the decoupling work, this CL:
* notifies time zone change to all the workerBackingThread, no matter they
do OWN their platform thread or not, while the old behavior is that only
those workerBackingThread OWNING a platform thread could get the
notifications, which is incorrect.
* introduces a new WebKit API blink::initializeMojoForTest(), clients of Blink
could call this API to initialize those mojo stuff which may have been
skipped by blink::initialize() because the preconditions (mojo edk and
message loop for current thread) are not ready at that time point.
This would never happen for production codes, but some test codes will hit
such case because they may create their own type of message loop *after*
blink::initialize().
BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
and then click "recheck" to ensure that the renderer picks up the new
time zone. Don't reload the page, which is likely to give you a new
renderer process, use the "recheck" link on the page.
==========
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Uploaded new patch sets, would you PTAnL? Thanks. ps#12 : address comments ps#13 : rebase only ps#14 : remove all duplicated calls to mojo::edk::init(), because I've centralized the mojo edk initialization into constructor of TestBlinkWebUnitTestSupport, right before calling blink::initialize(). https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... content/test/test_blink_web_unit_test_support.cc:128: // Initialize mojo firstly to enable Blink initialization use it. On 2016/10/18 14:34:48, leonhsl wrote: > On 2016/10/18 12:32:11, blundell wrote: > > nit: s/use/to use > > Acknowledged. Done. https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... File media/blink/run_all_unittests.cc (right): https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... media/blink/run_all_unittests.cc:89: // Initialize mojo firstly to enable Blink initialization use it. On 2016/10/18 14:34:48, leonhsl wrote: > On 2016/10/18 12:32:11, blundell wrote: > > ("to use" on all of these comments throughout this CL) > > Acknowledged. Done. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/OWNERS (left): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/OWNERS:1: blundell@chromium.org On 2016/10/18 14:34:48, leonhsl wrote: > On 2016/10/18 12:32:11, blundell wrote: > > Add mailto:mark@chromium.org please. > > Acknowledged. Done. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:71: thread->postTask(BLINK_FROM_HERE, createCrossThreadTask( On 2016/10/18 14:34:48, leonhsl wrote: > On 2016/10/18 12:32:11, blundell wrote: > > To be totally explicit about this issue: > > > > This behavior is different from what it was previously, as now this task will > be > > posted to workerBackingThreads that don't own their platform thread. nhiroki's > > comment upthread (and crbug.com/655028) seem to indicate that the *new* > behavior > > is the correct one, and that the old one was incorrect. nhiroki@, am I > > understanding you correctly there? > > > > If that is true, then this behavioral change should be called out in the CL > > description. > > > > I would also call out the changes that you're needing to make to support > > initialization of Mojo in Blink now. > > Acknowledged. Done. Updated cl description. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:85: if (Platform::isMessageLoopReady()) On 2016/10/18 19:39:11, haraken wrote: > On 2016/10/18 14:34:48, leonhsl wrote: > > On 2016/10/18 12:32:11, blundell wrote: > > > Nit: It seems like you don't need to add Platform::isMessageLoopReady() > > because > > > you can just use if (platform->currentThread()) as is done on line 89 below. > > > > I tried (platform->currentThread()) before, and turned out that it does not > > really indicate the existence of a message loop for current thread. I also > tried > > to plumbed down into its implementation but did not find any hints leading to > > base::MessageLoop::current(). Maybe haraken@ could help to comment about this. > > Thanks. > > I think Platform::currentThread() should work. Would you investigate a bit more > why it doesn't work? content::TestBlinkWebUnitTestSupport is an impl of blink::Platform, and its constructor will create the 'current thread' at [1], before calling blink::initialize(), while at this time point there may has no message loop ready, many test case codes will create their own type of message loop at a very late stage. [1] https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:146: static bool isMessageLoopReady(); On 2016/10/18 19:39:11, haraken wrote: > > I want to avoid introducing this if possible... I was assuming that we should put wrapper of base::XXX into WebKit platform, to enable other parts {modules,web} etc. to use. Maybe this is not the most appropriate place,, WDYT? Thanks. https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebKit.h (right): https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/web/WebKit.h:50: // For production code it's sure that above 2 are already fullfilled before On 2016/10/18 14:34:48, leonhsl wrote: > On 2016/10/18 12:32:11, blundell wrote: > > This comment strengthens my belief that we should name this method > > initializeMojoForTesting(). > > Acknowledged. Done. Referring the naming practice of other APIs in this file, changed the name to initializeMojoForTest(). https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/web/WebKit.h:57: // stuff, which has been skipped by blink::initialize(). On 2016/10/18 19:39:11, haraken wrote: > > Hmm, would you summarize why you cannot just use initialize() to initialize > Mojo? > > We really want to centralize all initialization paths to WebKit::initialize(). > (I've done a lot of refactoring to centralize distributed initialization paths > before.) > Inside Blink, as about the mojo stuff that must be initialized along with Blink initialization, for now the only piece is our newly created TimezoneMonitorClient, its instantiation would do some mojo message pipe operations such as getting remote interface and binding local interface ptr, mojo edk init and a message loop for current thread are *necessary* to these operations. For production code, blink::initialize() would initiate TimezoneMonitorClient without problem, but for some test codes blink::initialize() would skip TimezoneMonitorClient initialization because message loop may be not ready yet, later, if message loop got ready and the test codes need the TimezoneMonitorClient, then it could call our newly provided API blink::initializeMojoForTest() to initialize TimezoneMonitorClient again.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/19 08:47:23, leonhsl wrote: > Uploaded new patch sets, would you PTAnL? Thanks. > > ps#12 : address comments > ps#13 : rebase only > ps#14 : remove all duplicated calls to mojo::edk::init(), because I've > centralized the mojo edk initialization into constructor of > TestBlinkWebUnitTestSupport, right before calling blink::initialize(). > > https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... > File content/test/test_blink_web_unit_test_support.cc (right): > > https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... > content/test/test_blink_web_unit_test_support.cc:128: // Initialize mojo firstly > to enable Blink initialization use it. > On 2016/10/18 14:34:48, leonhsl wrote: > > On 2016/10/18 12:32:11, blundell wrote: > > > nit: s/use/to use > > > > Acknowledged. > > Done. > > https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... > File media/blink/run_all_unittests.cc (right): > > https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... > media/blink/run_all_unittests.cc:89: // Initialize mojo firstly to enable Blink > initialization use it. > On 2016/10/18 14:34:48, leonhsl wrote: > > On 2016/10/18 12:32:11, blundell wrote: > > > ("to use" on all of these comments throughout this CL) > > > > Acknowledged. > > Done. > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/time_zone_monitor/OWNERS (left): > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/time_zone_monitor/OWNERS:1: > mailto:blundell@chromium.org > On 2016/10/18 14:34:48, leonhsl wrote: > > On 2016/10/18 12:32:11, blundell wrote: > > > Add mailto:mark@chromium.org please. > > > > Acknowledged. > > Done. > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp > (right): > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:71: > thread->postTask(BLINK_FROM_HERE, createCrossThreadTask( > On 2016/10/18 14:34:48, leonhsl wrote: > > On 2016/10/18 12:32:11, blundell wrote: > > > To be totally explicit about this issue: > > > > > > This behavior is different from what it was previously, as now this task > will > > be > > > posted to workerBackingThreads that don't own their platform thread. > nhiroki's > > > comment upthread (and crbug.com/655028) seem to indicate that the *new* > > behavior > > > is the correct one, and that the old one was incorrect. nhiroki@, am I > > > understanding you correctly there? > > > > > > If that is true, then this behavioral change should be called out in the CL > > > description. > > > > > > I would also call out the changes that you're needing to make to support > > > initialization of Mojo in Blink now. > > > > Acknowledged. > > Done. Updated cl description. > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/web/WebKit.cpp (right): > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/WebKit.cpp:85: if (Platform::isMessageLoopReady()) > On 2016/10/18 19:39:11, haraken wrote: > > On 2016/10/18 14:34:48, leonhsl wrote: > > > On 2016/10/18 12:32:11, blundell wrote: > > > > Nit: It seems like you don't need to add Platform::isMessageLoopReady() > > > because > > > > you can just use if (platform->currentThread()) as is done on line 89 > below. > > > > > > I tried (platform->currentThread()) before, and turned out that it does not > > > really indicate the existence of a message loop for current thread. I also > > tried > > > to plumbed down into its implementation but did not find any hints leading > to > > > base::MessageLoop::current(). Maybe haraken@ could help to comment about > this. > > > Thanks. > > > > I think Platform::currentThread() should work. Would you investigate a bit > more > > why it doesn't work? > > content::TestBlinkWebUnitTestSupport is an impl of blink::Platform, and its > constructor will create the 'current thread' at [1], before calling > blink::initialize(), while at this time point there may has no message loop > ready, many test case codes will create their own type of message loop at a very > late stage. > [1] > https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/Platform.h (right): > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > third_party/WebKit/public/platform/Platform.h:146: static bool > isMessageLoopReady(); > On 2016/10/18 19:39:11, haraken wrote: > > > > I want to avoid introducing this if possible... > > I was assuming that we should put wrapper of base::XXX into WebKit platform, to > enable other parts {modules,web} etc. to use. Maybe this is not the most > appropriate place,, WDYT? Thanks. > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > File third_party/WebKit/public/web/WebKit.h (right): > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > third_party/WebKit/public/web/WebKit.h:50: // For production code it's sure that > above 2 are already fullfilled before > On 2016/10/18 14:34:48, leonhsl wrote: > > On 2016/10/18 12:32:11, blundell wrote: > > > This comment strengthens my belief that we should name this method > > > initializeMojoForTesting(). > > > > Acknowledged. > > Done. Referring the naming practice of other APIs in this file, changed the name > to initializeMojoForTest(). > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > third_party/WebKit/public/web/WebKit.h:57: // stuff, which has been skipped by > blink::initialize(). > On 2016/10/18 19:39:11, haraken wrote: > > > > Hmm, would you summarize why you cannot just use initialize() to initialize > > Mojo? > > > > We really want to centralize all initialization paths to WebKit::initialize(). > > (I've done a lot of refactoring to centralize distributed initialization paths > > before.) > > > > Inside Blink, as about the mojo stuff that must be initialized along with Blink > initialization, for now the only piece is our newly created > TimezoneMonitorClient, its instantiation would do some mojo message pipe > operations such as getting remote interface and binding local interface ptr, > mojo edk init and a message loop for current thread are *necessary* to these > operations. > For production code, blink::initialize() would initiate TimezoneMonitorClient > without problem, but for some test codes blink::initialize() would skip > TimezoneMonitorClient initialization because message loop may be not ready yet, > later, if message loop got ready and the test codes need the > TimezoneMonitorClient, then it could call our newly provided API > blink::initializeMojoForTest() to initialize TimezoneMonitorClient again. Is it possible to refactor content::TestBlinkWebUnitTestSupport so that it initializes the message loop before calling blink::initialize()?
On 2016/10/19 10:50:12, haraken wrote: > On 2016/10/19 08:47:23, leonhsl wrote: > > Uploaded new patch sets, would you PTAnL? Thanks. > > > > ps#12 : address comments > > ps#13 : rebase only > > ps#14 : remove all duplicated calls to mojo::edk::init(), because I've > > centralized the mojo edk initialization into constructor of > > TestBlinkWebUnitTestSupport, right before calling blink::initialize(). > > > > > https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... > > File content/test/test_blink_web_unit_test_support.cc (right): > > > > > https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... > > content/test/test_blink_web_unit_test_support.cc:128: // Initialize mojo > firstly > > to enable Blink initialization use it. > > On 2016/10/18 14:34:48, leonhsl wrote: > > > On 2016/10/18 12:32:11, blundell wrote: > > > > nit: s/use/to use > > > > > > Acknowledged. > > > > Done. > > > > > https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... > > File media/blink/run_all_unittests.cc (right): > > > > > https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... > > media/blink/run_all_unittests.cc:89: // Initialize mojo firstly to enable > Blink > > initialization use it. > > On 2016/10/18 14:34:48, leonhsl wrote: > > > On 2016/10/18 12:32:11, blundell wrote: > > > > ("to use" on all of these comments throughout this CL) > > > > > > Acknowledged. > > > > Done. > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/time_zone_monitor/OWNERS (left): > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/time_zone_monitor/OWNERS:1: > > mailto:blundell@chromium.org > > On 2016/10/18 14:34:48, leonhsl wrote: > > > On 2016/10/18 12:32:11, blundell wrote: > > > > Add mailto:mark@chromium.org please. > > > > > > Acknowledged. > > > > Done. > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > File > > third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp > > (right): > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:71: > > thread->postTask(BLINK_FROM_HERE, createCrossThreadTask( > > On 2016/10/18 14:34:48, leonhsl wrote: > > > On 2016/10/18 12:32:11, blundell wrote: > > > > To be totally explicit about this issue: > > > > > > > > This behavior is different from what it was previously, as now this task > > will > > > be > > > > posted to workerBackingThreads that don't own their platform thread. > > nhiroki's > > > > comment upthread (and crbug.com/655028) seem to indicate that the *new* > > > behavior > > > > is the correct one, and that the old one was incorrect. nhiroki@, am I > > > > understanding you correctly there? > > > > > > > > If that is true, then this behavioral change should be called out in the > CL > > > > description. > > > > > > > > I would also call out the changes that you're needing to make to support > > > > initialization of Mojo in Blink now. > > > > > > Acknowledged. > > > > Done. Updated cl description. > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/web/WebKit.cpp (right): > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > third_party/WebKit/Source/web/WebKit.cpp:85: if > (Platform::isMessageLoopReady()) > > On 2016/10/18 19:39:11, haraken wrote: > > > On 2016/10/18 14:34:48, leonhsl wrote: > > > > On 2016/10/18 12:32:11, blundell wrote: > > > > > Nit: It seems like you don't need to add Platform::isMessageLoopReady() > > > > because > > > > > you can just use if (platform->currentThread()) as is done on line 89 > > below. > > > > > > > > I tried (platform->currentThread()) before, and turned out that it does > not > > > > really indicate the existence of a message loop for current thread. I also > > > tried > > > > to plumbed down into its implementation but did not find any hints leading > > to > > > > base::MessageLoop::current(). Maybe haraken@ could help to comment about > > this. > > > > Thanks. > > > > > > I think Platform::currentThread() should work. Would you investigate a bit > > more > > > why it doesn't work? > > > > content::TestBlinkWebUnitTestSupport is an impl of blink::Platform, and its > > constructor will create the 'current thread' at [1], before calling > > blink::initialize(), while at this time point there may has no message loop > > ready, many test case codes will create their own type of message loop at a > very > > late stage. > > [1] > > > https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > File third_party/WebKit/public/platform/Platform.h (right): > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > third_party/WebKit/public/platform/Platform.h:146: static bool > > isMessageLoopReady(); > > On 2016/10/18 19:39:11, haraken wrote: > > > > > > I want to avoid introducing this if possible... > > > > I was assuming that we should put wrapper of base::XXX into WebKit platform, > to > > enable other parts {modules,web} etc. to use. Maybe this is not the most > > appropriate place,, WDYT? Thanks. > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > File third_party/WebKit/public/web/WebKit.h (right): > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > third_party/WebKit/public/web/WebKit.h:50: // For production code it's sure > that > > above 2 are already fullfilled before > > On 2016/10/18 14:34:48, leonhsl wrote: > > > On 2016/10/18 12:32:11, blundell wrote: > > > > This comment strengthens my belief that we should name this method > > > > initializeMojoForTesting(). > > > > > > Acknowledged. > > > > Done. Referring the naming practice of other APIs in this file, changed the > name > > to initializeMojoForTest(). > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > third_party/WebKit/public/web/WebKit.h:57: // stuff, which has been skipped by > > blink::initialize(). > > On 2016/10/18 19:39:11, haraken wrote: > > > > > > Hmm, would you summarize why you cannot just use initialize() to initialize > > > Mojo? > > > > > > We really want to centralize all initialization paths to > WebKit::initialize(). > > > (I've done a lot of refactoring to centralize distributed initialization > paths > > > before.) > > > > > > > Inside Blink, as about the mojo stuff that must be initialized along with > Blink > > initialization, for now the only piece is our newly created > > TimezoneMonitorClient, its instantiation would do some mojo message pipe > > operations such as getting remote interface and binding local interface ptr, > > mojo edk init and a message loop for current thread are *necessary* to these > > operations. > > For production code, blink::initialize() would initiate TimezoneMonitorClient > > without problem, but for some test codes blink::initialize() would skip > > TimezoneMonitorClient initialization because message loop may be not ready > yet, > > later, if message loop got ready and the test codes need the > > TimezoneMonitorClient, then it could call our newly provided API > > blink::initializeMojoForTest() to initialize TimezoneMonitorClient again. > > Is it possible to refactor content::TestBlinkWebUnitTestSupport so that it > initializes the message loop before calling blink::initialize()? This CL is getting more and more complicated. I'd suggest fixing the initialization path before landing this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/19 10:51:50, haraken wrote: > On 2016/10/19 10:50:12, haraken wrote: > > On 2016/10/19 08:47:23, leonhsl wrote: > > > Uploaded new patch sets, would you PTAnL? Thanks. > > > > > > ps#12 : address comments > > > ps#13 : rebase only > > > ps#14 : remove all duplicated calls to mojo::edk::init(), because I've > > > centralized the mojo edk initialization into constructor of > > > TestBlinkWebUnitTestSupport, right before calling blink::initialize(). > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... > > > File content/test/test_blink_web_unit_test_support.cc (right): > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/content/test/test_blin... > > > content/test/test_blink_web_unit_test_support.cc:128: // Initialize mojo > > firstly > > > to enable Blink initialization use it. > > > On 2016/10/18 14:34:48, leonhsl wrote: > > > > On 2016/10/18 12:32:11, blundell wrote: > > > > > nit: s/use/to use > > > > > > > > Acknowledged. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... > > > File media/blink/run_all_unittests.cc (right): > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/media/blink/run_all_un... > > > media/blink/run_all_unittests.cc:89: // Initialize mojo firstly to enable > > Blink > > > initialization use it. > > > On 2016/10/18 14:34:48, leonhsl wrote: > > > > On 2016/10/18 12:32:11, blundell wrote: > > > > > ("to use" on all of these comments throughout this CL) > > > > > > > > Acknowledged. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/modules/time_zone_monitor/OWNERS (left): > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/time_zone_monitor/OWNERS:1: > > > mailto:blundell@chromium.org > > > On 2016/10/18 14:34:48, leonhsl wrote: > > > > On 2016/10/18 12:32:11, blundell wrote: > > > > > Add mailto:mark@chromium.org please. > > > > > > > > Acknowledged. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > > File > > > > third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > > > > > third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:71: > > > thread->postTask(BLINK_FROM_HERE, createCrossThreadTask( > > > On 2016/10/18 14:34:48, leonhsl wrote: > > > > On 2016/10/18 12:32:11, blundell wrote: > > > > > To be totally explicit about this issue: > > > > > > > > > > This behavior is different from what it was previously, as now this task > > > will > > > > be > > > > > posted to workerBackingThreads that don't own their platform thread. > > > nhiroki's > > > > > comment upthread (and crbug.com/655028) seem to indicate that the *new* > > > > behavior > > > > > is the correct one, and that the old one was incorrect. nhiroki@, am I > > > > > understanding you correctly there? > > > > > > > > > > If that is true, then this behavioral change should be called out in the > > CL > > > > > description. > > > > > > > > > > I would also call out the changes that you're needing to make to support > > > > > initialization of Mojo in Blink now. > > > > > > > > Acknowledged. > > > > > > Done. Updated cl description. > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/web/WebKit.cpp (right): > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/web/WebKit.cpp:85: if > > (Platform::isMessageLoopReady()) > > > On 2016/10/18 19:39:11, haraken wrote: > > > > On 2016/10/18 14:34:48, leonhsl wrote: > > > > > On 2016/10/18 12:32:11, blundell wrote: > > > > > > Nit: It seems like you don't need to add > Platform::isMessageLoopReady() > > > > > because > > > > > > you can just use if (platform->currentThread()) as is done on line 89 > > > below. > > > > > > > > > > I tried (platform->currentThread()) before, and turned out that it does > > not > > > > > really indicate the existence of a message loop for current thread. I > also > > > > tried > > > > > to plumbed down into its implementation but did not find any hints > leading > > > to > > > > > base::MessageLoop::current(). Maybe haraken@ could help to comment about > > > this. > > > > > Thanks. > > > > > > > > I think Platform::currentThread() should work. Would you investigate a bit > > > more > > > > why it doesn't work? > > > > > > content::TestBlinkWebUnitTestSupport is an impl of blink::Platform, and its > > > constructor will create the 'current thread' at [1], before calling > > > blink::initialize(), while at this time point there may has no message loop > > > ready, many test case codes will create their own type of message loop at a > > very > > > late stage. > > > [1] > > > > > > https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > > File third_party/WebKit/public/platform/Platform.h (right): > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > > third_party/WebKit/public/platform/Platform.h:146: static bool > > > isMessageLoopReady(); > > > On 2016/10/18 19:39:11, haraken wrote: > > > > > > > > I want to avoid introducing this if possible... > > > > > > I was assuming that we should put wrapper of base::XXX into WebKit platform, > > to > > > enable other parts {modules,web} etc. to use. Maybe this is not the most > > > appropriate place,, WDYT? Thanks. > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > > File third_party/WebKit/public/web/WebKit.h (right): > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > > third_party/WebKit/public/web/WebKit.h:50: // For production code it's sure > > that > > > above 2 are already fullfilled before > > > On 2016/10/18 14:34:48, leonhsl wrote: > > > > On 2016/10/18 12:32:11, blundell wrote: > > > > > This comment strengthens my belief that we should name this method > > > > > initializeMojoForTesting(). > > > > > > > > Acknowledged. > > > > > > Done. Referring the naming practice of other APIs in this file, changed the > > name > > > to initializeMojoForTest(). > > > > > > > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > > third_party/WebKit/public/web/WebKit.h:57: // stuff, which has been skipped > by > > > blink::initialize(). > > > On 2016/10/18 19:39:11, haraken wrote: > > > > > > > > Hmm, would you summarize why you cannot just use initialize() to > initialize > > > > Mojo? > > > > > > > > We really want to centralize all initialization paths to > > WebKit::initialize(). > > > > (I've done a lot of refactoring to centralize distributed initialization > > paths > > > > before.) > > > > > > > > > > Inside Blink, as about the mojo stuff that must be initialized along with > > Blink > > > initialization, for now the only piece is our newly created > > > TimezoneMonitorClient, its instantiation would do some mojo message pipe > > > operations such as getting remote interface and binding local interface ptr, > > > mojo edk init and a message loop for current thread are *necessary* to these > > > operations. > > > For production code, blink::initialize() would initiate > TimezoneMonitorClient > > > without problem, but for some test codes blink::initialize() would skip > > > TimezoneMonitorClient initialization because message loop may be not ready > > yet, > > > later, if message loop got ready and the test codes need the > > > TimezoneMonitorClient, then it could call our newly provided API > > > blink::initializeMojoForTest() to initialize TimezoneMonitorClient again. > > > > Is it possible to refactor content::TestBlinkWebUnitTestSupport so that it > > initializes the message loop before calling blink::initialize()? > > This CL is getting more and more complicated. I'd suggest fixing the > initialization path before landing this CL. I agree with haraken@ (I didn't know that there had been work to trim blink::initialize() down to one entrypoint :\). I suggest seeing if you can refactor content::TestBlinkWebUnitTestSupport in this CL to know that it works for usecase, and if you can get that to work, pull it out into its own CL and land that one first.
https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > > > third_party/WebKit/public/web/WebKit.h:57: // stuff, which has been > skipped > > by > > > > blink::initialize(). > > > > On 2016/10/18 19:39:11, haraken wrote: > > > > > > > > > > Hmm, would you summarize why you cannot just use initialize() to > > initialize > > > > > Mojo? > > > > > > > > > > We really want to centralize all initialization paths to > > > WebKit::initialize(). > > > > > (I've done a lot of refactoring to centralize distributed initialization > > > paths > > > > > before.) > > > > > > > > > > > > > Inside Blink, as about the mojo stuff that must be initialized along with > > > Blink > > > > initialization, for now the only piece is our newly created > > > > TimezoneMonitorClient, its instantiation would do some mojo message pipe > > > > operations such as getting remote interface and binding local interface > ptr, > > > > mojo edk init and a message loop for current thread are *necessary* to > these > > > > operations. > > > > For production code, blink::initialize() would initiate > > TimezoneMonitorClient > > > > without problem, but for some test codes blink::initialize() would skip > > > > TimezoneMonitorClient initialization because message loop may be not ready > > > yet, > > > > later, if message loop got ready and the test codes need the > > > > TimezoneMonitorClient, then it could call our newly provided API > > > > blink::initializeMojoForTest() to initialize TimezoneMonitorClient again. > > > > > > Is it possible to refactor content::TestBlinkWebUnitTestSupport so that it > > > initializes the message loop before calling blink::initialize()? > > > > This CL is getting more and more complicated. I'd suggest fixing the > > initialization path before landing this CL. > > I agree with haraken@ (I didn't know that there had been work to trim > blink::initialize() down to one entrypoint :\). I suggest seeing if you can > refactor content::TestBlinkWebUnitTestSupport in this CL to know that it works > for usecase, and if you can get that to work, pull it out into its own CL and > land that one first. Yeah I'm also interested in the initialization paths of Blink, trying to figure out more clearly how to handle content::TestBlinkWebUnitTestSupport gracefully.
On 2016/10/21 03:16:06, leonhsl wrote: > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > > > > third_party/WebKit/public/web/WebKit.h:57: // stuff, which has been > > skipped > > > by > > > > > blink::initialize(). > > > > > On 2016/10/18 19:39:11, haraken wrote: > > > > > > > > > > > > Hmm, would you summarize why you cannot just use initialize() to > > > initialize > > > > > > Mojo? > > > > > > > > > > > > We really want to centralize all initialization paths to > > > > WebKit::initialize(). > > > > > > (I've done a lot of refactoring to centralize distributed > initialization > > > > paths > > > > > > before.) > > > > > > > > > > > > > > > > Inside Blink, as about the mojo stuff that must be initialized along > with > > > > Blink > > > > > initialization, for now the only piece is our newly created > > > > > TimezoneMonitorClient, its instantiation would do some mojo message pipe > > > > > operations such as getting remote interface and binding local interface > > ptr, > > > > > mojo edk init and a message loop for current thread are *necessary* to > > these > > > > > operations. > > > > > For production code, blink::initialize() would initiate > > > TimezoneMonitorClient > > > > > without problem, but for some test codes blink::initialize() would skip > > > > > TimezoneMonitorClient initialization because message loop may be not > ready > > > > yet, > > > > > later, if message loop got ready and the test codes need the > > > > > TimezoneMonitorClient, then it could call our newly provided API > > > > > blink::initializeMojoForTest() to initialize TimezoneMonitorClient > again. > > > > > > > > Is it possible to refactor content::TestBlinkWebUnitTestSupport so that it > > > > initializes the message loop before calling blink::initialize()? > > > > > > This CL is getting more and more complicated. I'd suggest fixing the > > > initialization path before landing this CL. > > > > I agree with haraken@ (I didn't know that there had been work to trim > > blink::initialize() down to one entrypoint :\). I suggest seeing if you can > > refactor content::TestBlinkWebUnitTestSupport in this CL to know that it works > > for usecase, and if you can get that to work, pull it out into its own CL and > > land that one first. > > Yeah I'm also interested in the initialization paths of Blink, trying to figure > out more clearly how to handle content::TestBlinkWebUnitTestSupport gracefully. Hi, blundell@, haraken@, OK until now I can't thought out a good way to do this: Let TestBlinkWebUnitTestSupport prepare the message loop before calling blink::initialize(). The root cause has already been summarized at [1] ... So I think now I really need some suggestions from you,, WDYT? Thanks. [1] https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su...
On 2016/10/21 09:35:24, leonhsl wrote: > On 2016/10/21 03:16:06, leonhsl wrote: > > > https://codereview.chromium.org/2402983002/diff/260001/third_party/WebKit/pub... > > > > > > third_party/WebKit/public/web/WebKit.h:57: // stuff, which has been > > > skipped > > > > by > > > > > > blink::initialize(). > > > > > > On 2016/10/18 19:39:11, haraken wrote: > > > > > > > > > > > > > > Hmm, would you summarize why you cannot just use initialize() to > > > > initialize > > > > > > > Mojo? > > > > > > > > > > > > > > We really want to centralize all initialization paths to > > > > > WebKit::initialize(). > > > > > > > (I've done a lot of refactoring to centralize distributed > > initialization > > > > > paths > > > > > > > before.) > > > > > > > > > > > > > > > > > > > Inside Blink, as about the mojo stuff that must be initialized along > > with > > > > > Blink > > > > > > initialization, for now the only piece is our newly created > > > > > > TimezoneMonitorClient, its instantiation would do some mojo message > pipe > > > > > > operations such as getting remote interface and binding local > interface > > > ptr, > > > > > > mojo edk init and a message loop for current thread are *necessary* to > > > these > > > > > > operations. > > > > > > For production code, blink::initialize() would initiate > > > > TimezoneMonitorClient > > > > > > without problem, but for some test codes blink::initialize() would > skip > > > > > > TimezoneMonitorClient initialization because message loop may be not > > ready > > > > > yet, > > > > > > later, if message loop got ready and the test codes need the > > > > > > TimezoneMonitorClient, then it could call our newly provided API > > > > > > blink::initializeMojoForTest() to initialize TimezoneMonitorClient > > again. > > > > > > > > > > Is it possible to refactor content::TestBlinkWebUnitTestSupport so that > it > > > > > initializes the message loop before calling blink::initialize()? > > > > > > > > This CL is getting more and more complicated. I'd suggest fixing the > > > > initialization path before landing this CL. > > > > > > I agree with haraken@ (I didn't know that there had been work to trim > > > blink::initialize() down to one entrypoint :\). I suggest seeing if you can > > > refactor content::TestBlinkWebUnitTestSupport in this CL to know that it > works > > > for usecase, and if you can get that to work, pull it out into its own CL > and > > > land that one first. > > > > Yeah I'm also interested in the initialization paths of Blink, trying to > figure > > out more clearly how to handle content::TestBlinkWebUnitTestSupport > gracefully. > > Hi, blundell@, haraken@, > OK until now I can't thought out a good way to do this: Let > TestBlinkWebUnitTestSupport prepare the message loop before calling > blink::initialize(). The root cause has already been summarized at [1] ... So I > think now I really need some suggestions from you,, WDYT? Thanks. > > [1] > https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... It looks like ssid@ added the comment. ssid@: Do you have any idea? We want to make sure that the message loop is initialized before blink::initialize is called.
haraken@chromium.org changed reviewers: + ssid@chromium.org
+ssid
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
[TimeZoneMonitor] Decouple renderer side impl from content to blink.
In addition to the decoupling work, this CL:
* notifies time zone change to all the workerBackingThread, no matter they
do OWN their platform thread or not, while the old behavior is that only
those workerBackingThread OWNING a platform thread could get the
notifications, which is incorrect.
* introduces a new WebKit API blink::initializeMojoForTest(), clients of Blink
could call this API to initialize those mojo stuff which may have been
skipped by blink::initialize() because the preconditions (mojo edk and
message loop for current thread) are not ready at that time point.
This would never happen for production codes, but some test codes will hit
such case because they may create their own type of message loop *after*
blink::initialize().
BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
and then click "recheck" to ensure that the renderer picks up the new
time zone. Don't reload the page, which is likely to give you a new
renderer process, use the "recheck" link on the page.
==========
to
==========
[TimeZoneMonitor] Decouple renderer side impl from content to blink.
In addition to the decoupling work, this CL:
* notifies time zone change to all the workerBackingThread, no matter they
do OWN their platform thread or not, while the old behavior is that only
those workerBackingThread OWNING a platform thread could get the
notifications, which is incorrect.
* will initialize TimeZoneMonitorClient in blink::initialize() if a message
loop has already been created, this is always true for production code,
while some unit tests may have not prepared the message loop when
calling blink::initialize(), so TimeZoneMonitorClient won't be initialized.
BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
and then click "recheck" to ensure that the renderer picks up the new
time zone. Don't reload the page, which is likely to give you a new
renderer process, use the "recheck" link on the page.
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, blundell@, haraken@, would you PTAnL at ps#15? According with discussion in chromium-mojo@, do not introduce blink::initializeMojoForTest() for now.
https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.cpp:84: TimeZoneMonitorClient::Init(); I'd simply move this into initialize(). https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:35: TimeZoneMonitorClient& TimeZoneMonitorClient::Init() { The return value is unused. https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:36: DEFINE_STATIC_LOCAL(TimeZoneMonitorClient, instance, ()); Why do you need to use DEFINE_STATIC_LOCAL? https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:89: modulesInitializer().initializeMojo(); Can we move this into modulesInitializer().initialize()?
Thanks for review. https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.cpp:84: TimeZoneMonitorClient::Init(); On 2016/10/26 11:21:59, haraken wrote: > > I'd simply move this into initialize(). Acknowledged. https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:35: TimeZoneMonitorClient& TimeZoneMonitorClient::Init() { On 2016/10/26 11:21:59, haraken wrote: > > The return value is unused. Just want to create it as long-live singleton, and no need to hold its reference elsewhere. Any suggestion? https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:36: DEFINE_STATIC_LOCAL(TimeZoneMonitorClient, instance, ()); On 2016/10/26 11:21:59, haraken wrote: > > Why do you need to use DEFINE_STATIC_LOCAL? According your previous comment at: https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... Assuming it's better than bare 'static XXX* xxx;' https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:89: modulesInitializer().initializeMojo(); On 2016/10/26 11:21:59, haraken wrote: > > Can we move this into modulesInitializer().initialize()? Acknowledged.
https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:44: Platform::current()->interfaceProvider()->getInterface( I begin to wonder why you need to run these steps when initializing Blink... Other Mojo clients are calling AddClient later when the client gets actually constructed. Why does TimeZoneMonitorClient need to register itself to Mojo in the initialization phase? In other words, why do you need to instantiate TimeZoneMonitorClient so eagerly?
https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:44: Platform::current()->interfaceProvider()->getInterface( On 2016/10/26 12:02:12, haraken wrote: > > I begin to wonder why you need to run these steps when initializing Blink... > Other Mojo clients are calling AddClient later when the client gets actually > constructed. > > Why does TimeZoneMonitorClient need to register itself to Mojo in the > initialization phase? In other words, why do you need to instantiate > TimeZoneMonitorClient so eagerly? > Because TimeZoneMonitorClient needs to be able to get time zone change notifications from Browser, once Blink started. Originally AddClient() happened in RenderThreadImpl::Init(), right after initializing Blink. Maybe 'Other Mojo clients' constructions are triggered by javascript function call or some other code logic.
haraken@chromium.org changed reviewers: + dcheng@chromium.org
LGTM Sorry for being back and forth. This CL is doing a couple of weird things and it was hard to convince myself about it. Just in case, please wait for dcheng@'s review for the mojo part. https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:35: TimeZoneMonitorClient& TimeZoneMonitorClient::Init() { On 2016/10/26 11:45:48, leonhsl wrote: > On 2016/10/26 11:21:59, haraken wrote: > > > > The return value is unused. > > Just want to create it as long-live singleton, and no need to hold its reference > elsewhere. Any suggestion? Can you just change the return type to void? https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:36: DEFINE_STATIC_LOCAL(TimeZoneMonitorClient, instance, ()); On 2016/10/26 11:45:48, leonhsl wrote: > On 2016/10/26 11:21:59, haraken wrote: > > > > Why do you need to use DEFINE_STATIC_LOCAL? > > According your previous comment at: > https://codereview.chromium.org/2402983002/diff/180001/third_party/WebKit/Sou... > Assuming it's better than bare 'static XXX* xxx;' OK, you want to make it a singleton... Then DEFINE_STATIC_LOCAL makes sense. https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:44: Platform::current()->interfaceProvider()->getInterface( On 2016/10/26 12:13:00, leonhsl wrote: > On 2016/10/26 12:02:12, haraken wrote: > > > > I begin to wonder why you need to run these steps when initializing Blink... > > Other Mojo clients are calling AddClient later when the client gets actually > > constructed. > > > > Why does TimeZoneMonitorClient need to register itself to Mojo in the > > initialization phase? In other words, why do you need to instantiate > > TimeZoneMonitorClient so eagerly? > > > > Because TimeZoneMonitorClient needs to be able to get time zone change > notifications from Browser, once Blink started. Originally AddClient() happened > in RenderThreadImpl::Init(), right after initializing Blink. > Maybe 'Other Mojo clients' constructions are triggered by javascript function > call or some other code logic. ok
The TimeZoneMonitor changes look good. I'm inclined to separate out all of the Mojo test initialization changes into a separate CL if my understanding is correct that they're now not actually needed in this CL given the direction we took wrt initializing TimeZoneMonitor conditionally in blink::initialize(). That would tighten up this CL a lot, and then those changes could be examined separately. https://codereview.chromium.org/2402983002/diff/380001/chrome/test/base/run_a... File chrome/test/base/run_all_unittests.cc (left): https://codereview.chromium.org/2402983002/diff/380001/chrome/test/base/run_a... chrome/test/base/run_all_unittests.cc:17: mojo::edk::Init(); Are all the test changes around Mojo initialization still worth doing in this CL given the decision around it being OK to not initialize Mojo in blink::initialize()?
On 2016/10/26 18:08:05, blundell wrote: > The TimeZoneMonitor changes look good. I'm inclined to separate out all of the > Mojo test initialization changes into a separate CL if my understanding is > correct that they're now not actually needed in this CL given the direction we > took wrt initializing TimeZoneMonitor conditionally in blink::initialize(). That > would tighten up this CL a lot, and then those changes could be examined > separately. > > https://codereview.chromium.org/2402983002/diff/380001/chrome/test/base/run_a... > File chrome/test/base/run_all_unittests.cc (left): > > https://codereview.chromium.org/2402983002/diff/380001/chrome/test/base/run_a... > chrome/test/base/run_all_unittests.cc:17: mojo::edk::Init(); > Are all the test changes around Mojo initialization still worth doing in this CL > given the decision around it being OK to not initialize Mojo in > blink::initialize()? It's true that changes for all the tests employing TestBlinkWebUnitTestSupport can be separated out now, actually what we're doing here is to centralize mojo edk initialization into TestBlinkWebUnitTestSupport, instead of doing that in every places: unit_tests, content_unittests, components_unittests, extensions_unittests and blink unittests. But changes for content::RenderViewTest and media blink unit tests are still necessary here, because they have prepared message loop before calling blink::initialize() so TimeZoneMonitorClient initialization won't be skipped which means mojo edk initialization must be ready before that.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
leon.han@intel.com changed reviewers: - ssid@chromium.org
Firstly uploaded ps#17 to address nits from haraken@. Will separate unit tests changes unrelated with time zone monitor into another CL. https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.cpp:84: TimeZoneMonitorClient::Init(); On 2016/10/26 11:45:48, leonhsl wrote: > On 2016/10/26 11:21:59, haraken wrote: > > > > I'd simply move this into initialize(). > > Acknowledged. Done. https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:35: TimeZoneMonitorClient& TimeZoneMonitorClient::Init() { On 2016/10/26 13:27:11, haraken wrote: > On 2016/10/26 11:45:48, leonhsl wrote: > > On 2016/10/26 11:21:59, haraken wrote: > > > > > > The return value is unused. > > > > Just want to create it as long-live singleton, and no need to hold its > reference > > elsewhere. Any suggestion? > > Can you just change the return type to void? Done. https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2402983002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:89: modulesInitializer().initializeMojo(); On 2016/10/26 11:45:48, leonhsl wrote: > On 2016/10/26 11:21:59, haraken wrote: > > > > Can we move this into modulesInitializer().initialize()? > > Acknowledged. Done.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
leon.han@intel.com changed reviewers: + jam@chromium.org
blundell@: ps#18 has only necessary changes for time zone monitor, unrelated test changes have been separated into another CL https://codereview.chromium.org/2453933003. haraken@: ps#19 adds mojo edk init path into blink_heap_unittests, because before calling blink::initialize() it has message pipe ready(means TimeZoneMonitorClient will be initialized) but did not init mojo edk. +jam@: For overall OWNER review after both blundell@ and haraken@ are happy. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks! https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:48: if (m_binding.is_bound()) Do you expect this method to be called more than once?
https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/RunAllTests.cpp (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/RunAllTests.cpp:47: mojo::edk::Init(); Do we need this? I don't think platform/heap/*tests are using Mojo. If we need to add this here, I guess you also need to add it to other RunAllTests.cpp. https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:145: // blink::initialize() determining whether can initialize mojo stuff or not. Add a TODO to remove this public API by ensuring a message loop before calling blink::initialize().
On 2016/10/27 16:13:15, haraken wrote: > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/RunAllTests.cpp (right): > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/RunAllTests.cpp:47: mojo::edk::Init(); > > Do we need this? I don't think platform/heap/*tests are using Mojo. > > If we need to add this here, I guess you also need to add it to other > RunAllTests.cpp. > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/Platform.h (right): > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/pub... > third_party/WebKit/public/platform/Platform.h:145: // blink::initialize() > determining whether can initialize mojo stuff or not. > > Add a TODO to remove this public API by ensuring a message loop before calling > blink::initialize(). LGTM with the above comments addressed.
https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.cpp:53: // later after they got a message loop ready. Can we just fix the tests to instantiate a message loop? I'd prefer to not have test conditional logic outside tests where possible, and this seems pretty easy to do. https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:57: void TimeZoneMonitorClient::OnTimeZoneChange(const WTF::String& timeZoneInfo) { Nit: omit WTF::
https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.cpp:53: // later after they got a message loop ready. On 2016/10/27 22:34:50, dcheng wrote: > Can we just fix the tests to instantiate a message loop? I'd prefer to not have > test conditional logic outside tests where possible, and this seems pretty easy > to do. This is not easy. The problem is here: https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... Instantiating a MessageLoop there clashes with dozens (hundreds?) of tests across various suites that have the properties mentioned in that comment.
On 2016/10/27 22:37:44, blundell wrote: > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/ModulesInitializer.cpp:53: // later after they > got a message loop ready. > On 2016/10/27 22:34:50, dcheng wrote: > > Can we just fix the tests to instantiate a message loop? I'd prefer to not > have > > test conditional logic outside tests where possible, and this seems pretty > easy > > to do. > > This is not easy. The problem is here: > https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... > > Instantiating a MessageLoop there clashes with dozens (hundreds?) of tests > across various suites that have the properties mentioned in that comment. That said, it would be easy to have a distinct initializeWithoutMessageLoopForTests() entrypoint, and to only allow for the non-existence of a MessageLoop when entered from that entrypoint. That would eliminate the risk that this inadvertently causes silent failure in production code. Kentaro was not really in favor of that IIRC.
On 2016/10/27 22:48:39, blundell wrote: > On 2016/10/27 22:37:44, blundell wrote: > > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): > > > > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/ModulesInitializer.cpp:53: // later after > they > > got a message loop ready. > > On 2016/10/27 22:34:50, dcheng wrote: > > > Can we just fix the tests to instantiate a message loop? I'd prefer to not > > have > > > test conditional logic outside tests where possible, and this seems pretty > > easy > > > to do. > > > > This is not easy. The problem is here: > > > https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... > > > > Instantiating a MessageLoop there clashes with dozens (hundreds?) of tests > > across various suites that have the properties mentioned in that comment. > > That said, it would be easy to have a distinct > initializeWithoutMessageLoopForTests() entrypoint, and to only allow for the > non-existence of a MessageLoop when entered from that entrypoint. That would > eliminate the risk that this inadvertently causes silent failure in production > code. Kentaro was not really in favor of that IIRC. Is there anyone who's willing to work on fixing this? Doing this to unblock the CL is not great but also not unreasonable. But if we don't fix it, the next time we hit this, we'll just end up adding in yet another hack the next time we hit issues with this... 1. Does anyone know if it's possible to create / tear-down TestBlinkWebUnitTestSupport multiple times in the same process? 2. Do *all* the tests in UnitTestTestSuite really need a blink test environment? I imagine a lot of them do (e.g. the ones deriving from RenderViewTest), but I imagine a lot also don't.
On 2016/10/27 22:55:40, dcheng wrote: > On 2016/10/27 22:48:39, blundell wrote: > > On 2016/10/27 22:37:44, blundell wrote: > > > > > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): > > > > > > > > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/ModulesInitializer.cpp:53: // later after > > they > > > got a message loop ready. > > > On 2016/10/27 22:34:50, dcheng wrote: > > > > Can we just fix the tests to instantiate a message loop? I'd prefer to not > > > have > > > > test conditional logic outside tests where possible, and this seems pretty > > > easy > > > > to do. > > > > > > This is not easy. The problem is here: > > > > > > https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... > > > > > > Instantiating a MessageLoop there clashes with dozens (hundreds?) of tests > > > across various suites that have the properties mentioned in that comment. > > > > That said, it would be easy to have a distinct > > initializeWithoutMessageLoopForTests() entrypoint, and to only allow for the > > non-existence of a MessageLoop when entered from that entrypoint. That would > > eliminate the risk that this inadvertently causes silent failure in production > > code. Kentaro was not really in favor of that IIRC. > > Is there anyone who's willing to work on fixing this? Doing this to unblock the > CL is not great but also not unreasonable. What is the path to fixing this? If there's a test that (a) needs TestBlikWebUnitTestSupport and (b) needs a custom MessageLoop, what would we do? > > But if we don't fix it, the next time we hit this, we'll just end up adding in > yet another hack the next time we hit issues with this... > > 1. Does anyone know if it's possible to create / tear-down > TestBlinkWebUnitTestSupport multiple times in the same process? > 2. Do *all* the tests in UnitTestTestSuite really need a blink test environment? > I imagine a lot of them do (e.g. the ones deriving from RenderViewTest), but I > imagine a lot also don't.
On 2016/10/27 22:58:29, blundell wrote: > On 2016/10/27 22:55:40, dcheng wrote: > > On 2016/10/27 22:48:39, blundell wrote: > > > On 2016/10/27 22:37:44, blundell wrote: > > > > > > > > > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/modules/ModulesInitializer.cpp:53: // later > after > > > they > > > > got a message loop ready. > > > > On 2016/10/27 22:34:50, dcheng wrote: > > > > > Can we just fix the tests to instantiate a message loop? I'd prefer to > not > > > > have > > > > > test conditional logic outside tests where possible, and this seems > pretty > > > > easy > > > > > to do. > > > > > > > > This is not easy. The problem is here: > > > > > > > > > > https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... > > > > > > > > Instantiating a MessageLoop there clashes with dozens (hundreds?) of tests > > > > across various suites that have the properties mentioned in that comment. > > > > > > That said, it would be easy to have a distinct > > > initializeWithoutMessageLoopForTests() entrypoint, and to only allow for the > > > non-existence of a MessageLoop when entered from that entrypoint. That would > > > eliminate the risk that this inadvertently causes silent failure in > production > > > code. Kentaro was not really in favor of that IIRC. > > > > Is there anyone who's willing to work on fixing this? Doing this to unblock > the > > CL is not great but also not unreasonable. > > What is the path to fixing this? If there's a test that (a) needs > TestBlikWebUnitTestSupport > and (b) needs a custom MessageLoop, what would we do? Tests that need custom threads often instantiate the message loop or a TestBrowserThreadBundle themselves. If we could make tests that depend on Blink instantiate the Blink test support themselves, that would solve this, right? (This is assuming that it's actually safe to call blink::initialize() / blink::shutdown() multiple times) > > > > > But if we don't fix it, the next time we hit this, we'll just end up adding in > > yet another hack the next time we hit issues with this... > > > > 1. Does anyone know if it's possible to create / tear-down > > TestBlinkWebUnitTestSupport multiple times in the same process? > > 2. Do *all* the tests in UnitTestTestSuite really need a blink test > environment? > > I imagine a lot of them do (e.g. the ones deriving from RenderViewTest), but I > > imagine a lot also don't.
On 2016/10/27 23:13:22, dcheng wrote: > On 2016/10/27 22:58:29, blundell wrote: > > On 2016/10/27 22:55:40, dcheng wrote: > > > On 2016/10/27 22:48:39, blundell wrote: > > > > On 2016/10/27 22:37:44, blundell wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > > > > > File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/modules/ModulesInitializer.cpp:53: // later > > after > > > > they > > > > > got a message loop ready. > > > > > On 2016/10/27 22:34:50, dcheng wrote: > > > > > > Can we just fix the tests to instantiate a message loop? I'd prefer to > > not > > > > > have > > > > > > test conditional logic outside tests where possible, and this seems > > pretty > > > > > easy > > > > > > to do. > > > > > > > > > > This is not easy. The problem is here: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... > > > > > > > > > > Instantiating a MessageLoop there clashes with dozens (hundreds?) of > tests > > > > > across various suites that have the properties mentioned in that > comment. > > > > > > > > That said, it would be easy to have a distinct > > > > initializeWithoutMessageLoopForTests() entrypoint, and to only allow for > the > > > > non-existence of a MessageLoop when entered from that entrypoint. That > would > > > > eliminate the risk that this inadvertently causes silent failure in > > production > > > > code. Kentaro was not really in favor of that IIRC. > > > > > > Is there anyone who's willing to work on fixing this? Doing this to unblock > > the > > > CL is not great but also not unreasonable. > > > > What is the path to fixing this? If there's a test that (a) needs > > TestBlikWebUnitTestSupport > > and (b) needs a custom MessageLoop, what would we do? > > Tests that need custom threads often instantiate the message loop or a > TestBrowserThreadBundle themselves. > If we could make tests that depend on Blink instantiate the Blink test support > themselves, that would solve this, right? > > (This is assuming that it's actually safe to call blink::initialize() / > blink::shutdown() multiple times) blink::initialize() asserts that it's only called once. My preference would be to introduce the blink::initializeWithoutMessageLoopForTesting() codepath and call it a day. The code under question is being moved from RenderThreadImpl, which isn't even created in these test suites. So by definition it's not needed in the test suites. In general if a unittest needs a particular Mojo connection set up it can set that up explicitly. Unittests shouldn't need all of blink's Mojo connections implicitly set up since they're unittests. > > > > > > > > > But if we don't fix it, the next time we hit this, we'll just end up adding > in > > > yet another hack the next time we hit issues with this... > > > > > > 1. Does anyone know if it's possible to create / tear-down > > > TestBlinkWebUnitTestSupport multiple times in the same process? > > > 2. Do *all* the tests in UnitTestTestSuite really need a blink test > > environment? > > > I imagine a lot of them do (e.g. the ones deriving from RenderViewTest), but > I > > > imagine a lot also don't.
FWIW, we're pulling Leon back and forth for a long time on this CL about whether we should fix all unittests to initialize a message loop or we should work around it somehow. We should make a clear decision asap.
On 2016/10/28 01:55:47, haraken wrote: > FWIW, we're pulling Leon back and forth for a long time on this CL about whether > we should fix all unittests to initialize a message loop or we should work > around it somehow. We should make a clear decision asap. To be clear, I see that as somewhat out of scope of this CL. But we should try to fix this (outside of the CL); I'll start a thread on platform-architecture-dev.
On 2016/10/28 02:14:00, dcheng wrote: > On 2016/10/28 01:55:47, haraken wrote: > > FWIW, we're pulling Leon back and forth for a long time on this CL about > whether > > we should fix all unittests to initialize a message loop or we should work > > around it somehow. We should make a clear decision asap. > > To be clear, I see that as somewhat out of scope of this CL. > > But we should try to fix this (outside of the CL); I'll start a thread on > platform-architecture-dev. Thanks. I totally agree that we should fix the unittests at some point.
On 2016/10/28 02:16:10, haraken wrote: > On 2016/10/28 02:14:00, dcheng wrote: > > On 2016/10/28 01:55:47, haraken wrote: > > > FWIW, we're pulling Leon back and forth for a long time on this CL about > > whether > > > we should fix all unittests to initialize a message loop or we should work > > > around it somehow. We should make a clear decision asap. > > > > To be clear, I see that as somewhat out of scope of this CL. > > > > But we should try to fix this (outside of the CL); I'll start a thread on > > platform-architecture-dev. > > Thanks. I totally agree that we should fix the unittests at some point. A bit more context: - https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/2Z3W2aQqBZY/di... - https://cs.chromium.org/chromium/src/content/test/test_blink_web_unit_test_su... (It looks really weird that we're creating a dummy message loop before calling blink::initialize.)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks all for your time to review and all the discussions around the blink initialization path issue. I understand this CL has raised the "trouble" and we'll solve it later, I'll try to take part in the discussions later to see what I can help. # Thanks Hara-san for kindly consideration :) https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:48: if (m_binding.is_bound()) On 2016/10/27 15:31:19, blundell wrote: > Do you expect this method to be called more than once? This should happen only once, so I added the guard, maybe should change to use DCHECK? Actually I created TimeZoneMonitorClient::Bind() function because I must USE the 'instance' created by DEFINE_STATIC_LOCAL at line36 in TimeZoneMonitorClient::Init(), otherwise compiler will warn(error) 'unused variable' there.. Seems weird,, but I have no other good ideas to handle this well. Any suggestion? https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:57: void TimeZoneMonitorClient::OnTimeZoneChange(const WTF::String& timeZoneInfo) { On 2016/10/27 22:34:50, dcheng wrote: > Nit: omit WTF:: Done. https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/RunAllTests.cpp (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/RunAllTests.cpp:47: mojo::edk::Init(); On 2016/10/27 16:13:15, haraken wrote: > > Do we need this? I don't think platform/heap/*tests are using Mojo. > > If we need to add this here, I guess you also need to add it to other > RunAllTests.cpp. At line42 in this one RunAllTests.cpp, it is calling content::SetUpBlinkTestEnvironment(), which will create a message loop and then call blink::initialize(), which will initialize the mojo stuff(TimeZoneMonitorClient), this means mojo edk MUST be ready before that. For other 2 RunAllTests.cpp hitting such case, they've already had the mojo edk init path. They're: third_party/WebKit/Source/platform/testing/BlinkFuzzerTestSupport.cpp and third_party/WebKit/Source/web/tests/RunAllTests.cpp. Further more, because https://codereview.chromium.org/2453933003 has been landed, mojo edk init path has been centralized, this CL does not need to add mojo::edk::Init() here. Reverted these changes for blink heap unit tests by ps#20. https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:145: // blink::initialize() determining whether can initialize mojo stuff or not. On 2016/10/27 16:13:15, haraken wrote: > > Add a TODO to remove this public API by ensuring a message loop before calling > blink::initialize(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:48: if (m_binding.is_bound()) On 2016/10/28 05:59:06, leonhsl wrote: > On 2016/10/27 15:31:19, blundell wrote: > > Do you expect this method to be called more than once? > > This should happen only once, so I added the guard, maybe should change to use > DCHECK? > Actually I created TimeZoneMonitorClient::Bind() function because I must USE the > 'instance' created by DEFINE_STATIC_LOCAL at line36 in > TimeZoneMonitorClient::Init(), otherwise compiler will warn(error) 'unused > variable' there.. Seems weird,, but I have no other good ideas to handle this > well. Any suggestion? You could presumably just inline the code and use instance.m_binding instead of m_binding?
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:48: if (m_binding.is_bound()) On 2016/10/28 14:36:13, blundell wrote: > On 2016/10/28 05:59:06, leonhsl wrote: > > On 2016/10/27 15:31:19, blundell wrote: > > > Do you expect this method to be called more than once? > > > > This should happen only once, so I added the guard, maybe should change to use > > DCHECK? > > Actually I created TimeZoneMonitorClient::Bind() function because I must USE > the > > 'instance' created by DEFINE_STATIC_LOCAL at line36 in > > TimeZoneMonitorClient::Init(), otherwise compiler will warn(error) 'unused > > variable' there.. Seems weird,, but I have no other good ideas to handle this > > well. Any suggestion? > > You could presumably just inline the code and use instance.m_binding instead of > m_binding? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Daniel, how can we move forward here? Thanks!
Well I'm currently reviewing again now since it was the weekend when Leon last replied =) Daniel On Mon, Oct 31, 2016 at 4:36 PM <blundell@chromium.org> wrote: > Daniel, how can we move forward here? Thanks! > > https://codereview.chromium.org/2402983002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Well I'm currently reviewing again now since it was the weekend when Leon last replied =) Daniel On Mon, Oct 31, 2016 at 4:36 PM <blundell@chromium.org> wrote: > Daniel, how can we move forward here? Thanks! > > https://codereview.chromium.org/2402983002/ > -- 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.
(I generally try to turn around code reviews within 24 business hours, but if something is higher priority, please let me know. Thanks!) LGTM with an optional nit https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp (right): https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:41: monitor->AddClient(instance.m_binding.CreateInterfacePtrAndBind()); Nit: Personal preference here, but I prefer that we do all the initialization in the ctor, by something like this: device::mojom::blink::TimeZoneMonitorClientPtr clientPtr; DEFINE_STATIC_LOCAL(TimeZoneMonitorClient, instance, (&clientPtr)); Since I'm not a fan of the constructor leaving an object partially initialized if it's not necessary.
On 2016/10/31 23:37:41, blink-reviews wrote: > Well I'm currently reviewing again now since it was the weekend when Leon > last replied =) > > Daniel > My apologies, I had mistakenly thought that we were still blocked on deciding what to do re: blink:initialize() and the lack of a MessageLoop, and I had been waiting for your followup input there -- that's what my question was about. I hadn't realize that your previous comment had decided that question for this CL. Apologies for the crossed wires and what must have come across as snippiness!
Ping jam@, would you PTAL for //content OWNER review? Thanks.
leon.han@intel.com changed reviewers: + kinuko@chromium.org - jam@chromium.org
Hi, kinuko@, would you PTAL for //content OWNER review? Thanks.
https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.cpp:54: if (Platform::isMessageLoopReady()) Huh this is sad :( (and read all the threads about this) https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:148: static bool isMessageLoopReady(); (Putting aside the need of this method that seems to have discussed so long) do we need this method to be exposed at Platform? This seems to be only used in Source/modules/ModulesInitializer.cpp or my text search is broken. Could this be placed, say, in platform/mojo/MojoHelper.h? (It's orthogonal to mojo but we wouldn't want to make it stick around for general use either)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:148: static bool isMessageLoopReady(); On 2016/11/08 03:22:56, kinuko wrote: > (Putting aside the need of this method that seems to have discussed so long) > > do we need this method to be exposed at Platform? This seems to be only used in > Source/modules/ModulesInitializer.cpp or my text search is broken. Could this > be placed, say, in platform/mojo/MojoHelper.h? (It's orthogonal to mojo but we > wouldn't want to make it stick around for general use either) Yes the only user is Source/modules/ModulesInitializer.cpp. I'm sure ModulesInitializer.cpp is able to call Platform public APIs so I put the API here. And, I tried to put isMessageLoopReady() definition into platform/mojo/MojoHelper.h, but got build error as bellow. Any ideas? Thanks. " In file included from ../../third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:18: ../../third_party/WebKit/Source/platform/mojo/MojoHelper.h:36:15: error: unused function 'isMessageLoopReady' [-Werror,-Wunused-function] static bool isMessageLoopReady() { "
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/08 08:20:39, leonhsl wrote: > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/Platform.h (right): > > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > third_party/WebKit/public/platform/Platform.h:148: static bool > isMessageLoopReady(); > On 2016/11/08 03:22:56, kinuko wrote: > > (Putting aside the need of this method that seems to have discussed so long) > > > > do we need this method to be exposed at Platform? This seems to be only used > in > > Source/modules/ModulesInitializer.cpp or my text search is broken. Could this > > be placed, say, in platform/mojo/MojoHelper.h? (It's orthogonal to mojo but > we > > wouldn't want to make it stick around for general use either) > > Yes the only user is Source/modules/ModulesInitializer.cpp. > I'm sure ModulesInitializer.cpp is able to call Platform public APIs so I put > the API here. > > And, I tried to put isMessageLoopReady() definition into > platform/mojo/MojoHelper.h, but got build error as bellow. Any ideas? Thanks. > > " > In file included from > ../../third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:18: > ../../third_party/WebKit/Source/platform/mojo/MojoHelper.h:36:15: error: unused > function 'isMessageLoopReady' [-Werror,-Wunused-function] > static bool isMessageLoopReady() { > " Where were you putting includes of platform/mojo/MojoHelper.h so that it got included in ImageCapture.cpp? It seems like it should only need to be included in ModulesInitializer.cpp.
On 2016/11/08 08:37:40, blundell wrote: > On 2016/11/08 08:20:39, leonhsl wrote: > > > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > > File third_party/WebKit/public/platform/Platform.h (right): > > > > > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > > third_party/WebKit/public/platform/Platform.h:148: static bool > > isMessageLoopReady(); > > On 2016/11/08 03:22:56, kinuko wrote: > > > (Putting aside the need of this method that seems to have discussed so long) > > > > > > do we need this method to be exposed at Platform? This seems to be only > used > > in > > > Source/modules/ModulesInitializer.cpp or my text search is broken. Could > this > > > be placed, say, in platform/mojo/MojoHelper.h? (It's orthogonal to mojo but > > we > > > wouldn't want to make it stick around for general use either) > > > > Yes the only user is Source/modules/ModulesInitializer.cpp. > > I'm sure ModulesInitializer.cpp is able to call Platform public APIs so I put > > the API here. > > > > And, I tried to put isMessageLoopReady() definition into > > platform/mojo/MojoHelper.h, but got build error as bellow. Any ideas? Thanks. > > > > " > > In file included from > > ../../third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:18: > > ../../third_party/WebKit/Source/platform/mojo/MojoHelper.h:36:15: error: > unused > > function 'isMessageLoopReady' [-Werror,-Wunused-function] > > static bool isMessageLoopReady() { > > " > > Where were you putting includes of platform/mojo/MojoHelper.h so that it got > included in ImageCapture.cpp? It seems like it should only need to be included > in ModulesInitializer.cpp. ImageCapture.cpp itself is including platform/mojo/MojoHelper.h. And another several files are also doing so.
On 2016/11/08 08:49:26, leonhsl wrote: > On 2016/11/08 08:37:40, blundell wrote: > > On 2016/11/08 08:20:39, leonhsl wrote: > > > > > > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > > > File third_party/WebKit/public/platform/Platform.h (right): > > > > > > > > > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > > > third_party/WebKit/public/platform/Platform.h:148: static bool > > > isMessageLoopReady(); > > > On 2016/11/08 03:22:56, kinuko wrote: > > > > (Putting aside the need of this method that seems to have discussed so > long) > > > > > > > > do we need this method to be exposed at Platform? This seems to be only > > used > > > in > > > > Source/modules/ModulesInitializer.cpp or my text search is broken. Could > > this > > > > be placed, say, in platform/mojo/MojoHelper.h? (It's orthogonal to mojo > but > > > we > > > > wouldn't want to make it stick around for general use either) > > > > > > Yes the only user is Source/modules/ModulesInitializer.cpp. > > > I'm sure ModulesInitializer.cpp is able to call Platform public APIs so I > put > > > the API here. > > > > > > And, I tried to put isMessageLoopReady() definition into > > > platform/mojo/MojoHelper.h, but got build error as bellow. Any ideas? > Thanks. > > > > > > " > > > In file included from > > > ../../third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:18: > > > ../../third_party/WebKit/Source/platform/mojo/MojoHelper.h:36:15: error: > > unused > > > function 'isMessageLoopReady' [-Werror,-Wunused-function] > > > static bool isMessageLoopReady() { > > > " > > > > Where were you putting includes of platform/mojo/MojoHelper.h so that it got > > included in ImageCapture.cpp? It seems like it should only need to be included > > in ModulesInitializer.cpp. > > ImageCapture.cpp itself is including platform/mojo/MojoHelper.h. And another > several files are also doing so. Oops, I should have looked. I had assumed that MojoHelper.h was a new file that Kinuko was suggesting creating. Kinuko/Kentaro, is there a known convention for working around this issue? Note that if we're putting this function in MojoHelper.h I would call it something like canInitializeMojo() rather than isMessageLoopReady().
On 2016/11/08 08:52:44, blundell wrote: > On 2016/11/08 08:49:26, leonhsl wrote: > > On 2016/11/08 08:37:40, blundell wrote: > > > On 2016/11/08 08:20:39, leonhsl wrote: > > > > > > > > > > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > > > > File third_party/WebKit/public/platform/Platform.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > > > > third_party/WebKit/public/platform/Platform.h:148: static bool > > > > isMessageLoopReady(); > > > > On 2016/11/08 03:22:56, kinuko wrote: > > > > > (Putting aside the need of this method that seems to have discussed so > > long) > > > > > > > > > > do we need this method to be exposed at Platform? This seems to be only > > > used > > > > in > > > > > Source/modules/ModulesInitializer.cpp or my text search is broken. > Could > > > this > > > > > be placed, say, in platform/mojo/MojoHelper.h? (It's orthogonal to mojo > > but > > > > we > > > > > wouldn't want to make it stick around for general use either) > > > > > > > > Yes the only user is Source/modules/ModulesInitializer.cpp. > > > > I'm sure ModulesInitializer.cpp is able to call Platform public APIs so I > > put > > > > the API here. > > > > > > > > And, I tried to put isMessageLoopReady() definition into > > > > platform/mojo/MojoHelper.h, but got build error as bellow. Any ideas? > > Thanks. > > > > > > > > " > > > > In file included from > > > > ../../third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:18: > > > > ../../third_party/WebKit/Source/platform/mojo/MojoHelper.h:36:15: error: > > > unused > > > > function 'isMessageLoopReady' [-Werror,-Wunused-function] > > > > static bool isMessageLoopReady() { > > > > " > > > > > > Where were you putting includes of platform/mojo/MojoHelper.h so that it got > > > included in ImageCapture.cpp? It seems like it should only need to be > included > > > in ModulesInitializer.cpp. > > > > ImageCapture.cpp itself is including platform/mojo/MojoHelper.h. And another > > several files are also doing so. > > Oops, I should have looked. I had assumed that MojoHelper.h was a new file that > Kinuko was suggesting creating. Kinuko/Kentaro, is there a known convention for > working around this issue? > > Note that if we're putting this function in MojoHelper.h I would call it > something like canInitializeMojo() rather than isMessageLoopReady(). Remove the static keyword and it should compile in MojoHelpers.h
On 2016/11/08 09:07:39, dcheng wrote: > On 2016/11/08 08:52:44, blundell wrote: > > On 2016/11/08 08:49:26, leonhsl wrote: > > > On 2016/11/08 08:37:40, blundell wrote: > > > > On 2016/11/08 08:20:39, leonhsl wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > > > > > File third_party/WebKit/public/platform/Platform.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2402983002/diff/480001/third_party/WebKit/pub... > > > > > third_party/WebKit/public/platform/Platform.h:148: static bool > > > > > isMessageLoopReady(); > > > > > On 2016/11/08 03:22:56, kinuko wrote: > > > > > > (Putting aside the need of this method that seems to have discussed so > > > long) > > > > > > > > > > > > do we need this method to be exposed at Platform? This seems to be > only > > > > used > > > > > in > > > > > > Source/modules/ModulesInitializer.cpp or my text search is broken. > > Could > > > > this > > > > > > be placed, say, in platform/mojo/MojoHelper.h? (It's orthogonal to > mojo > > > but > > > > > we > > > > > > wouldn't want to make it stick around for general use either) > > > > > > > > > > Yes the only user is Source/modules/ModulesInitializer.cpp. > > > > > I'm sure ModulesInitializer.cpp is able to call Platform public APIs so > I > > > put > > > > > the API here. > > > > > > > > > > And, I tried to put isMessageLoopReady() definition into > > > > > platform/mojo/MojoHelper.h, but got build error as bellow. Any ideas? > > > Thanks. > > > > > > > > > > " > > > > > In file included from > > > > > > ../../third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:18: > > > > > ../../third_party/WebKit/Source/platform/mojo/MojoHelper.h:36:15: error: > > > > unused > > > > > function 'isMessageLoopReady' [-Werror,-Wunused-function] > > > > > static bool isMessageLoopReady() { > > > > > " > > > > > > > > Where were you putting includes of platform/mojo/MojoHelper.h so that it > got > > > > included in ImageCapture.cpp? It seems like it should only need to be > > included > > > > in ModulesInitializer.cpp. > > > > > > ImageCapture.cpp itself is including platform/mojo/MojoHelper.h. And another > > > several files are also doing so. > > > > Oops, I should have looked. I had assumed that MojoHelper.h was a new file > that > > Kinuko was suggesting creating. Kinuko/Kentaro, is there a known convention > for > > working around this issue? > > > > Note that if we're putting this function in MojoHelper.h I would call it > > something like canInitializeMojo() rather than isMessageLoopReady(). > > Remove the static keyword and it should compile in MojoHelpers.h Ah,, sorry! Got it.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded ps#23, PTAnL, Thanks.
initialization related stuff still gives me a bit weird feeling but lgtm for content changes, thanks. https://codereview.chromium.org/2402983002/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/mojo/MojoHelper.h (right): https://codereview.chromium.org/2402983002/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/MojoHelper.h:39: } nit: // namespace blink
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Thanks all. Landing. https://codereview.chromium.org/2402983002/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/mojo/MojoHelper.h (right): https://codereview.chromium.org/2402983002/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/MojoHelper.h:39: } On 2016/11/08 11:35:12, kinuko wrote: > nit: // namespace blink Done.
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org, haraken@chromium.org, blundell@chromium.org, dcheng@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2402983002/#ps540001 (title: "Address nit and rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from
==========
[TimeZoneMonitor] Decouple renderer side impl from content to blink.
In addition to the decoupling work, this CL:
* notifies time zone change to all the workerBackingThread, no matter they
do OWN their platform thread or not, while the old behavior is that only
those workerBackingThread OWNING a platform thread could get the
notifications, which is incorrect.
* will initialize TimeZoneMonitorClient in blink::initialize() if a message
loop has already been created, this is always true for production code,
while some unit tests may have not prepared the message loop when
calling blink::initialize(), so TimeZoneMonitorClient won't be initialized.
BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
and then click "recheck" to ensure that the renderer picks up the new
time zone. Don't reload the page, which is likely to give you a new
renderer process, use the "recheck" link on the page.
==========
to
==========
[TimeZoneMonitor] Decouple renderer side impl from content to blink.
In addition to the decoupling work, this CL:
* notifies time zone change to all the workerBackingThread, no matter they
do OWN their platform thread or not, while the old behavior is that only
those workerBackingThread OWNING a platform thread could get the
notifications, which is incorrect.
* will initialize TimeZoneMonitorClient in blink::initialize() if a message
loop has already been created, this is always true for production code,
while some unit tests may have not prepared the message loop when
calling blink::initialize(), so TimeZoneMonitorClient won't be initialized.
TBR=xhwang@chromium.org,rockot@chromium.org
BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
and then click "recheck" to ensure that the renderer picks up the new
time zone. Don't reload the page, which is likely to give you a new
renderer process, use the "recheck" link on the page.
==========
Let me TBR and land, Thanks! xhwang@ for trivial changes for //media/blink/ rockot@ for adding dependency on //mojo/edk into //media/blink/DEPS
Let me TBR and land, Thanks! xhwang@ for trivial changes for //media/blink/ rockot@ for adding dependency on //mojo/edk into //media/blink/DEPS
leon.han@intel.com changed reviewers: + rockot@chromium.org, xhwang@chromium.org
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org, kinuko@chromium.org, dcheng@chromium.org, haraken@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2402983002/#ps560001 (title: "Rebase only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #25 (id:560001)
Message was sent while issue was closed.
Description was changed from
==========
[TimeZoneMonitor] Decouple renderer side impl from content to blink.
In addition to the decoupling work, this CL:
* notifies time zone change to all the workerBackingThread, no matter they
do OWN their platform thread or not, while the old behavior is that only
those workerBackingThread OWNING a platform thread could get the
notifications, which is incorrect.
* will initialize TimeZoneMonitorClient in blink::initialize() if a message
loop has already been created, this is always true for production code,
while some unit tests may have not prepared the message loop when
calling blink::initialize(), so TimeZoneMonitorClient won't be initialized.
TBR=xhwang@chromium.org,rockot@chromium.org
BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
and then click "recheck" to ensure that the renderer picks up the new
time zone. Don't reload the page, which is likely to give you a new
renderer process, use the "recheck" link on the page.
==========
to
==========
[TimeZoneMonitor] Decouple renderer side impl from content to blink.
In addition to the decoupling work, this CL:
* notifies time zone change to all the workerBackingThread, no matter they
do OWN their platform thread or not, while the old behavior is that only
those workerBackingThread OWNING a platform thread could get the
notifications, which is incorrect.
* will initialize TimeZoneMonitorClient in blink::initialize() if a message
loop has already been created, this is always true for production code,
while some unit tests may have not prepared the message loop when
calling blink::initialize(), so TimeZoneMonitorClient won't be initialized.
TBR=xhwang@chromium.org,rockot@chromium.org
BUG=612341
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
and then click "recheck" to ensure that the renderer picks up the new
time zone. Don't reload the page, which is likely to give you a new
renderer process, use the "recheck" link on the page.
Committed: https://crrev.com/7a219524da03c684d4ca6367caca55bbcf79dbab
Cr-Commit-Position: refs/heads/master@{#430857}
==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/7a219524da03c684d4ca6367caca55bbcf79dbab Cr-Commit-Position: refs/heads/master@{#430857} |
