Plumb mixed script URL to FrameLoaderClient
Created attachment 78833 [details] Patch
Attachment 78833 [details] did not build on qt: Build output: http://queues.webkit.org/results/7507019
Created attachment 78845 [details] Patch
Comment on attachment 78845 [details] Patch I'm terribly worried that I've missed updating some override site because the compiler doesn't warn about that. Although I believe that the tests will catch it. I've deliberately not bothered to plumb this into the ObjC and Windows IDL code because I didn't need to get this value there. However, if there's a general policy that the functions should match then I can do. cq- because I've still to find a mac to run the layout tests on.
Comment on attachment 78845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78845&action=review This looks fine, module changing String -> URL and the extra changelog entries. It's fine not to plumb through objective C. That's an API choice for that port. We'll also need fishd to review the WebKit API change. > Tools/ChangeLog:12 > +2011-01-13 Adam Langley <agl@chromium.org> You've got two changelog entries. > Tools/DumpRenderTree/chromium/WebViewHost.h:202 > - virtual void didRunInsecureContent(WebKit::WebFrame*, const WebKit::WebSecurityOrigin&); > + virtual void didRunInsecureContent(WebKit::WebFrame*, const WebKit::WebSecurityOrigin&, const WebKit::WebString& target_url); I tink we use webkit-style names here. > WebKit/chromium/public/WebFrameClient.h:279 > - virtual void didRunInsecureContent(WebFrame*, const WebSecurityOrigin&) { } > + virtual void didRunInsecureContent(WebFrame*, const WebSecurityOrigin&, const WebString&) { } As I commented in the Chromium bug, this should probably be a WebURL.
This is not a refactoring - how can it be "Covered by http/tests/security/mixedContent/*"? If I understand the patch correctly, the explanation is that is doesn't change behavior, only allowing for better error reporting in the client. Ideally, DumpRenderTree for chromium would be enhanced accordingly, and there would be a regression test.
(In reply to comment #6) > This is not a refactoring - how can it be "Covered by http/tests/security/mixedContent/*"? If I understand the patch correctly, the explanation is that is doesn't change behavior, only allowing for better error reporting in the client. The danger of this patch is that, by changing the underlying method signature, overrides will no longer function. That case should be caught by the tests. > Ideally, DumpRenderTree for chromium would be enhanced accordingly, and there would be a regression test. I can do something like the following in Chromium's DRT: printf("didRunInsecureContent %s://%s:%d %s\n", origin.protocol().utf8().data(), origin.host().utf8().data(), origin.port(), targetURL.spec().data()); But that results in output like: didRunInsecureContent https://127.0.0.1:8443 http://127.0.0.1:8080/security/resources/redir.php?url=https://127.0.0.1:8443/security/mixedContent/resources/script.js I could omit printing the origin, but the URL is still going to encode the IP address and port. These are things that I don't think should be in the test expectations but I'll do whatever is advised.
Created attachment 79496 [details] Patch
Attachment 79496 [details] did not build on win: Build output: http://queues.webkit.org/results/7496227
Attachment 79496 [details] did not build on qt: Build output: http://queues.webkit.org/results/7539231
Created attachment 79515 [details] Patch
Attachment 79515 [details] did not build on mac: Build output: http://queues.webkit.org/results/7633082
(In reply to comment #12) > Attachment 79515 [details] did not build on mac: > Build output: http://queues.webkit.org/results/7633082 This appears to be a flake. I've built the patch on OS X and all layout tests pass.
Looks good to me, but we'll need fishd to give the official review because this changes the Chromium WebKit API.
Yes, you're right: /Developer/usr/bin/../libexec/gcc/i686-apple-darwin10/4.2.1/as: can't fork a new process to execute: /Developer/usr/bin/../libexec/gcc/darwin/x86_64/as (Resource temporarily unavailable) /Projects/MacEWS/Source/WebKit2/WebProcess/Authentication/AuthenticationManager.cpp:104: fatal error: error writing to -: Broken pipe That's concerning.
Comment on attachment 79515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79515&action=review Chromium WebKit API changes LGTM modulo this one nit. > Source/WebKit/chromium/public/WebFrameClient.h:279 > + virtual void didRunInsecureContent(WebFrame*, const WebSecurityOrigin&, const WebURL&) { } can you give this WebURL parameter a descriptive name? it is not obvious what it refers to. targetURL seems good, or insecureURL might be better.
Created attachment 80698 [details] Patch
Comment on attachment 80698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80698&action=review > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1160 > -void FrameLoaderClientImpl::didRunInsecureContent(SecurityOrigin* origin) > +void FrameLoaderClientImpl::didRunInsecureContent(SecurityOrigin* origin, const KURL& targetURL) > { > if (m_webFrame->client()) > - m_webFrame->client()->didRunInsecureContent(m_webFrame, WebSecurityOrigin(origin)); > + m_webFrame->client()->didRunInsecureContent(m_webFrame, WebSecurityOrigin(origin), targetURL); > } Its kind of lame that the parameter changes name. Seems like it should be insecureURL everywhere...
(In reply to comment #18) > Its kind of lame that the parameter changes name. Seems like it should be insecureURL everywhere... I like using that name everywhere. However, I'll just point out that it is OK for terms used in the WebKit API to differ from terms used in WebCore. It is nice for them to be the same, but WebCore can and does change out from underneath the WebKit API. So, terms that were once consistent may end up becoming inconsistent over time. What is more important is that the WebKit API stand on its own and make sense without the context of whatever WebCore may be doing.
Created attachment 81095 [details] updated targetURL to insecureURL
Comment on attachment 81095 [details] updated targetURL to insecureURL Thanks!
Comment on attachment 81095 [details] updated targetURL to insecureURL Clearing flags on attachment: 81095 Committed r77602: <http://trac.webkit.org/changeset/77602>
All reviewed patches have been landed. Closing bug.