Replacing wtf types in WebCORS and WebCORSPreflightResultCache [chromium/src : master]
Daniel Hintze (Gerrit)
Hey,
this CL is supposed to fix the dependency rule violations pointed out by @kinuko.
Please take a look :-)
Thanks!
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Takeshi Yoshino (Gerrit)
Thanks! Yeah, this is better as we're moving out of Blink :)
1 comment:
File third_party/WebKit/public/platform/WebString.h:
Patch Set #3, Line 252:
} // namespace stdPlease take a look at the section about unordered_set in https://chromium-cpp.appspot.com/.
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Takeshi Yoshino (Gerrit)
Patch Set 3:
(1 comment)
Thanks! Yeah, this is better as we're moving out of Blink :)
Clarification: I meant it's better than my proposal of enclosing INSIDE_BLINK.
Kinuko Yasuda (Gerrit)
2 comments:
File third_party/WebKit/Source/core/exported/WebAssociatedURLLoaderImpl.cpp:
Patch Set #3, Line 232:
exposed_headers.find(header.key) == exposed_headers.end()))Thanks for making these changes, but this change also makes me feel slightly uncomfortable, as currently all these consumers seem to be in Blink. We're shuffling things around so we'll probably need to temporarily introduce a bit of uncommon patterns, but I'm still trying to figure out what transition could be smoother. Sorry for being a unclear on these, I'll try to be specific here (Takeshi, Mike-- if anything I write is not compatible with your thinking please let me know, we can also have a quick VC / chat if it works better).
Here are concerns: 1) for the things in core/ and modules/ using std containers is a bit unusual (highly discouraged actually) and 2) defining multiple new WebString utilities for this transition feels slightly wasteful given that the current state is transient (and WebString is not what we'll be using forever).
The easiest, minimal change to fix the current status would be to start with adding INSIDE_BLINK in the header file, and just keep the Blink-side callsites as is. But I suppose that in the next few changes we'll also want to include these header files outside Blink too-- then we'll need to make these work there too.
Below are some patterns that feel a little more common to me, you could probably mix some of these case by case:
1) just use std:: and base:: types only, without mixing them up with blink types like WebString. Consumers in Blink would need to do some type conversions when it calls CORS code but the usage would look similar to how we use, say, //base or //net code in Blink. I think for HTTPHeader{Set,Map} we might eventually need to take this path.
2) or, just don't expose implementation details in the header file if they are not necessarily accessed by the consumers. For example for the private members in WebCORSPreflightResultCacheItem we really don't need to expose them to the consumers. You can probably use pimpl pattern (e.g. just declare an inner class in the header file and have it with std::unique_ptr, and implement the class within the .cpp file).
3) or, have two types of implementations via template and/or ifdef's (INSIDE_BLINK and else), one with std:: and base:: for the code outside Blink and the other with WTF:: types for the code within Blink. This'd likely introduce code bloat and could make things less readable, but there might be cases this works better (not sure if it applies anything in this CL).
File third_party/WebKit/public/platform/WebCORS.h:
Patch Set #3, Line 44:
typedef std::unordered_set<WebString,nit: using
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Takeshi Yoshino (Gerrit)
1 comment:
Patch Set #3, Line 232:
exposed_headers.find(header.key) == exposed_headers.end()))
Thanks for making these changes, but this change also makes me feel slightly uncomfortable, as curre […]
Regarding the concern (1), I was feeling it's at acceptable level given we're in rapid re-architecturing era. IIUC, we've been using WTF types in Source/ for some real reason such as code consistency, benefit of PartitionAlloc based allocation, etc. Except for ones that are relatively less important during the big transition, it looks the only real disadvantage is that we're introducing conversion. But as we're going to use //base and stl soon (in WebKit/common/), it happens anyway. We're just doing them a bit earlier.
I agree with the concern (2). Yes, it's going to go away and we should avoid it if possible.
I think the solution (1) and (2) (looks over killing. just INSIDE_BLINK looks enough) are both reasonable. (3) looks ok as temporary solution, but sounds not so worth compared to (1) and (2).
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
1 comment:
Patch Set #3, Line 232:
exposed_headers.find(header.key) == exposed_headers.end()))
Regarding the concern (1), I was feeling it's at acceptable level given we're in rapid re-architectu […]
Hey, thank you both for the elaborate discussion and sorry for making you feel uncomfortable! :-)
I'd like to emphasize that for my project to come to an reasonable end state within my remaining two weeks, I need to be able to use WebCORS from content/child/loader/CORSURLLoader and it is my understanding that content/child does not qualify as being INSIDE_BLINK(?).
As for the solutions, considering that I am literally counting the days that I have left to finish, I would appreciate something that wouldn't take me (i.e. a green intern) too much time or could be polished later once CORSURLLoader and friends are landed.
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
Meanwhile fixed the smaller issued pointed out so far.
2 comments:
File third_party/WebKit/public/platform/WebCORS.h:
Patch Set #3, Line 44:
using HTTPHeaderSet = std::unordered_set<WebString,nit: using
Replaced typedef with using here and at other changes in this CL.
File third_party/WebKit/public/platform/WebString.h:
Please take a look at the section about unordered_set in https://chromium-cpp.appspot.com/.
I see. Replaced it with a struct.
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Kinuko Yasuda (Gerrit)
(Sorry, haven't hit 'Send' yet)
1 comment:
Patch Set #3, Line 232:
exposed_headers.find(header.key) == exposed_headers.end()))
(2) (looks over killing. just INSIDE_BLINK looks enough)
Hiding actual fields in INSIDE_BLINK would make the exposed struct size incompatible, we'll need to follow pimpl pattern for member fields.
I would appreciate something that wouldn't take me (i.e. a green intern) too much time
Yep, I'll try reducing discussion time, while we'll anyway need to add some conversion code somewhere as far as we want to share the code both inside and outside the Blink. It shouldn't be hard, but just a little more typing.
Anyways in short I think both I and Takeshi are suggesting that only using std:: (and base::) types for the things that are exposed (e.g. HTTPSet/HTTPMap in WebCORS), and using pimpl pattern for the things that can be hidden inside .cpp (e.g. private fields).
I think I can possibly help some of these if you feel you'll need some extra investigation to do that.
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Takeshi Yoshino (Gerrit)
1 comment:
Patch Set #3, Line 232:
exposed_headers.find(header.key) == exposed_headers.end()))(2) (looks over killing. just INSIDE_BLINK looks enough)
Hiding actual fields in INSIDE_BLINK would make the exposed struct size incompatible, we'll need to follow pimpl pattern for member fields.
Ah, I see. Sorry, I was misunderstanding that it's done commonly in WebKit/public and is viable here. It's not commonly done (only inside WebServiceWorkerInstalledScriptsManager.h) and though allocation of WebCORSPreflightResultCache is done inside Blink but can be destroyed outside of Blink where the mismatch will be problematic.
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
1 comment:
Patch Set #3, Line 232:
exposed_headers.find(header.key) == exposed_headers.end()))
> (2) (looks over killing. just INSIDE_BLINK looks enough) […]
Thank you for your support, it is most appreciated!
Regarding the suggested changes, I want to make sure I do understand them correctly. So I would replace platform/network/HTTPHeaderMap with some std:: container in the interface and only use std:: types (i.e. std::string) in those containers.
However, I would keep the rest of the interfaces as is, in particular would still use WebString as parameters in methods (not in containers)? Or should I replace WebString with std::string everywhere in the headers? I also assume I will keep using Web* types like WebURL.
And I would use the pimpl pattern only if it helps to achieve the above stated with regards to the interface, therefore assuming all private fields/methods rely on std::/base:: types only, I would not make use of the pimpl pattern, correct?
Those changes indeed don't sound like too much work. Lets see how far I get and maybe I'll ping you if I get stuck.
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Kinuko Yasuda (Gerrit)
1 comment:
Patch Set #3, Line 232:
exposed_headers.find(header.key) == exposed_headers.end()))
Thank you for your support, it is most appreciated! […]
Reg: HTTPHeaderMap I think it's also used in other places in Blink, would it be probably easier to just define std:: version of it separately? We can convert it to HTTPHeaderMap internally at boundary when necessary.
Keep using WebString / WebURL as methd parameters sound fine with me.
Yes, pimpl pattern can be used only if it helps. If all private fields/methods can just use std::/base:: types no need to use that.
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
1 comment:
Patch Set #3, Line 232:
exposed_headers.find(header.key) == exposed_headers.end()))
Reg: HTTPHeaderMap I think it's also used in other places in Blink, would it be probably easier to j […]
Thanks for the clarification. Yes, I meant creating a std:: version of HTTPHeaderMap; my comment was ambiguous.
I'll start implementing the discussed changes and ping you once done. Thanks!
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
Daniel Hintze would like Jochen Eisinger to review this change.
Replacing wtf types in WebCORS and WebCORSPreflightResultCache
As pointed out by @kinuko, CLs https://chromium-review.googlesource.com/c/612176
and https://chromium-review.googlesource.com/c/604028 introduced
dependency violations by using platform/wtf/* types in the header
files. This CL fixed those headers to rely only on Web* types and
std::* datastructures.
Bug: 736308
Change-Id: I7cc035db394e3d0b761988f089bc5c53168c1575
---
M third_party/WebKit/Source/core/exported/WebAssociatedURLLoaderImpl.cpp
M third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
M third_party/WebKit/Source/modules/fetch/FetchResponseData.cpp
M third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp
M third_party/WebKit/Source/platform/BUILD.gn
M third_party/WebKit/Source/platform/DEPS
M third_party/WebKit/Source/platform/exported/WebCORS.cpp
M third_party/WebKit/Source/platform/exported/WebCORSPreflightResultCache.cpp
M third_party/WebKit/Source/platform/exported/WebCORSTest.cpp
A third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp
M third_party/WebKit/Source/platform/exported/WebString.cpp
M third_party/WebKit/public/platform/WebCORS.h
M third_party/WebKit/public/platform/WebCORSPreflightResultCache.h
A third_party/WebKit/public/platform/WebHTTPHeaderMap.h
M third_party/WebKit/public/platform/WebString.h
15 files changed, 266 insertions(+), 90 deletions(-)
Daniel Hintze (Gerrit)
I now replaced (almost) all WTF types in WebCORS.h and WebCORSPreflightCache.h and introduced a WebHTTPHeaderMap which basically wraps around the corresponding types on either side of platform. I hope that approach is reasonable, so please take a look.
I think I still need to remove platform/wtf/ThreadSpecific.h from WebCORSPreflightCache.h - could someone give me a hint on how to implement something similar in platform?
Kinuko Yasuda (Gerrit)
Patch Set 5:
I now replaced (almost) all WTF types in WebCORS.h and WebCORSPreflightCache.h and introduced a WebHTTPHeaderMap which basically wraps around the corresponding types on either side of platform. I hope that approach is reasonable, so please take a look.
I think I still need to remove platform/wtf/ThreadSpecific.h from WebCORSPreflightCache.h - could someone give me a hint on how to implement something similar in platform?
You can use base::ThreadLocalPointer when it's out of blink.
Kinuko Yasuda (Gerrit)
6 comments:
File third_party/WebKit/Source/core/exported/WebAssociatedURLLoaderImpl.cpp:
Patch Set #9, Line 234:
blocked_headers.insert(header.key.Ascii().data());.data() is probably not needed (here and everywhere)
File third_party/WebKit/public/platform/WebCORSPreflightResultCache.h:
Patch Set #9, Line 63:
std::equal_to<std::string>>;Don't we need to make it case insensitive?
By the way if there's no strong requirement could we use simple std::{set,map} instead of unoredered_{set,map}?
File third_party/WebKit/public/platform/WebHTTPHeaderMap.h:
Patch Set #9, Line 25:
WebHTTPHeaderMap(const net::HttpRequestHeaders*);Could we make these explicit ctor or static creation methods, e.g. FromHTTPResponseHeaders(...), FromHTTPRequestHeaders(...) ?
Patch Set #9, Line 35:
WebHTTPHeaderMapImpl* implementation_; // HandlePlease use std::unique_ptr, otherwise we could easily double-free or leak it
File third_party/WebKit/public/platform/WebHTTPHeaderSet.h:
You can use base::ToLowerASCII (base/strings/string_util.h)
Patch Set #9, Line 43:
std::unordered_set<std::string, HashIgnoreCase, EqualIgnoreCase>;If we don't have good idea about the expected size on these sets/maps please just use std::set, or if the set will likely be small we can also use std::flat_set.
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
Thanks a lot for the fast review!
6 comments:
Patch Set #9, Line 234:
blocked_headers.insert(header.key.Ascii().data());.data() is probably not needed (here and everywhere)
Hm. I agree it is ugly, but I get a "error: no matching member function for call to 'find|insert'" if I remove the .data().
File third_party/WebKit/public/platform/WebCORSPreflightResultCache.h:
Patch Set #9, Line 63:
bool Parse(const WebHTTPHeaderMap& response_header,Don't we need to make it case insensitive? […]
Actually, no, but the diff does suggest so. Headers are supposed to be case insensitive (which is what WebHTTPHeaderSet sees to) and methods are supposed to be case sensitive. Should be less confusing in the new patchset though.
I also replaces all unsorted container with the std::{set,map}.
File third_party/WebKit/public/platform/WebHTTPHeaderMap.h:
Patch Set #9, Line 25:
explicit WebHTTPHeaderMap(const net::HttpResponseHeaders*);Could we make these explicit ctor or static creation methods, e.g. FromHTTPResponseHeaders(... […]
Made the ctors explicit.
Please use std::unique_ptr, otherwise we could easily double-free or leak it
Done
File third_party/WebKit/public/platform/WebHTTPHeaderSet.h:
You can use base::ToLowerASCII (base/strings/string_util. […]
That sounds better!
If we don't have good idea about the expected size on these sets/maps please just use std::set, or i […]
Done
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Kinuko Yasuda (Gerrit)
7 comments:
File third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp:
Patch Set #10, Line 58:
HTTPHeaderMap mutable_map;mutable_map_
Patch Set #10, Line 60:
const HTTPHeaderMap& map_;Please make these fields private, and add a public accessor to it
Patch Set #10, Line 76:
implementation_.release();This line's not needed
File third_party/WebKit/public/platform/WebCORSPreflightResultCache.h:
Patch Set #10, Line 73:
std::set<std::string, std::less<std::string>> methods_;I think you can just use std::set<std::string> now?
File third_party/WebKit/public/platform/WebHTTPHeaderMap.h:
Patch Set #10, Line 19:
class BLINK_PLATFORM_EXPORT WebHTTPHeaderMap {Can you add a brief class-level comment? E.g.
"A HTTP header map that takes net:: types but internally stores them in blink::HTTPHeaderMap"
Patch Set #10, Line 29:
WebHTTPHeaderMap(const HTTPHeaderMap&);The behavior that this just takes a reference but doesn't copy the given parameter is a bit surprising (this means the caller needs to keep the HTTPHeaderMap alive). Is it intentional? I think this'd definitely need a clear documentation (i.e. comment) if that's the case. (Some class that does that has a clear name like: WrappedFoo)
File third_party/WebKit/public/platform/WebHTTPHeaderSet.h:
Patch Set #10, Line 16:
return base::ToLowerASCII(Left) < base::ToLowerASCII(Right);Left, Right -> left, right (snake_case)
Also, this should do the trick:
return base::CompareCaseInsensitiveASCII(left, right) < 0
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
Thank you for your feedback!
7 comments:
File third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp:
Patch Set #10, Line 58:
implementation_ = base::MakeUnique<WebHTTPHeaderMapImpl>(headers);mutable_map_
Done
Please make these fields private, and add a public accessor to it
Done
This line's not needed
Done
File third_party/WebKit/public/platform/WebCORSPreflightResultCache.h:
Patch Set #10, Line 73:
std::set<std::string> methods_;
I think you can just use std::set<std::string> now?
Done
File third_party/WebKit/public/platform/WebHTTPHeaderMap.h:
Patch Set #10, Line 19:
// A HTTP header map that takes net:: types but internally stores them inCan you add a brief class-level comment? E.g. […]
Done
Patch Set #10, Line 29:
explicit WebHTTPHeaderMap(const net::HttpResponseHeaders*);The behavior that this just takes a reference but doesn't copy the given parameter is a bit surprisi […]
Hm, I can see that I am not consistent here, given that I actually do store a detached copy if one of the other constructors is used. I found it appealing not to copy the content of the map around "just" for a type conversion but it also felt like a waist of resources to compute the HTTPHeaderMap anew if called with one of the net::*Headers.
Anyways, for now I uploaded a patch that creates a copy instead of holding a reference but I don't have a strong opinion on which approach would be better, so let me know if you prefer a wrapper-like solution.
File third_party/WebKit/public/platform/WebHTTPHeaderSet.h:
Patch Set #10, Line 16:
return base::CompareCaseInsensitiveASCII(left, right) < 0;Left, Right -> left, right (snake_case) […]
Done
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Kinuko Yasuda (Gerrit)
lgtm
Patch set 11:Code-Review +1
5 comments:
File third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp:
Patch Set #11, Line 17:
WebHTTPHeaderMapImpl(const HTTPHeaderMap& map) : map_(map){};explicit
Patch Set #11, Line 19:
WebHTTPHeaderMapImpl(const net::HttpRequestHeaders* headers) {explicit
Patch Set #11, Line 27:
WebHTTPHeaderMapImpl(const net::HttpResponseHeaders* headers) {explicit
Patch Set #11, Line 45:
const HTTPHeaderMap& GetMap() const { return map_; };map() is fine for a simple getter
File third_party/WebKit/public/platform/WebHTTPHeaderMap.h:
Patch Set #11, Line 39:
std::unique_ptr<WebHTTPHeaderMapImpl> implementation_; // Handlenit: the comment's not needed.
btw you can also directly store std::unique_ptr<HTTPHeaderMap> here with forward declaration, then you don't need this wrapper class.
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
Many thanks, Kinuko!
@jochen, could you give it a look as well? Thanks!
5 comments:
File third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp:
Patch Set #11, Line 17:
explicit WebHTTPHeaderMapImpl(const HTTPHeaderMap& map) : map_(map){};explicit
Done
Patch Set #11, Line 19:
explicit WebHTTPHeaderMapImpl(const net::HttpRequestHeaders* headers) {explicit
Done
Patch Set #11, Line 27:
explicit WebHTTPHeaderMapImpl(const net::HttpResponseHeaders* headers) {explicit
Done
Patch Set #11, Line 45:
const HTTPHeaderMap& Map() const { return map_; };
map() is fine for a simple getter
Patch Set #11, Line 39:
std::unique_ptr<WebHTTPHeaderMapImpl> implementation_; // Handle
nit: the comment's not needed. […]
Removed the comment.
Regarding std::unique_ptr<HTTPHeaderMap>, I think DISALLOW_NEW in HTTPHeaderMap prevents me from creating an instance on heap which I think I would need?
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Jochen Eisinger (Gerrit)
lgtm from my side with cimments
Patch set 12:Code-Review +1
3 comments:
File third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp:
Patch Set #12, Line 45:
const HTTPHeaderMap& Map() const { return map_; };nit. simple getters are all lower-case
File third_party/WebKit/public/platform/WebCORSPreflightResultCache.h:
Patch Set #12, Line 73:
std::set<std::string> methods_;base::flat_set maybe? (I'd expect at most three methods)
File third_party/WebKit/public/platform/WebHTTPHeaderSet.h:
Patch Set #12, Line 14:
struct CompareIgnoreCase {it's a bit odd to have this in the blink namespace, but not sure what else to do here..
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
Thanks @Jochen!
3 comments:
File third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp:
Patch Set #12, Line 45:
const HTTPHeaderMap& map() const { return map_; };nit. […]
Done
File third_party/WebKit/public/platform/WebCORSPreflightResultCache.h:
Patch Set #12, Line 73:
base::flat_set<std::string> methods_;
base::flat_set maybe? (I'd expect at most three methods)
Fair point. Done.
File third_party/WebKit/public/platform/WebHTTPHeaderSet.h:
Patch Set #12, Line 14:
struct CompareIgnoreCase {it's a bit odd to have this in the blink namespace, but not sure what else to do here..
Would adding say a nested WebHTTPHeaderSet namespace make it better?
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Takeshi Yoshino (Gerrit)
I'm reviewing now but feel free to submit without my l-g-t-m. Further comments of mine can be addressed later.
3 comments:
File third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp:
Patch Set #13, Line 74:
headers.insert(header.Ascii().data());how about headers.emplace(header.Ascii().data(), header.Ascii().length) for double sure?
File third_party/WebKit/Source/platform/exported/WebCORS.cpp:
Patch Set #13, Line 116:
output.insert(value_.Substring(token_start, token_size).Ascii().data());ditto
File third_party/WebKit/public/platform/WebHTTPHeaderMap.h:
put a blank line here
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Takeshi Yoshino (Gerrit)
Basically trivial IWYU comments and suggestion of utilization of CString's length to avoid scanning null terminator.
Again, feel free to skip them as you're hurried.
Thank you!
Patch set 13:Code-Review +1
5 comments:
File third_party/WebKit/Source/modules/fetch/FetchResponseData.cpp:
Patch Set #13, Line 131:
response->cors_exposed_header_names_.insert(name.Ascii().data());use emplace?
File third_party/WebKit/Source/platform/exported/WebCORS.cpp:
Patch Set #13, Line 547:
header_set.insert(header.Ascii().data());ditto (how about emplace?)
File third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp:
add <string>
Patch Set #13, Line 10:
#include "platform/network/HTTPHeaderMap.h"add platform/wtf/text/AtomicString.h
File third_party/WebKit/public/platform/WebCORSPreflightResultCache.h:
Patch Set #13, Line 30:
#include <memory>#include <string>
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
Thanks for your diligent feedback late at night - it is very much appreciated!
8 comments:
File third_party/WebKit/Source/modules/fetch/FetchResponseData.cpp:
Patch Set #13, Line 131:
response->cors_exposed_header_names_.emplace(name.Ascii().data(),use emplace?
Done
File third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp:
Patch Set #13, Line 74:
headers.emplace(header.Ascii().data(), header.Ascii().length());how about headers.emplace(header.Ascii().data(), header.Ascii(). […]
Sounds reasonable. Done.
File third_party/WebKit/Source/platform/exported/WebCORS.cpp:
ditto
Done
Patch Set #13, Line 547:
if (response.WasFetchedViaServiceWorker()) {ditto (how about emplace?)
Done
File third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp:
add <string>
Done
Patch Set #13, Line 10:
#include "net/http/http_response_headers.h"add platform/wtf/text/AtomicString. […]
Done
File third_party/WebKit/public/platform/WebCORSPreflightResultCache.h:
Patch Set #13, Line 30:
#include <memory>#include <string>
put a blank line here
Done
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Takeshi Yoshino (Gerrit)
lgtm!
1 comment:
File third_party/WebKit/Source/platform/exported/WebCORS.cpp:
Patch Set #14, Line 117:
CString substring = value_.Substring(token_start, token_size).Ascii();const CString&
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
Thanks!
1 comment:
File third_party/WebKit/Source/platform/exported/WebCORS.cpp:
Patch Set #14, Line 117:
const CString& name = value_.Substring(token_start, token_size).Ascii();const CString&
Done
To view, visit change 618716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Hintze (Gerrit)
Patch set 15:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Review comment round six" https://chromium-review.googlesource.com/c/618716/15
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/618716/15
Bot data: {"action": "start", "triggered_at": "2017-08-30T16:09:27.0Z", "cq_cfg_revision": "81f1a3f0cb07446abeff7dd01ff1b8bae94ffe0d", "revision": "ea5c367d7e7903ab3fd754925141ebac5287f4e9"}
Commit Bot (Gerrit)
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_swarming_rel/builds/252983)
Daniel Hintze (Gerrit)
Hey,
unfortunately, Visual Studio apparently does not like my changes and complains about the use of a deleted function. Though I got help from Mike and Daniel, I could not figure out the root cause (unless the last commit actually fixed it - can't wait for the build bot though) throughout the day.
So to my great annoyance, I won't land this once either - could someone please adopt this CL and bring it home?
Thanks a lot!
Over and out,
Daniel
Jochen Eisinger (Gerrit)
Patch set 20:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Adding WebNonCopyable to see if this helps" https://chromium-review.googlesource.com/c/618716/20
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/618716/20
Bot data: {"action": "start", "triggered_at": "2017-08-31T16:15:51.0Z", "cq_cfg_revision": "81f1a3f0cb07446abeff7dd01ff1b8bae94ffe0d", "revision": "43b159d85e71693500094a3405bfe812edca4945"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Replacing wtf types in WebCORS and WebCORSPreflightResultCache
As pointed out by @kinuko, CLs https://chromium-review.googlesource.com/c/612176
and https://chromium-review.googlesource.com/c/604028 introduced
dependency violations by using platform/wtf/* types in the header
files. This CL fixed those headers to rely only on Web* types and
std::* datastructures.
Bug: 736308
Change-Id: I7cc035db394e3d0b761988f089bc5c53168c1575
Reviewed-on: https://chromium-review.googlesource.com/618716
Commit-Queue: Daniel Hintze <hin...@google.com>
Commit-Queue: Jochen Eisinger <joc...@chromium.org>
Reviewed-by: Takeshi Yoshino <tyos...@chromium.org>
Reviewed-by: Jochen Eisinger <joc...@chromium.org>
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498897}
---
M third_party/WebKit/Source/core/exported/WebAssociatedURLLoaderImpl.cpp
M third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
M third_party/WebKit/Source/modules/fetch/FetchManager.cpp
M third_party/WebKit/Source/modules/fetch/FetchResponseData.cpp
M third_party/WebKit/Source/modules/fetch/FetchResponseData.h
M third_party/WebKit/Source/modules/fetch/FetchResponseDataTest.cpp
M third_party/WebKit/Source/modules/fetch/Response.cpp
M third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp
M third_party/WebKit/Source/platform/BUILD.gn
M third_party/WebKit/Source/platform/DEPS
M third_party/WebKit/Source/platform/exported/WebCORS.cpp
M third_party/WebKit/Source/platform/exported/WebCORSPreflightResultCache.cpp
M third_party/WebKit/Source/platform/exported/WebCORSTest.cpp
A third_party/WebKit/Source/platform/exported/WebHTTPHeaderMap.cpp
M third_party/WebKit/public/platform/DEPS
M third_party/WebKit/public/platform/WebCORS.h
M third_party/WebKit/public/platform/WebCORSPreflightResultCache.h
A third_party/WebKit/public/platform/WebHTTPHeaderMap.h
A third_party/WebKit/public/platform/WebHTTPHeaderSet.h
19 files changed, 306 insertions(+), 150 deletions(-)
Mike West (Gerrit)
Patch Set 20:
Hey,
unfortunately, Visual Studio apparently does not like my changes and complains about the use of a deleted function. Though I got help from Mike and Daniel, I could not figure out the root cause (unless the last commit actually fixed it - can't wait for the build bot though) throughout the day.
So to my great annoyance, I won't land this once either - could someone please adopt this CL and bring it home?
Thanks a lot!
Over and out,
Daniel
Yay, we got it!
Nice work getting this in just in the nick of time. ;)
Daniel Hintze (Gerrit)
Patch Set 21:
Patch Set 20:
Hey,
unfortunately, Visual Studio apparently does not like my changes and complains about the use of a deleted function. Though I got help from Mike and Daniel, I could not figure out the root cause (unless the last commit actually fixed it - can't wait for the build bot though) throughout the day.
So to my great annoyance, I won't land this once either - could someone please adopt this CL and bring it home?
Thanks a lot!
Over and out,
Daniel
Yay, we got it!
Nice work getting this in just in the nick of time. ;)
Great! Thanks Mike!