Bug 46430 – Platform specific ResourceResponse and ResourceRequest can't fully cross threads.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Michael Nordman]

Platform specific ResourceResponse and ResourceRequest classes have no means to copy their data via across threads. The CrossThreadResourceRequestData and CrossThreadResourceResponseData structs are fully defined in common code as are the methods to copyData() and adopt(copiedData).

[Michael Nordman]

Created attachment 69096 [details]
crossThread patch

[Michael Nordman]

Created attachment 69098 [details]
crossThread patch

Fixed typos in the ChangeLog.

[David Levin]

Comment on attachment 69098 [details]
crossThread patch

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

Awesome!

> WebCore/platform/network/ResourceRequestBase.h:143
> +        static bool compare(const ResourceRequest& a, const ResourceRequest& b);

When I see "compare", I think stpcmp and expect that 0 means equal. How about isSame or isEqual or equivalent, etc.? Ugh, I see ResourceResponse already has this. I still hate the name but consistency is good, so nevermind :(


Do this: omit "a" and "b" as they add no information. (Yes, ResourceResponseBase.h does the same thing but it is wrong and consistency here adds no value.)

> WebCore/platform/network/android/ResourceRequest.h:58
> +    void doPlatformAdopt(PassOwnPtr<CrossThreadResourceRequestData> data) { }

Omit data here or else you will get unused variable erros/warnings on some platforms (throughout).

> WebCore/platform/network/chromium/ResourceRequest.h:93
> +        PassOwnPtr<CrossThreadResourceRequestData> doPlatformCopyData(PassOwnPtr<CrossThreadResourceRequestData> data) const;

Omit param name "data" here as it adds no information.

> WebCore/platform/network/chromium/ResourceRequest.h:94
> +        void doPlatformAdopt(PassOwnPtr<CrossThreadResourceRequestData> data);

Ditto.

[Michael Nordman]

Created attachment 69126 [details]
crossThread patch

Done... removed param names where possible.

[David Levin]

Comment on attachment 69126 [details]
crossThread patch

Needs minor clean up of nits as discussed.

[Michael Nordman]

Created attachment 69134 [details]
crossThread patch

Fixed a typo in the changeLog, removed param names from ResourceResponseBase::compare, deleted a blank line in ResourceResponseBase.cpp.

[WebKit Commit Bot]

Comment on attachment 69134 [details]
crossThread patch

Rejecting patch 69134 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet']" exit_code: 2
Building WebKit
Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1

Full output: http://queues.webkit.org/results/4163008

[Michael Nordman]

bumping heads with http://trac.webkit.org/changeset/68510?

i'll update my client

[Michael Nordman]

Created attachment 69150 [details]
crossThread patch

updated my client and created a new patch

[Dumitru Daniliuc]

Comment on attachment 69150 [details]
crossThread patch

rs=me, based on dave's r+.

[WebKit Commit Bot]

Comment on attachment 69150 [details]
crossThread patch

Rejecting patch 69150 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet']" exit_code: 2
Building WebKit
Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1

Full output: http://queues.webkit.org/results/4202007

[Michael Nordman]

(In reply to comment #11)
> (From update of attachment 69150 [details])
> Rejecting patch 69150 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet']" exit_code: 2
> Building WebKit
> Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
> 
> Full output: http://queues.webkit.org/results/4202007

I guess that's not a merge conflict after all?
WebKitTools/Scripts/build-webkit fails

Looking...

[Michael Nordman]

A link error on mac.

Ld /Users/michaeln/webkit/1/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore normal i386
 .....
Undefined symbols:
  "__ZN7WebCoreeqERKNS_19ResourceRequestBaseES2_", referenced from:
     -exported_symbols_list command line option
ld: symbol(s) not found
collect2: ld returned 1 exit status

[Michael Nordman]

Ok... need to delete a symbol in WebCore.exp.in

[Michael Nordman]

Created attachment 69241 [details]
crossThread patch

removed the now inlined export from webcore.exp.in

[WebKit Commit Bot]

Comment on attachment 69241 [details]
crossThread patch

Clearing flags on attachment: 69241

Committed r68762: <http://trac.webkit.org/changeset/68762>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.