Bug 52384 - Plumb mixed script URL to FrameLoaderClient
: Plumb mixed script URL to FrameLoaderClient
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-01-13 11:25 PST by
Modified: 2011-02-03 21:30 PST (History)


Attachments
Patch (18.47 KB, patch)
2011-01-13 11:26 PST, Adam Langley
no flags Review Patch | Details | Formatted Diff | Diff
Patch (30.12 KB, patch)
2011-01-13 12:46 PST, Adam Langley
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.38 KB, patch)
2011-01-19 15:05 PST, Adam Langley
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.39 KB, patch)
2011-01-19 16:20 PST, Adam Langley
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.66 KB, patch)
2011-01-31 16:21 PST, Adam Langley
no flags Review Patch | Details | Formatted Diff | Diff
updated targetURL to insecureURL (26.70 KB, patch)
2011-02-03 11:55 PST, Adam Langley
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-01-13 11:25:57 PST
Plumb mixed script URL to FrameLoaderClient
------- Comment #1 From 2011-01-13 11:26:24 PST -------
Created an attachment (id=78833) [details]
Patch
------- Comment #2 From 2011-01-13 12:01:49 PST -------
Attachment 78833 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7507019
------- Comment #3 From 2011-01-13 12:46:39 PST -------
Created an attachment (id=78845) [details]
Patch
------- Comment #4 From 2011-01-13 14:03:11 PST -------
(From update of attachment 78845 [details])
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 #5 From 2011-01-13 14:33:21 PST -------
(From update of attachment 78845 [details])
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.
------- Comment #6 From 2011-01-13 21:48:22 PST -------
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.
------- Comment #7 From 2011-01-19 14:59:50 PST -------
(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.
------- Comment #8 From 2011-01-19 15:05:50 PST -------
Created an attachment (id=79496) [details]
Patch
------- Comment #9 From 2011-01-19 15:40:12 PST -------
Attachment 79496 [details] did not build on win:
Build output: http://queues.webkit.org/results/7496227
------- Comment #10 From 2011-01-19 15:42:00 PST -------
Attachment 79496 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7539231
------- Comment #11 From 2011-01-19 16:20:02 PST -------
Created an attachment (id=79515) [details]
Patch
------- Comment #12 From 2011-01-19 21:26:50 PST -------
Attachment 79515 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7633082
------- Comment #13 From 2011-01-20 08:39:18 PST -------
(In reply to comment #12)
> Attachment 79515 [details] [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.
------- Comment #14 From 2011-01-20 11:00:57 PST -------
Looks good to me, but we'll need fishd to give the official review because this changes the Chromium WebKit API.
------- Comment #15 From 2011-01-20 13:40:40 PST -------
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 #16 From 2011-01-31 15:17:31 PST -------
(From update of attachment 79515 [details])
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.
------- Comment #17 From 2011-01-31 16:21:33 PST -------
Created an attachment (id=80698) [details]
Patch
------- Comment #18 From 2011-02-01 11:21:50 PST -------
(From update of attachment 80698 [details])
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...
------- Comment #19 From 2011-02-01 14:15:17 PST -------
(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.
------- Comment #20 From 2011-02-03 11:55:20 PST -------
Created an attachment (id=81095) [details]
updated targetURL to insecureURL
------- Comment #21 From 2011-02-03 12:22:48 PST -------
(From update of attachment 81095 [details])
Thanks!
------- Comment #22 From 2011-02-03 21:30:23 PST -------
(From update of attachment 81095 [details])
Clearing flags on attachment: 81095

Committed r77602: <http://trac.webkit.org/changeset/77602>
------- Comment #23 From 2011-02-03 21:30:32 PST -------
All reviewed patches have been landed.  Closing bug.