Consider using std::unique_ptr for blink::ResourceRequest and blink::ResourceResponse |
|||||
Issue descriptionWe guess there are unnecessary copies happening causing some performance issues. Consider making them again std::unique_ptr wrapped (originally they were OwnPtr wrapped a while ago).
,
Nov 22 2017
,
Nov 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d0d92c5d1765633b60e397df001cad5894935467 commit d0d92c5d1765633b60e397df001cad5894935467 Author: Takashi Toyoshima <toyoshim@chromium.org> Date: Tue Nov 28 08:12:29 2017 Move redirect ResourceRequest creation code to ResourceRequest ResourceLoader had a code that constructs a new ResourceRequest for a redirect, and setup properties based on the last ResourceRequest. This code will be broken easily when people add new items to ResourceRequest because it's hard to notice that we also need to copy the new items even in ResourceLoader for redirection. This patch moves the creation code to ResourceRequest so that people who are going to change ResourceRequest can easily notice the requirement. Bug: 736308, 787704 Change-Id: I605b4c20f14b02fc5418878aa329687dce3638a4 Reviewed-on: https://chromium-review.googlesource.com/776407 Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org> Cr-Commit-Position: refs/heads/master@{#519604} [modify] https://crrev.com/d0d92c5d1765633b60e397df001cad5894935467/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp [modify] https://crrev.com/d0d92c5d1765633b60e397df001cad5894935467/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp [modify] https://crrev.com/d0d92c5d1765633b60e397df001cad5894935467/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.h
,
Dec 13 2017
How about RefCounted or GarbageCollectedFinalized? My context is, for example: https://codereview.chromium.org/2724673002/ Resource can be revalidated, and its users (e.g. ScriptResourceClients) might want to keep the old ResourceResponse before revalidation, for a while even after a new ResourceResponse is received during revalidation, because they don't want to be affected by revalidations. In the CL above, ScriptResourceData copies ResourceResponse from Resource on NotifyFinished(), and ScriptResource's users uses the ResourceResponse in ScriptResourceData. When ResourceResponse can be shared, then ScriptResource and ScriptResourceData can share the ResourceResponse, instead of copying. The CL above is not committed due to other blocking issues, but I have similar motiviation in some other places e.g. for ImageResource.
,
Dec 13 2017
,
Dec 19 2017
I haven't understood all of your changes in the CL above and may miss something, but could you just move the ownership of ResourceResponse to ScriptResourceData? Since ScriptResource seems to own the ScriptResourceData, it's possible to access the ResourceResponse that ScriptResourceData owns only when it is necessary?
,
Feb 15 2018
,
Mar 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f7f9493df845d725f34cbf50e2437456d3be3bea commit f7f9493df845d725f34cbf50e2437456d3be3bea Author: Takashi Toyoshima <toyoshim@chromium.org> Date: Tue Mar 27 08:32:58 2018 OOR-CORS: Simplify DocumentThreadableLoader's preflight request creation code Now DocumentThreadableLoader uses Blink API types to construct a CORS-preflight request, but this isn't necessary. Also, this change prefer using std::unique_ptr for ResourceRequest so to respect the direction of crbug.com/787704. Bug: 803766 , 787704 Change-Id: I897b17b7ea20e47316c05436c7d36438e03f2d7d Reviewed-on: https://chromium-review.googlesource.com/964074 Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#546062} [modify] https://crrev.com/f7f9493df845d725f34cbf50e2437456d3be3bea/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp [modify] https://crrev.com/f7f9493df845d725f34cbf50e2437456d3be3bea/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h [modify] https://crrev.com/f7f9493df845d725f34cbf50e2437456d3be3bea/third_party/WebKit/Source/core/loader/DocumentThreadableLoaderTest.cpp |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tyoshino@chromium.org
, Nov 22 2017