Fix AtomicString regression caused by r201603 http://trac.webkit.org/changeset/201603 Discussion of the regression is in the bug: https://bugs.webkit.org/show_bug.cgi?id=158293
rdar://problem/26612033
Created attachment 280421 [details] WIP just to show my plan This patch is incomplete and I haven't even fully built yet, but this is what I plan on doing to replace the old busted mechanism.
Comment on attachment 280421 [details] WIP just to show my plan View in context: https://bugs.webkit.org/attachment.cgi?id=280421&action=review > Source/WebCore/platform/network/ResourceResponseBase.h:62 > + ResourceResponse createResponse() const; I which we would use an (implicit) ResponseResponse constructor for this instead, to make the call sites a bit simpler. > Source/WebCore/platform/network/ResourceResponseBase.h:65 > + std::unique_ptr<CrossThreadData> crossThreadData() const; This does not really need to return a unique_ptr<> right?
Comment on attachment 280421 [details] WIP just to show my plan View in context: https://bugs.webkit.org/attachment.cgi?id=280421&action=review >> Source/WebCore/platform/network/ResourceResponseBase.h:62 >> + ResourceResponse createResponse() const; > > I which we would use an (implicit) ResponseResponse constructor for this instead, to make the call sites a bit simpler. I meant, I wish! :)
(In reply to comment #3) > Comment on attachment 280421 [details] > WIP just to show my plan > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280421&action=review > > > Source/WebCore/platform/network/ResourceResponseBase.h:62 > > + ResourceResponse createResponse() const; > > I which we would use an (implicit) ResponseResponse constructor for this > instead, to make the call sites a bit simpler. I'd be against an implicit constructor, as it would hide the "magic" that's happening here. What we're doing here is rather non-obvious, and trying to hide it wouldn't be great for future readability. But another reason why I don't want to add even and explicit constructor is because of the ridiculous relationship between ResourceResponseBase and ResourceResponse - of which there are 3 completely separate implementations. Touching all the platforms and duplicating all the code... doesn't seem like a great idea. I'd rather *improve* the current platform-specific-response situation, and that would just make it worse again. > > > Source/WebCore/platform/network/ResourceResponseBase.h:65 > > + std::unique_ptr<CrossThreadData> crossThreadData() const; > > This does not really need to return a unique_ptr<> right? Strictly? No. But it needs to be non copyable, and it turns out making a non copyable object and putting it on the stack and then getting it to compile requires a lot of gross &&'s and WTFMoves - Way more than you might expect. (I know because I tried)
Created attachment 280425 [details] Patch
I don't think this patch makes the lambda site any grosser - It's still the same number of lines of code, still init-in-capture. It's also quite clear what's going on to somebody reading the code but unfamiliar with ResourceResponse's impl.
Comment on attachment 280425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280425&action=review R=me > Source/WebCore/platform/network/ResourceResponseBase.h:62 > + ResourceResponse createResponse(); I think I would have have preferred a static method on ResourceResponseBase. E.g. ResourceResponse::fromCrossThreadData(CrossThreadData&&) or ResourceResponse::adoptCrossThreadData(CrossThreadData&&). It seems a bit odd to have a factory on the cross-thread struct, especially considering to kind of makes the cross-thread data useless (since it WTFMoves httpHeaderFields for e.g.). Using a static function a WTFMove() makes it more OK to WTFMove() members. Also note that with my proposal, there is no real benefit to using unique_ptr I believe.
(In reply to comment #8) > Comment on attachment 280425 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280425&action=review > > R=me > > > Source/WebCore/platform/network/ResourceResponseBase.h:62 > > + ResourceResponse createResponse(); > > I think I would have have preferred a static method on ResourceResponseBase. > E.g. ResourceResponse::fromCrossThreadData(CrossThreadData&&) or > ResourceResponse::adoptCrossThreadData(CrossThreadData&&). I'll move the method as suggested. > It seems a bit > odd to have a factory on the cross-thread struct, especially considering to > kind of makes the cross-thread data useless (since it WTFMoves > httpHeaderFields for e.g.). Using a static function a WTFMove() makes it > more OK to WTFMove() members. Also note that with my proposal, there is no > real benefit to using unique_ptr I believe. Believe me, there is. I can walk you through the compiler goose hunt it causes if you'd like.
(In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 280425 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=280425&action=review > > > > R=me > > > > > Source/WebCore/platform/network/ResourceResponseBase.h:62 > > > + ResourceResponse createResponse(); > > > > I think I would have have preferred a static method on ResourceResponseBase. > > E.g. ResourceResponse::fromCrossThreadData(CrossThreadData&&) or > > ResourceResponse::adoptCrossThreadData(CrossThreadData&&). > > I'll move the method as suggested. > > > It seems a bit > > odd to have a factory on the cross-thread struct, especially considering to > > kind of makes the cross-thread data useless (since it WTFMoves > > httpHeaderFields for e.g.). Using a static function a WTFMove() makes it > > more OK to WTFMove() members. Also note that with my proposal, there is no > > real benefit to using unique_ptr I believe. > > Believe me, there is. I can walk you through the compiler goose hunt it > causes if you'd like. Ok.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Comment on attachment 280425 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=280425&action=review > > > > > > R=me > > > > > > > Source/WebCore/platform/network/ResourceResponseBase.h:62 > > > > + ResourceResponse createResponse(); > > > > > > I think I would have have preferred a static method on ResourceResponseBase. > > > E.g. ResourceResponse::fromCrossThreadData(CrossThreadData&&) or > > > ResourceResponse::adoptCrossThreadData(CrossThreadData&&). > > > > I'll move the method as suggested. > > > > > It seems a bit > > > odd to have a factory on the cross-thread struct, especially considering to > > > kind of makes the cross-thread data useless (since it WTFMoves > > > httpHeaderFields for e.g.). Using a static function a WTFMove() makes it > > > more OK to WTFMove() members. Also note that with my proposal, there is no > > > real benefit to using unique_ptr I believe. > > > > Believe me, there is. I can walk you through the compiler goose hunt it > > causes if you'd like. > > Ok. I meant like tomorrow in person, because I need to get this landed tonight before crashing. :)
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > Comment on attachment 280425 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=280425&action=review > > > > > > > > R=me > > > > > > > > > Source/WebCore/platform/network/ResourceResponseBase.h:62 > > > > > + ResourceResponse createResponse(); > > > > > > > > I think I would have have preferred a static method on ResourceResponseBase. > > > > E.g. ResourceResponse::fromCrossThreadData(CrossThreadData&&) or > > > > ResourceResponse::adoptCrossThreadData(CrossThreadData&&). > > > > > > I'll move the method as suggested. > > > > > > > It seems a bit > > > > odd to have a factory on the cross-thread struct, especially considering to > > > > kind of makes the cross-thread data useless (since it WTFMoves > > > > httpHeaderFields for e.g.). Using a static function a WTFMove() makes it > > > > more OK to WTFMove() members. Also note that with my proposal, there is no > > > > real benefit to using unique_ptr I believe. > > > > > > Believe me, there is. I can walk you through the compiler goose hunt it > > > causes if you'd like. > > > > Ok. > > I meant like tomorrow in person, because I need to get this landed tonight > before crashing. :) Oh, I meant OK to land using unique_ptr. But I am happy to get more details tomorrow too ;)
(In reply to comment #12) > (In reply to comment #11) > > > > I meant like tomorrow in person, because I need to get this landed tonight > > before crashing. :) > > Oh, I meant OK to land using unique_ptr. But I am happy to get more details > tomorrow too ;) Right - I figured that's what r+ meant 👏
http://trac.webkit.org/changeset/201637