Bug 175391

Summary: Teach ScriptExecutionContexts about their SessionID
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, buildbot, cdumez, commit-queue, dbates, esprehn+autocc, kangil.han, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=174541
Attachments:
Description Flags
Patch none

Description Brady Eidson 2017-08-09 11:22:07 PDT
Teach ScriptExecutionContexts about their SessionID

Especially needed to expose SessionIDs to workers.
Comment 1 Brady Eidson 2017-08-09 11:57:19 PDT
Created attachment 317726 [details]
Patch
Comment 2 Andy Estes 2017-08-09 12:05:10 PDT
Comment on attachment 317726 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317726&action=review

> Source/WebCore/page/SessionID.cpp:61
> +SessionID SessionID::isolatedCopy() const
> +{
> +    return SessionID { m_sessionID };
> +}

Seems like you could just do a regular copy-by-value in WorkerThreadStartupData instead of adding this, since there's no actual isolated copying going on here.
Comment 3 Brady Eidson 2017-08-09 12:56:14 PDT
(In reply to Andy Estes from comment #2)
> Comment on attachment 317726 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317726&action=review
> 
> > Source/WebCore/page/SessionID.cpp:61
> > +SessionID SessionID::isolatedCopy() const
> > +{
> > +    return SessionID { m_sessionID };
> > +}
> 
> Seems like you could just do a regular copy-by-value in
> WorkerThreadStartupData instead of adding this, since there's no actual
> isolated copying going on here.

I much prefer future proofing (future resisting?) any class that is passed cross thread.

By having isolatedCopy() in the SessionID.h header, it suggests to anybody adding any future arguments that they need to account for them.
Comment 4 Brady Eidson 2017-08-09 13:04:45 PDT
(In reply to Brady Eidson from comment #3)
> (In reply to Andy Estes from comment #2)
> > Comment on attachment 317726 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=317726&action=review
> > 
> > > Source/WebCore/page/SessionID.cpp:61
> > > +SessionID SessionID::isolatedCopy() const
> > > +{
> > > +    return SessionID { m_sessionID };
> > > +}
> > 
> > Seems like you could just do a regular copy-by-value in
> > WorkerThreadStartupData instead of adding this, since there's no actual
> > isolated copying going on here.
> 
> I much prefer future proofing (future resisting?) any class that is passed
> cross thread.
> 
> By having isolatedCopy() in the SessionID.h header, it suggests to anybody
> adding any future arguments that they need to account for them.

s/future arguments/future members/
Comment 5 WebKit Commit Bot 2017-08-09 13:26:01 PDT
Comment on attachment 317726 [details]
Patch

Clearing flags on attachment: 317726

Committed r220475: <http://trac.webkit.org/changeset/220475>
Comment 6 WebKit Commit Bot 2017-08-09 13:26:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2017-08-09 13:26:44 PDT
<rdar://problem/33809118>