Issue metadata
Sign in to add a comment
|
Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader |
||||||||||||||||||
Issue description
,
Aug 22
,
Sep 7
Taking this
,
Oct 18
Agreed with svillar offline to take over the work here.
,
Oct 19
FTR: https://crrev.com/c/1289849 is up for review.
,
Oct 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c7e36b09013c046e265d70dfed0ca40b73bc23da commit c7e36b09013c046e265d70dfed0ca40b73bc23da Author: Antonio Gomes <tonikitoo@igalia.com> Date: Sun Oct 21 03:54:44 2018 Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader URLFetcher will stop working with advent of Network Service, and SimpleURLLoader is the replacement API for most clients. This CL migrates chromedriver and its respective unittests away from URLFetcher. Note that because of the multithreaded nature of the code, the natural approach was to simply call SharedURLLoaderFactory::Clone and pass the "info" mojo handle accross threads, until the operational thread. However, the structure holding this mojo handle is called/passed from (i) an indirect chain of BindRepeating calls, (ii) various threads. Given that network::mojom::URLLoaderFactoryPtrInfo and network::SharedURLLoaderFactoryInfo are both move-only object (which does not comply with BingRepeating) and that raw a network::SharedURLoaderLoader instance must operate on the same thread it was created on, this CL comes up with a local thread-agnostic "wrapper" to mojom::URLLoaderFactory, named WrapperURLLoaderFactory (see chrome/test/chromedriver/server/http_handler.cc). Its instance replaces URLRequestContextGetter instead across threads and BindRepeating chains just fine. This CL is based on the original work of Sergio Villar (svillar@igalia.com) on [1]. TBR=johnchen@chromium.org (John +1'ed the original incarnation of this CL at [1]). [1] https://crrev.com/c/1221256 Bug: 872887 Change-Id: I21386edb04b25438ba3aa40dd5ecc1461342aecf Reviewed-on: https://chromium-review.googlesource.com/c/1289849 Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Reviewed-by: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#601422} [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/BUILD.gn [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/chrome/devtools_http_client.cc [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/chrome/devtools_http_client.h [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/chrome_launcher.cc [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/chrome_launcher.h [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/log_replay/replay_http_client.cc [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/log_replay/replay_http_client.h [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/net/net_util.cc [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/net/net_util.h [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/net/net_util_unittest.cc [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/server/chromedriver_server.cc [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/server/http_handler.cc [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/server/http_handler.h [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/session_commands.cc [modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/session_commands.h
,
Oct 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/557c93c3070e306ea9b685632dddc7c8c8fd8d4b commit 557c93c3070e306ea9b685632dddc7c8c8fd8d4b Author: Findit <findit-for-me@appspot.gserviceaccount.com> Date: Sun Oct 21 05:02:08 2018 Revert "Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader" This reverts commit c7e36b09013c046e265d70dfed0ca40b73bc23da. Reason for revert: Findit (https://goo.gl/kROfz5) identified CL at revision 601422 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2M3ZTM2YjA5MDEzYzA0NmUyNjVkNzBkZmVkMGNhNDBiNzNiYzIzZGEM Sample Failed Build: https://ci.chromium.org/buildbot/chromium/mac-dbg/1239 Sample Failed Step: compile Original change's description: > Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader > > URLFetcher will stop working with advent of Network Service, and > SimpleURLLoader is the replacement API for most clients. > This CL migrates chromedriver and its respective unittests away from > URLFetcher. > > Note that because of the multithreaded nature of the code, the natural > approach was to simply call SharedURLLoaderFactory::Clone and pass > the "info" mojo handle accross threads, until the operational thread. > However, the structure holding this mojo handle is called/passed from > > (i) an indirect chain of BindRepeating calls, > (ii) various threads. > > Given that network::mojom::URLLoaderFactoryPtrInfo and > network::SharedURLLoaderFactoryInfo are both move-only object > (which does not comply with BingRepeating) and that raw > a network::SharedURLoaderLoader instance must operate on the same > thread it was created on, this CL comes up with a local thread-agnostic > "wrapper" to mojom::URLLoaderFactory, named WrapperURLLoaderFactory > (see chrome/test/chromedriver/server/http_handler.cc). > Its instance replaces URLRequestContextGetter instead across threads > and BindRepeating chains just fine. > > This CL is based on the original work of Sergio Villar (svillar@igalia.com) > on [1]. > > TBR=johnchen@chromium.org (John +1'ed the original incarnation of this CL at [1]). > > [1] https://crrev.com/c/1221256 > > Bug: 872887 > > Change-Id: I21386edb04b25438ba3aa40dd5ecc1461342aecf > Reviewed-on: https://chromium-review.googlesource.com/c/1289849 > Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> > Reviewed-by: Matt Menke <mmenke@chromium.org> > Cr-Commit-Position: refs/heads/master@{#601422} Change-Id: I0bb5f07fde222dacb7e34892d907458c8b8535a5 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 872887 Reviewed-on: https://chromium-review.googlesource.com/c/1293041 Cr-Commit-Position: refs/heads/master@{#601430} [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/BUILD.gn [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/chrome/devtools_http_client.cc [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/chrome/devtools_http_client.h [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/chrome_launcher.cc [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/chrome_launcher.h [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/log_replay/replay_http_client.cc [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/log_replay/replay_http_client.h [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/net/net_util.cc [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/net/net_util.h [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/net/net_util_unittest.cc [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/server/chromedriver_server.cc [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/server/http_handler.cc [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/server/http_handler.h [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/session_commands.cc [modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/session_commands.h
,
Oct 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/930cc098af1abb8e09065037de9dc3938d24e858 commit 930cc098af1abb8e09065037de9dc3938d24e858 Author: Antonio Gomes <tonikitoo@igalia.com> Date: Mon Oct 22 04:59:46 2018 Reland "Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader" This is a reland of c7e36b09013c046e265d70dfed0ca40b73bc23da The difference between this CL and its original version (https://crrev.com/c/c1289849) is that dependencies are made more explicit in chrome/test/chromedriver/BUILD.gn. TBR=mmenke@chromium.org,johnchen@chromium.org (John +1'ed the original incarnation of this CL at [1]). Original change's description: > Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader > > URLFetcher will stop working with advent of Network Service, and > SimpleURLLoader is the replacement API for most clients. > This CL migrates chromedriver and its respective unittests away from > URLFetcher. > > Note that because of the multithreaded nature of the code, the natural > approach was to simply call SharedURLLoaderFactory::Clone and pass > the "info" mojo handle accross threads, until the operational thread. > However, the structure holding this mojo handle is called/passed from > > (i) an indirect chain of BindRepeating calls, > (ii) various threads. > > Given that network::mojom::URLLoaderFactoryPtrInfo and > network::SharedURLLoaderFactoryInfo are both move-only object > (which does not comply with BingRepeating) and that raw > a network::SharedURLoaderLoader instance must operate on the same > thread it was created on, this CL comes up with a local thread-agnostic > "wrapper" to mojom::URLLoaderFactory, named WrapperURLLoaderFactory > (see chrome/test/chromedriver/server/http_handler.cc). > Its instance replaces URLRequestContextGetter instead across threads > and BindRepeating chains just fine. > > This CL is based on the original work of Sergio Villar (svillar@igalia.com) > on [1]. > > TBR=johnchen@chromium.org (John +1'ed the original incarnation of this CL at [1]). > > [1] https://crrev.com/c/1221256 > > Bug: 872887 > > Change-Id: I21386edb04b25438ba3aa40dd5ecc1461342aecf > Reviewed-on: https://chromium-review.googlesource.com/c/1289849 > Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> > Reviewed-by: Matt Menke <mmenke@chromium.org> > Cr-Commit-Position: refs/heads/master@{#601422} Bug: 872887 Change-Id: I645a34d28cfb92c15f2f2db409e0fe7cb7a5ac6b Reviewed-on: https://chromium-review.googlesource.com/c/1292933 Reviewed-by: Antonio Gomes <tonikitoo@igalia.com> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Cr-Commit-Position: refs/heads/master@{#601478} [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/BUILD.gn [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/chrome/devtools_http_client.cc [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/chrome/devtools_http_client.h [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/chrome_launcher.cc [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/chrome_launcher.h [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/log_replay/replay_http_client.cc [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/log_replay/replay_http_client.h [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/net/net_util.cc [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/net/net_util.h [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/net/net_util_unittest.cc [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/server/chromedriver_server.cc [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/server/http_handler.cc [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/server/http_handler.h [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/session_commands.cc [modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/session_commands.h
,
Oct 23
|
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by dxie@chromium.org
, Aug 9