Bug 158313 - Modernize how many platform/network classes do isolatedCopy()
Summary: Modernize how many platform/network classes do isolatedCopy()
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:
Depends on: 158695
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-02 13:19 PDT by Brady Eidson
Modified: 2016-06-13 10:03 PDT (History)
11 users (show)

See Also:


Attachments
Patch (36.37 KB, patch)
2016-06-02 13:37 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (36.49 KB, patch)
2016-06-02 14:28 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (36.49 KB, patch)
2016-06-02 14:29 PDT, Brady Eidson
no flags 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 13:19:04 PDT
Modernize how many platform/network classes do isolatedCopy()

First part of this work was in https://bugs.webkit.org/show_bug.cgi?id=158293

Now let's get rid of these "CrossThread*Data" objects altogether.
Comment 1 Brady Eidson 2016-06-02 13:37:35 PDT
Created attachment 280359 [details]
Patch
Comment 2 Alex Christensen 2016-06-02 14:01:39 PDT
Comment on attachment 280359 [details]
Patch

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

> Source/WebCore/platform/network/ResourceRequestBase.cpp:87
> +    setAllowCookies(m_allowCookies);

other.m_allowCookies

> Source/WebCore/platform/network/ResourceRequestBase.cpp:89
> +    const_cast<ResourceRequest&>(asResourceRequest()).doPlatformSetAsIsolatedCopy(other);

Would it work to make a non-const asResourceRequest() instead of a const_cast here?
Comment 3 Brady Eidson 2016-06-02 14:20:53 PDT
(In reply to comment #2)
> Comment on attachment 280359 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280359&action=review
> 
> > Source/WebCore/platform/network/ResourceRequestBase.cpp:87
> > +    setAllowCookies(m_allowCookies);
> 
> other.m_allowCookies

Yup yikes.

> > Source/WebCore/platform/network/ResourceRequestBase.cpp:89
> > +    const_cast<ResourceRequest&>(asResourceRequest()).doPlatformSetAsIsolatedCopy(other);
> 
> Would it work to make a non-const asResourceRequest() instead of a
> const_cast here?

const_cast'ing the const ResourceRequest& is an already established pattern in the class.


The relationship between ResourceRequestBase and ResourceRequest is already so gnarly that I didn't feel like tackling that here.

Would be a good followup cleanup.
Comment 4 Brady Eidson 2016-06-02 14:24:46 PDT
The linux build errors:

In file included from ../../Source/WebCore/platform/network/ResourceErrorBase.cpp:28:0:
../../Source/WebCore/platform/network/soup/ResourceError.h: In member function 'WebCore::ResourceError WebCore::ResourceErrorBase::isolatedCopy() const':
../../Source/WebCore/platform/network/soup/ResourceError.h:70:10: error: 'void WebCore::ResourceError::doPlatformIsolatedCopy(const WebCore::ResourceError&)' is private
../../Source/WebCore/platform/network/ResourceErrorBase.cpp:52:55: error: within this context

Funny, that method is private in the CF/Mac version as well, but it builds on Clang.

Anyways, yah I'll make it public.

(Normally I'd virtualize it, but we try to keep these classes non-virtual for some historic reason I forgot)
Comment 5 Brady Eidson 2016-06-02 14:25:42 PDT
(In reply to comment #4)
> The linux build errors:
> 
> In file included from
> ../../Source/WebCore/platform/network/ResourceErrorBase.cpp:28:0:
> ../../Source/WebCore/platform/network/soup/ResourceError.h: In member
> function 'WebCore::ResourceError WebCore::ResourceErrorBase::isolatedCopy()
> const':
> ../../Source/WebCore/platform/network/soup/ResourceError.h:70:10: error:
> 'void WebCore::ResourceError::doPlatformIsolatedCopy(const
> WebCore::ResourceError&)' is private
> ../../Source/WebCore/platform/network/ResourceErrorBase.cpp:52:55: error:
> within this context
> 
> Funny, that method is private in the CF/Mac version as well, but it builds
> on Clang.
> 
> Anyways, yah I'll make it public.
> 
> (Normally I'd virtualize it, but we try to keep these classes non-virtual
> for some historic reason I forgot)

Oh duh:
    friend class ResourceErrorBase;


Will add to the other headers.
Comment 6 Brady Eidson 2016-06-02 14:27:20 PDT
Last 500 characters of output:
p.o.d -o Source/WebCore/CMakeFiles/WebCore.dir/platform/network/FormData.cpp.o -c ../../Source/WebCore/platform/network/FormData.cpp
../../Source/WebCore/platform/network/FormData.cpp: In member function 'WebCore::FormDataElement WebCore::FormDataElement::isolatedCopy() const':
../../Source/WebCore/platform/network/FormData.cpp:139:1: error: control reaches end of non-void function [-Werror=return-type]

That's just GCC not recognizing that all 3 of the possible cases are handled. *sigh*
Comment 7 Brady Eidson 2016-06-02 14:28:26 PDT
Created attachment 280367 [details]
Patch
Comment 8 Brady Eidson 2016-06-02 14:29:22 PDT
Created attachment 280368 [details]
Patch
Comment 9 Brady Eidson 2016-06-02 14:30:03 PDT
For an EWS run. Will cq+ once linux bots are happy
Comment 10 WebKit Commit Bot 2016-06-02 15:42:05 PDT
Comment on attachment 280368 [details]
Patch

Clearing flags on attachment: 280368

Committed r201623: <http://trac.webkit.org/changeset/201623>
Comment 11 WebKit Commit Bot 2016-06-02 15:42:12 PDT
All reviewed patches have been landed.  Closing bug.