Summary: | [chromium] add extraData field to resource requests | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||||||
Component: | New Bugs | Assignee: | jochen | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, battre, commit-queue, fishd, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
jochen
2011-05-18 02:59:59 PDT
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 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
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
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
Comment on attachment 94129 [details] Patch Clearing flags on attachment: 94129 Committed r87015: <http://trac.webkit.org/changeset/87015> All reviewed patches have been landed. Closing bug. *** Bug 49113 has been marked as a duplicate of this bug. *** |