WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
52384
Plumb mixed script URL to FrameLoaderClient
https://bugs.webkit.org/show_bug.cgi?id=52384
Summary
Plumb mixed script URL to FrameLoaderClient
Adam Langley
Reported
2011-01-13 11:25:57 PST
Plumb mixed script URL to FrameLoaderClient
Attachments
Patch
(18.47 KB, patch)
2011-01-13 11:26 PST
,
Adam Langley
no flags
Details
Formatted Diff
Diff
Patch
(30.12 KB, patch)
2011-01-13 12:46 PST
,
Adam Langley
no flags
Details
Formatted Diff
Diff
Patch
(27.38 KB, patch)
2011-01-19 15:05 PST
,
Adam Langley
no flags
Details
Formatted Diff
Diff
Patch
(27.39 KB, patch)
2011-01-19 16:20 PST
,
Adam Langley
no flags
Details
Formatted Diff
Diff
Patch
(26.66 KB, patch)
2011-01-31 16:21 PST
,
Adam Langley
no flags
Details
Formatted Diff
Diff
updated targetURL to insecureURL
(26.70 KB, patch)
2011-02-03 11:55 PST
,
Adam Langley
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Adam Langley
Comment 1
2011-01-13 11:26:24 PST
Created
attachment 78833
[details]
Patch
Early Warning System Bot
Comment 2
2011-01-13 12:01:49 PST
Attachment 78833
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/7507019
Adam Langley
Comment 3
2011-01-13 12:46:39 PST
Created
attachment 78845
[details]
Patch
Adam Langley
Comment 4
2011-01-13 14:03:11 PST
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.
Adam Barth
Comment 5
2011-01-13 14:33:21 PST
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.
Alexey Proskuryakov
Comment 6
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.
Adam Langley
Comment 7
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.
Adam Langley
Comment 8
2011-01-19 15:05:50 PST
Created
attachment 79496
[details]
Patch
Build Bot
Comment 9
2011-01-19 15:40:12 PST
Attachment 79496
[details]
did not build on win: Build output:
http://queues.webkit.org/results/7496227
Early Warning System Bot
Comment 10
2011-01-19 15:42:00 PST
Attachment 79496
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/7539231
Adam Langley
Comment 11
2011-01-19 16:20:02 PST
Created
attachment 79515
[details]
Patch
Eric Seidel (no email)
Comment 12
2011-01-19 21:26:50 PST
Attachment 79515
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/7633082
Adam Langley
Comment 13
2011-01-20 08:39:18 PST
(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.
Adam Barth
Comment 14
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.
Eric Seidel (no email)
Comment 15
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.
Darin Fisher (:fishd, Google)
Comment 16
2011-01-31 15:17:31 PST
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.
Adam Langley
Comment 17
2011-01-31 16:21:33 PST
Created
attachment 80698
[details]
Patch
Adam Barth
Comment 18
2011-02-01 11:21:50 PST
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...
Darin Fisher (:fishd, Google)
Comment 19
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.
Adam Langley
Comment 20
2011-02-03 11:55:20 PST
Created
attachment 81095
[details]
updated targetURL to insecureURL
Adam Barth
Comment 21
2011-02-03 12:22:48 PST
Comment on
attachment 81095
[details]
updated targetURL to insecureURL Thanks!
WebKit Commit Bot
Comment 22
2011-02-03 21:30:23 PST
Comment on
attachment 81095
[details]
updated targetURL to insecureURL Clearing flags on attachment: 81095 Committed
r77602
: <
http://trac.webkit.org/changeset/77602
>
WebKit Commit Bot
Comment 23
2011-02-03 21:30:32 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug