net_unittests has a dependency on Blink |
||||||||
Issue description//net:net_unittests --[private]--> //net:net_with_v8 --[private]--> //tools/v8_context_snapshot:v8_context_snapshot --[public]--> //tools/v8_context_snapshot:generate_v8_context_snapshot --[private]--> //tools/v8_context_snapshot:v8_context_snapshot_generator --[private]--> //third_party/WebKit/public:blink This wasn't the case before sometime in the last few days, and it makes net_unittests build time a lot slower. The dependency is via v8 but I don't know the right v8 component for this.
,
Dec 12 2017
+peria@: could you take a look? Is this dependency introduced in https://chromium-review.googlesource.com/c/chromium/src/+/810347 ?
,
Dec 12 2017
Beyond the build-time issue, making net depend on blink isn't a good idea - blink depends on net/ (Though not, I guess, on net_with_v8), and we don't want that sort of circular dependency.
,
Dec 12 2017
We could move the v8-related classes over to services/proxy_resolver (Though they do have direct consumers elsewhere, I believe), but having that depend on blink is also not great.
,
Dec 15 2017
Setting to P0 for developer productivity.
,
Dec 19 2017
Changing status to 'Assigned' to get on peria's radar.
,
Dec 19 2017
,
Dec 19 2017
Looks like peria@ didn't set an OOO message, but is out until 1/5 Setting back to untriaged so it shows up in the Blink>JS queue
,
Dec 20 2017
I'm sorry to be slow. I think it is possible to revert the change. I'll revert it in a few days, but I'm happy if someone does it instead. from a mobile phone
,
Dec 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f84dd03781e2df91f343e3474a65e05edd3f7ac7 commit f84dd03781e2df91f343e3474a65e05edd3f7ac7 Author: Hitoshi Yoshida <peria@chromium.org> Date: Wed Dec 20 08:03:17 2017 Revert "net: Make net/ work with v8_context_snapshot.bin" This reverts commit f07e903f08d16bf8cdf7fac86ae50f4d9b2d7177. Reason for revert: This CL makes net/ depend on Blink, and it makes build time of net/ very long. Original change's description: > net: Make net/ work with v8_context_snapshot.bin > > We are planning to replace snapshot_blob.bin with v8_context_snapshot.bin. > This CL makes net/ code to load v8_context_snapshot.bin to work with it. > > Bug: 789964 , 764576 > Change-Id: Ifcbe58d632dd76fd4bcc02182044d1e111f4267a > Reviewed-on: https://chromium-review.googlesource.com/810347 > Reviewed-by: Matt Menke <mmenke@chromium.org> > Commit-Queue: Hitoshi Yoshida <peria@chromium.org> > Cr-Commit-Position: refs/heads/master@{#522648} TBR=peria@chromium.org,mmenke@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 789964 , 764576, 794234 Change-Id: I1e92c339914a6e2b1f33d280e4b41207213688b9 Reviewed-on: https://chromium-review.googlesource.com/836247 Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Commit-Queue: Hitoshi Yoshida <peria@chromium.org> Cr-Commit-Position: refs/heads/master@{#525286} [modify] https://crrev.com/f84dd03781e2df91f343e3474a65e05edd3f7ac7/net/BUILD.gn [modify] https://crrev.com/f84dd03781e2df91f343e3474a65e05edd3f7ac7/net/proxy/proxy_resolver_v8.cc
,
Dec 20 2017
The revert in #11 should fix the issue itself. I'll work for introducing the new snapshot without introducing heavy dependencies to unit tests. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by csharrison@chromium.org
, Dec 12 2017