Summary: | [Chromium] Expose SecurityOrigin::shouldHideReferrer() in chromium port api. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nate Chapin <japhet> | ||||||||
Component: | WebKit Misc. | Assignee: | Nate Chapin <japhet> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, dimich, fishd, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Nate Chapin
2009-12-10 16:11:44 PST
Created attachment 44646 [details]
patch
Attachment 44646 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebSecurityOrigin.cpp:96: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 1
Comment on attachment 44646 [details] patch A few style nits: > Index: WebKit/chromium/src/WebSecurityOrigin.cpp > +bool WebSecurityOrigin::shouldHideReferrer(const WebURL& url, const WebString& referrer) { '}' should go on the next line as bot said ^^^ > + return SecurityOrigin::shouldHideReferrer(WebCore::KURL(url.spec(), url.parsed(), url.isValid()), You could simply pass WebURL into SecurityOrigin::shouldHideReferer since WebURL has conversion operator to convert to KURL. > + WebCore::String(referrer)); WebKit code usually does not wrap a long line like this. > Index: WebKit/chromium/public/WebSecurityOrigin.h > + // Returns whether the url should be allowed to see the referrer > + // based on their respective protocols. > + WEBKIT_API static bool shouldHideReferrer(const WebURL& url, > + const WebString& referrer); No need to wrap lines like this. (In reply to comment #3) > (From update of attachment 44646 [details]) > A few style nits: > > > Index: WebKit/chromium/src/WebSecurityOrigin.cpp > > +bool WebSecurityOrigin::shouldHideReferrer(const WebURL& url, const WebString& referrer) { > > '}' should go on the next line as bot said ^^^ > > > + return SecurityOrigin::shouldHideReferrer(WebCore::KURL(url.spec(), url.parsed(), url.isValid()), > > You could simply pass WebURL into SecurityOrigin::shouldHideReferer since > WebURL has conversion operator to convert to KURL. Are you sure about this one? If I just try to pass the WebURL into SecurityOrigin::shouldHideReferrer(), I'm getting compile errors (claims it can't do the conversion). > > > + WebCore::String(referrer)); > > WebKit code usually does not wrap a long line like this. > > > Index: WebKit/chromium/public/WebSecurityOrigin.h > > > + // Returns whether the url should be allowed to see the referrer > > + // based on their respective protocols. > > + WEBKIT_API static bool shouldHideReferrer(const WebURL& url, > > + const WebString& referrer); > > No need to wrap lines like this. Created attachment 44695 [details]
patch2
Never mind, I got it. All your comments should be resolved.
style-queue ran check-webkit-style on attachment 44695 [details] without any errors.
Comment on attachment 44695 [details] patch2 > Index: WebKit/chromium/public/WebSecurityOrigin.h ... > + // Returns whether the url should be allowed to see the referrer > + // based on their respective protocols. > + WEBKIT_API static bool shouldHideReferrer(const WebURL& url, const WebString& referrer); Even though WebCore uses String for referrer, please use a WebURL here because it is in fact supposed to be an URL. The WebKit API tries to use WebURL everywhere it is true that a parameter is an URL. Also, I think this method would be better on WebSecurityPolicy. The WebKit API has policy related stuff on that interface instead of lumping them on WebSecurityOrigin. Created attachment 44700 [details]
patch3
fishd and I discussed it off thread and concluded that it makes sense to leave the referrer as a WebString.
This patch moves the change to WebSecurityPolicy, as requested.
style-queue ran check-webkit-style on attachment 44700 [details] without any errors.
> fishd and I discussed it off thread and concluded that it makes sense to leave
> the referrer as a WebString.
Just so I know for the future, what was the reasoning here?
(In reply to comment #11) > > fishd and I discussed it off thread and concluded that it makes sense to leave > > the referrer as a WebString. > > Just so I know for the future, what was the reasoning here? As I understood it, the rationale was that in WebHistoryItem and WebURLRequest (two of the places that store referrers), we are returning the referrer as a WebString, and it didn't make sense to do a WebString->WebUrl->WebString->WebCore::String translation, particularly since translating between WebString and WebURL is not well supported at this time. fishd, feel free to correct me if I misundestood the logic. :-) > fishd, feel free to correct me if I misundestood the logic. :-)
yeah, that's basically it. in general, i would prefer to pass around anything that is an url as a {Web,K,G}URL, but in this case WebString seems like the more practical choice.
(In reply to comment #13) > > fishd, feel free to correct me if I misundestood the logic. :-) > > yeah, that's basically it. in general, i would prefer to pass around anything > that is an url as a {Web,K,G}URL, but in this case WebString seems like the > more practical choice. http://trac.webkit.org/changeset/52027 |