Bug 61033 – [chromium] add extraData field to resource requests

• Component: New Bugs
• Status: RESOLVED FIXED

[chromium] add extraData field to resource requests

Created attachment 93891 [details]
Patch

There is actual a test in chromium/tests

The idea is to set additional fields that are only interesting to Chrome like the WebFrame identifier the requests was made for in willSendRequest

[Adam Barth]

Comment on attachment 93891 [details]
Patch

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

> Source/WebCore/platform/network/chromium/ResourceRequest.h:115
> +        PassRefPtr<ExtraData> extraData() const { return m_extraData; }

This should return ExtraData* because the function doesn't transfer ownership of m_extraData.

> Source/WebKit/chromium/src/WebURLRequest.cpp:308
> +    RefPtr<ExtraDataContainer> container = adoptRef(new ExtraDataContainer(extraData));

ExtraDataContainer should have an static create method.

Created attachment 93978 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 93978 [details]
Patch

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

I guess ExtraData just won't be something we copy across threads when we copy
a ResourceRequest to another thread.  That is probably OK.

> Source/WebCore/platform/network/chromium/ResourceRequest.h:112
> +        // pointer will be deleted when the request is destroyed. Setting the

deleted -> deref'd

> Source/WebCore/platform/network/chromium/ResourceRequest.h:114
> +        // pointer to be deleted.

this comment is a bit incorrect.  the existing extra data may not be deleted,
it will be deref'd.  that may or may not cause it to be deleted.

> Source/WebKit/chromium/public/WebURLRequest.h:59
> +    class ExtraData {

nit: try not to break up the flow of enum declarations.  grouping enums separately from inner classes is nicer.
how about putting this below TargetType?

> Source/WebKit/chromium/src/WebURLRequest.cpp:310
> +    RefPtr<ExtraDataContainer> container = ExtraDataContainer::create(extraData);

nit: no need for the container temp var...

just put it all in a single line:

  m_private->m_resourceRequest->setExtraData(ExtraDataContainer::create(extraData));

> Source/WebKit/chromium/tests/WebURLRequestTest.cpp:59
> +        WebURLRequest urlRequest;

this is a good test, but you should also test that copying urlRequest
works properly too.

Created attachment 94127 [details]
Patch

[WebKit Review Bot]

Attachment 94127 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/public/WebURLRequest.h:175:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #5)
> (From update of attachment 93978 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93978&action=review
> 
> I guess ExtraData just won't be something we copy across threads when we copy
> a ResourceRequest to another thread.  That is probably OK.
> 
> > Source/WebCore/platform/network/chromium/ResourceRequest.h:112
> > +        // pointer will be deleted when the request is destroyed. Setting the
> 
> deleted -> deref'd

Done

> 
> > Source/WebCore/platform/network/chromium/ResourceRequest.h:114
> > +        // pointer to be deleted.
> 
> this comment is a bit incorrect.  the existing extra data may not be deleted,
> it will be deref'd.  that may or may not cause it to be deleted.

I simplified the comment. Given that the signature has a PassRefPtr, I think it's clear what's happening

Instead, I expanded the comment in WebURLRequest.h where it's not clear how long the ExtraData object will live

> 
> > Source/WebKit/chromium/public/WebURLRequest.h:59
> > +    class ExtraData {
> 
> nit: try not to break up the flow of enum declarations.  grouping enums separately from inner classes is nicer.
> how about putting this below TargetType?

Done

> 
> > Source/WebKit/chromium/src/WebURLRequest.cpp:310
> > +    RefPtr<ExtraDataContainer> container = ExtraDataContainer::create(extraData);
> 
> nit: no need for the container temp var...
> 
> just put it all in a single line:
> 
>   m_private->m_resourceRequest->setExtraData(ExtraDataContainer::create(extraData));

Done

> 
> > Source/WebKit/chromium/tests/WebURLRequestTest.cpp:59
> > +        WebURLRequest urlRequest;
> 
> this is a good test, but you should also test that copying urlRequest
> works properly too.

Done

Created attachment 94129 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 94129 [details]
Patch

Clearing flags on attachment: 94129

Committed r87015: <http://trac.webkit.org/changeset/87015>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

*** Bug 49113 has been marked as a duplicate of this bug. ***