Bug 158338 - Fix AtomicString regression caused by r201603
Summary: Fix AtomicString regression caused by r201603
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-02 20:41 PDT by Brady Eidson
Modified: 2016-06-03 08:36 PDT (History)
4 users (show)

See Also:


Attachments
WIP just to show my plan (3.36 KB, patch)
2016-06-02 20:43 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2016-06-02 22:07 PDT, Brady Eidson
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-06-02 20:41:52 PDT
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
Comment 1 Chris Dumez 2016-06-02 20:43:08 PDT
rdar://problem/26612033
Comment 2 Brady Eidson 2016-06-02 20:43:10 PDT
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 3 Chris Dumez 2016-06-02 20:45:49 PDT
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 4 Chris Dumez 2016-06-02 20:46:16 PDT
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! :)
Comment 5 Brady Eidson 2016-06-02 22:04:32 PDT
(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)
Comment 6 Brady Eidson 2016-06-02 22:07:18 PDT
Created attachment 280425 [details]
Patch
Comment 7 Brady Eidson 2016-06-02 22:08:25 PDT
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 8 Chris Dumez 2016-06-02 22:19:30 PDT
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.
Comment 9 Brady Eidson 2016-06-02 22:22:34 PDT
(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.
Comment 10 Chris Dumez 2016-06-02 22:26:33 PDT
(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.
Comment 11 Brady Eidson 2016-06-02 22:30:40 PDT
(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.  :)
Comment 12 Chris Dumez 2016-06-02 22:31:30 PDT
(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 ;)
Comment 13 Brady Eidson 2016-06-02 22:38:23 PDT
(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 👏
Comment 14 Brady Eidson 2016-06-03 08:36:28 PDT
http://trac.webkit.org/changeset/201637