New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug

Blocking:
issue 898307
issue 890978



Sign in to add a comment
link

Issue 826465: Migrate WebThreadImpl et al. to match new BrowserThreadImpl et al.

Reported by gab@chromium.org, Mar 27 2018 Project Member

Issue description

//content just went through a major cleanup in this space

Refactor : https://chromium-review.googlesource.com/c/chromium/src/+/980793
Remove BrowserThread lock: https://chromium-review.googlesource.com/c/chromium/src/+/973556

The same can be applied to WebThreads but this is too big of a change for me to copy/paste in without the ability to compile.

@sdefresne to perform this migration, thanks!

When this is done, I'll proceed with cleanup of base::Thread::SetMessageLoop which will no longer be used :)
 

Comment 1 by bugdroid1@chromium.org, Oct 31

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35cc3303536984d35616f1bfaf6c664be6fc0b5e

commit 35cc3303536984d35616f1bfaf6c664be6fc0b5e
Author: Alexander Timin <altimin@chromium.org>
Date: Wed Oct 31 18:54:49 2018

[message_loop] Remove implicit cast from MessageLoopCurrent to MessageLoop*.

Remove implicit cast and convert relevant callsites to work around it.
Add hash, ostream and comparison operators to MLC.

TBR=gab@chromium.org
R=gab@chromium.org
CC=​​​​alexclarke@chromium.org
BUG=891670,826465

Change-Id: I04cda773496e1690160fb7b4236f6db9ab8c7d38
Reviewed-on: https://chromium-review.googlesource.com/c/1292886
Commit-Queue: Alexander Timin <altimin@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604354}
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/message_loop/message_loop.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/message_loop/message_loop.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/message_loop/message_loop_current.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/message_loop/message_loop_current.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/message_loop/message_loop_task_runner_unittest.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/message_loop/message_pump_perftest.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/task/sequence_manager/sequence_manager_impl.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/task/sequence_manager/test/lazy_thread_controller_for_test.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/threading/thread.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/trace_event/trace_log.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/base/trace_event/trace_log.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/chrome/browser/io_thread.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/chrome/browser/metrics/thread_watcher.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/components/cronet/android/cronet_library_loader.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/components/cronet/android/test/cronet_test_util.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/components/cronet/native/perftest/perf_test.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/components/discardable_memory/service/discardable_shared_memory_manager.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/components/discardable_memory/service/discardable_shared_memory_manager.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/components/exo/wayland/clients/test/run_all_client_perftests.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/components/exo/wayland/clients/test/wayland_client_test.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/components/exo/wayland/clients/test/wayland_client_test.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/content/renderer/media/stream/webmediaplayer_ms_compositor.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/content/renderer/media/stream/webmediaplayer_ms_compositor.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/device/bluetooth/bluez/bluetooth_bluez_unittest.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/gpu/ipc/service/gpu_watchdog_thread.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/gpu/ipc/service/gpu_watchdog_thread.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/ios/web/app/web_main_loop.mm
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/ios/web/public/global_state/ios_global_state.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/ios/web/public/global_state/ios_global_state.mm
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/ios/web/test/test_web_thread_bundle.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/ios/web/web_thread_impl.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/media/cdm/library_cdm/clear_key_cdm/cdm_video_decoder.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/media/renderers/video_renderer_impl_unittest.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/net/cert/cert_database.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/net/cert/cert_database_mac.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/net/cookies/cookie_store_unittest.h
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/net/dns/serial_worker_unittest.cc
[modify] https://crrev.com/35cc3303536984d35616f1bfaf6c664be6fc0b5e/ui/gl/gl_context_cgl.cc

Comment 2 by gab@chromium.org, Nov 12

Cc: alexclarke@chromium.org altimin@chromium.org rohitrao@chromium.org
Labels: -Pri-2 -M-67 FoundIn-67 Target-72 Pri-1
Hi Sylvain, //ios lagging behind here is now blocking https://chromium-review.googlesource.com/c/chromium/src/+/1320329 and follow-up important refactors in //base.

Not having https://chromium-review.googlesource.com/c/chromium/src/+/973556 on the //ios side means it suffers from  issue 821034  (and thus  issue 890978  as well).

What's the ios team's strategy here? I find it unfortunate to have this copy/pasted code from //content for all the threading code... could it not have been made into a component like other shared code? Apart from the naming of UI/IO thread enums, there isn't anything particularly //content specific about this code...

It being in an ios specific section of the codebase means we can't reasonably keep it up to date as the majority of us can't even compile ios locally so clearly won't engage in a refactoring...

Comment 3 by gab@chromium.org, Nov 12

Blocking: 898307
Move to a component would also facilitate issue 898307

Comment 4 by gab@chromium.org, Nov 12

Blocking: 890978
Doing this blocks getting the benefits of https://chromium-review.googlesource.com/973556 on iOS to fix  issue 821034  and hence  issue 	890978  there.

Comment 5 by gab@chromium.org, Nov 12

Owner: rohitrao@chromium.org
Spoke with Rohit offline, he will be picking this up.

Thanks Rohit!

Comment 6 by bugdroid1@chromium.org, Nov 15

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8532e551ec71212df79dc674a8cee0dc1f306d73

commit 8532e551ec71212df79dc674a8cee0dc1f306d73
Author: Rohit Rao <rohitrao@chromium.org>
Date: Thu Nov 15 13:12:35 2018

[ios] Clarifies that WebThread::SetDelegate can only be called for the IO thread.

This is the //web counterpart to https://codereview.chromium.org/2558943002.

BUG=826465

Change-Id: I83e09e189ad0cbbdccaf9fa0de104efa7a6386a3
Reviewed-on: https://chromium-review.googlesource.com/c/1333898
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608342}
[modify] https://crrev.com/8532e551ec71212df79dc674a8cee0dc1f306d73/ios/components/io_thread/ios_io_thread.mm
[modify] https://crrev.com/8532e551ec71212df79dc674a8cee0dc1f306d73/ios/web/public/web_thread.h
[modify] https://crrev.com/8532e551ec71212df79dc674a8cee0dc1f306d73/ios/web/public/web_thread_delegate.h
[modify] https://crrev.com/8532e551ec71212df79dc674a8cee0dc1f306d73/ios/web/web_thread_impl.cc

Comment 8 by bugdroid1@chromium.org, Dec 13

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d3f6bf113b71a9d0d2a259152581a5d65f76a22

commit 3d3f6bf113b71a9d0d2a259152581a5d65f76a22
Author: Rohit Rao <rohitrao@chromium.org>
Date: Thu Dec 13 15:07:50 2018

[ios] Store TaskRunners instead of threads in WebThreadImpl.

Track thread states explicitly instead of simply checking
whether a TaskRunner pointer exists or not.

BUG=826465

Change-Id: Id0f84e7eedc6b9f6568aec629abbb86f46979efa
Reviewed-on: https://chromium-review.googlesource.com/c/1340827
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616319}
[modify] https://crrev.com/3d3f6bf113b71a9d0d2a259152581a5d65f76a22/ios/web/test/test_web_thread_bundle.cc
[modify] https://crrev.com/3d3f6bf113b71a9d0d2a259152581a5d65f76a22/ios/web/web_thread_impl.cc
[modify] https://crrev.com/3d3f6bf113b71a9d0d2a259152581a5d65f76a22/ios/web/web_thread_impl.h

Comment 9 by bugdroid1@chromium.org, Dec 13

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be41fab665818d4184f70143c39f6caed80364fc

commit be41fab665818d4184f70143c39f6caed80364fc
Author: Javier Ernesto Flores Robles <javierrobles@chromium.org>
Date: Thu Dec 13 17:27:17 2018

Revert "[ios] Store TaskRunners instead of threads in WebThreadImpl."

This reverts commit 3d3f6bf113b71a9d0d2a259152581a5d65f76a22.

Reason for revert: This broke ios_chrome_unittest, more info in 
 crbug.com/914869 

Original change's description:
> [ios] Store TaskRunners instead of threads in WebThreadImpl.
> 
> Track thread states explicitly instead of simply checking
> whether a TaskRunner pointer exists or not.
> 
> BUG=826465
> 
> Change-Id: Id0f84e7eedc6b9f6568aec629abbb86f46979efa
> Reviewed-on: https://chromium-review.googlesource.com/c/1340827
> Reviewed-by: Rohit Rao <rohitrao@chromium.org>
> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Commit-Queue: Rohit Rao <rohitrao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616319}

TBR=rohitrao@chromium.org,gab@chromium.org,sdefresne@chromium.org

Change-Id: I1297465d60f4b3b5a76882b5e723080c9376a7dc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 826465
Reviewed-on: https://chromium-review.googlesource.com/c/1375890
Reviewed-by: Javier Ernesto Flores Robles <javierrobles@chromium.org>
Commit-Queue: Javier Ernesto Flores Robles <javierrobles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616348}
[modify] https://crrev.com/be41fab665818d4184f70143c39f6caed80364fc/ios/web/test/test_web_thread_bundle.cc
[modify] https://crrev.com/be41fab665818d4184f70143c39f6caed80364fc/ios/web/web_thread_impl.cc
[modify] https://crrev.com/be41fab665818d4184f70143c39f6caed80364fc/ios/web/web_thread_impl.h

Comment 10 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e07395847275faf4e81181dce9e1474f5f85b4d1

commit e07395847275faf4e81181dce9e1474f5f85b4d1
Author: Rohit Rao <rohitrao@chromium.org>
Date: Fri Dec 14 17:28:31 2018

Reland "[ios] Store TaskRunners instead of threads in WebThreadImpl."

This is a reland of 3d3f6bf113b71a9d0d2a259152581a5d65f76a22

The call to ResetGlobalsForTesting() has been moved from
TestWebThreadBundle to TestWebThread, as some unittests instantiate
TestWebThreads directly.

Original change's description:
> [ios] Store TaskRunners instead of threads in WebThreadImpl.
>
> Track thread states explicitly instead of simply checking
> whether a TaskRunner pointer exists or not.
>
> BUG=826465
>
> Change-Id: Id0f84e7eedc6b9f6568aec629abbb86f46979efa
> Reviewed-on: https://chromium-review.googlesource.com/c/1340827
> Reviewed-by: Rohit Rao <rohitrao@chromium.org>
> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Commit-Queue: Rohit Rao <rohitrao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616319}

Bug: 826465, 914869 
Change-Id: I522b1bb46464a17ade84a20857ee7db2cb713a8d
Reviewed-on: https://chromium-review.googlesource.com/c/1377320
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616723}
[modify] https://crrev.com/e07395847275faf4e81181dce9e1474f5f85b4d1/ios/web/public/test/test_web_thread.h
[modify] https://crrev.com/e07395847275faf4e81181dce9e1474f5f85b4d1/ios/web/test/test_web_thread.cc
[modify] https://crrev.com/e07395847275faf4e81181dce9e1474f5f85b4d1/ios/web/web_thread_impl.cc
[modify] https://crrev.com/e07395847275faf4e81181dce9e1474f5f85b4d1/ios/web/web_thread_impl.h

Comment 11 by bugdroid1@chromium.org, Dec 22

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/752eeba72e3136a4d81cfbcff56d6dffd1338ac0

commit 752eeba72e3136a4d81cfbcff56d6dffd1338ac0
Author: Rohit Rao <rohitrao@chromium.org>
Date: Sat Dec 22 19:28:08 2018

Refactor WebThreadImpl, WebSubThread, and WebMainLoop.

WebThreadImpl is now merely a scoped object that binds a provided
SingleThreadTaskRunner to a WebThread::ID. It no longer subclasses
base::Thread. That functionality has been moved to WebSubThread, which is
effectively only used for the IO thread now.

This is the //web implementation of
https://chromium-review.googlesource.com/c/chromium/src/+/980793. See the
description of that CL for more details.

BUG=826465

Change-Id: I636051b154c4156bc32517d649081131f9bdd14d
Reviewed-on: https://chromium-review.googlesource.com/c/1383140
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618761}
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/base/threading/thread_restrictions.h
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/components/io_thread/DEPS
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/components/io_thread/ios_io_thread_unittest.mm
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/app/web_main_loop.h
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/app/web_main_loop.mm
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/public/test/test_web_thread.h
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/public/web_thread.h
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/public/web_thread_delegate.h
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/test/test_web_thread.cc
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/web_sub_thread.cc
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/web_sub_thread.h
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/web_thread_impl.cc
[modify] https://crrev.com/752eeba72e3136a4d81cfbcff56d6dffd1338ac0/ios/web/web_thread_impl.h

Comment 12 by gab@chromium.org, Feb 20 (3 days ago)

Any updates here? Looks like we're only missing https://chromium-review.googlesource.com/973556 and I'd expect iOS is susceptible to  issue 821034  and similarly  issue 890978  without that CL (that second issue was about fixing the MessageLoop lock but iOS isn't benefiting from that gain if it still has the BrowserThread lock on top of it all).

Comment 13 by bugdroid, Feb 21 (2 days ago)

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17cf8347c6d375bade4d15f46cac4feca5310799

commit 17cf8347c6d375bade4d15f46cac4feca5310799
Author: Alex Clarke <alexclarke@chromium.org>
Date: Thu Feb 21 19:29:48 2019

Remove unused external message loop support from base::Thread

The callsites were removed in crrev.com/618761.

Bug: 826465
Change-Id: If1b36e9b95b83183653bf5c927d8bdd8dbee7959
Reviewed-on: https://chromium-review.googlesource.com/c/1478893
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634300}
[modify] https://crrev.com/17cf8347c6d375bade4d15f46cac4feca5310799/base/threading/thread.cc
[modify] https://crrev.com/17cf8347c6d375bade4d15f46cac4feca5310799/base/threading/thread.h
[modify] https://crrev.com/17cf8347c6d375bade4d15f46cac4feca5310799/base/threading/thread_unittest.cc

Sign in to add a comment