Bug 158338 – Fix AtomicString regression caused by r201603

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

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

[Chris Dumez]

rdar://problem/26612033

[Brady Eidson]

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.

[Chris Dumez]

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?

[Chris Dumez]

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! :)

[Brady Eidson]

(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)

[Brady Eidson]

Created attachment 280425 [details]
Patch

[Brady Eidson]

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.

[Chris Dumez]

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.

[Brady Eidson]

(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.

[Chris Dumez]

(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.

[Brady Eidson]

(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.  :)

[Chris Dumez]

(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 ;)

[Brady Eidson]

(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 👏

[Brady Eidson]

http://trac.webkit.org/changeset/201637