Bug 58378 - Add WebCore::Setting to block displaying and/or running insecure content on secure pages
Summary: Add WebCore::Setting to block displaying and/or running insecure content on s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Chris Evans
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-12 14:41 PDT by Adam Barth
Modified: 2012-11-12 18:48 PST (History)
17 users (show)

See Also:


Attachments
Work in progress (no tests) (9.90 KB, patch)
2011-04-12 14:41 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (12.97 KB, patch)
2011-04-29 13:35 PDT, Chris Evans
no flags Details | Formatted Diff | Diff
Patch (12.87 KB, patch)
2011-04-29 15:30 PDT, Chris Evans
no flags Details | Formatted Diff | Diff
Patch (13.17 KB, patch)
2011-04-29 16:08 PDT, Chris Evans
no flags Details | Formatted Diff | Diff
Plumbing... (28.71 KB, patch)
2011-05-09 18:19 PDT, Chris Evans
no flags Details | Formatted Diff | Diff
Plumbing... (28.54 KB, patch)
2011-05-09 19:36 PDT, Chris Evans
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff
Plumbing #1 (4.92 KB, patch)
2011-05-10 15:16 PDT, Chris Evans
no flags Details | Formatted Diff | Diff
Plumbing #1 (5.95 KB, patch)
2011-05-10 16:26 PDT, Chris Evans
no flags Details | Formatted Diff | Diff
Plumbing #2 (35.48 KB, patch)
2011-05-11 13:29 PDT, Chris Evans
no flags Details | Formatted Diff | Diff
Plumbing #2 (35.48 KB, patch)
2011-05-12 13:10 PDT, Chris Evans
tonikitoo: review-
Details | Formatted Diff | Diff
Plumbing #3 (12.70 KB, patch)
2012-06-29 12:50 PDT, Josh Rickmar
no flags Details | Formatted Diff | Diff
Plumbing #3 (12.68 KB, patch)
2012-07-02 07:00 PDT, Josh Rickmar
buildbot: commit-queue-
Details | Formatted Diff | Diff
Plumbing #3 (17.29 KB, patch)
2012-07-02 08:01 PDT, Josh Rickmar
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Plumbing #3 (17.29 KB, patch)
2012-07-02 08:26 PDT, Josh Rickmar
mrobinson: review-
Details | Formatted Diff | Diff
Plumbing #3 (6.12 KB, patch)
2012-07-03 08:13 PDT, Josh Rickmar
no flags Details | Formatted Diff | Diff
Plumbing #3 (6.13 KB, patch)
2012-07-05 06:03 PDT, Josh Rickmar
jrick: commit-queue?
Details | Formatted Diff | Diff
Plumbing #3 (6.09 KB, patch)
2012-08-01 08:36 PDT, Josh Rickmar
gns: review-
mrobinson: commit-queue-
Details | Formatted Diff | Diff
Plumbing #3 (6.06 KB, patch)
2012-11-12 06:15 PST, Josh Rickmar
no flags Details | Formatted Diff | Diff
plumbing #3 (5.60 KB, patch)
2012-11-12 14:24 PST, Josh Rickmar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-04-12 14:41:20 PDT
Add WebCore::Setting to block running insecure content on secure pages
Comment 1 Adam Barth 2011-04-12 14:41:43 PDT
Created attachment 89276 [details]
Work in progress (no tests)
Comment 2 Chris Evans 2011-04-29 13:35:20 PDT
Created attachment 91725 [details]
Patch

Doing this one in pieces -- piece #1.
Comment 3 Adam Barth 2011-04-29 13:46:00 PDT
Comment on attachment 91725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91725&action=review

Tests?  Like many WebCore::Settings, this is a PITA to test.  There's a layoutTestController method for overriding preferences, so one approach is to wire this setting up to a private WebKit setting on mac.

> Source/WebCore/loader/MainResourceLoader.cpp:217
> +    Frame* top = m_frame->tree()->top();
> +    if (top != m_frame) {
> +        if (!frameLoader()->checkIfDisplayInsecureContent(top->document()->securityOrigin(), newRequest.url())) {
> +            cancel();
> +            return;
> +        }
> +    }
> +
>      ResourceLoader::willSendRequest(newRequest, redirectResponse);

The problem is that willSendRequest can change the URL.  Can we move this check back after willSendRequest?  Also, I'm not 100% sure how to cancel the request at this stage.  If you look below, redirectResponse.isNull seems to be the "is canceled" bit.

> Source/WebCore/loader/FrameLoader.h:260
> +    // These two return false if policy dictates that the resource load should
> +    // not continue.

I'd skip this comment.
Comment 4 Chris Evans 2011-04-29 14:31:19 PDT
- Tests. Yep! Definitely going to wire in the new settings and do tests. I was planning on doing this in an immediate follow-up change in the same way that we handled the "file access from files" setting. It's nice to separate the noise of all the platform-specific test setting wiring.

- FrameLoader.h comment: removed as requested.

- Canceling in MainResourceLoader.cpp.
Do we care that willSendRequest() can change the URL? It seems that we shouldn't, as:
a) The main case for that looks to be a redirect, which is a not a situation we'll hit since we're stopping the request going out.
b) The cancel() machinery isn't interested in the URL. It does seem to gracefully handle cancellation at any point, including this one.

The reason I moved the check up to before willSendRequest() is that it seems perverse to call willSendRequest() when in fact we want to be sure that we won't. Without the move, the network inspector _does_ show a canceled HTTP load for the resource. It doesn't appear that the resource fetch ever hits the network, but this is not something we wish to risk. I think it's safest if the callbacks in willSendRequest() never fire.
Comment 5 Adam Barth 2011-04-29 14:35:53 PDT
We probably need to check both before and after willSendRequest in that case.
Comment 6 Adam Barth 2011-04-29 14:36:56 PDT
> - Tests. Yep! Definitely going to wire in the new settings and do tests. I was planning on doing this in an immediate follow-up change in the same way that we handled the "file access from files" setting. It's nice to separate the noise of all the platform-specific test setting wiring.

I think that's ok, but be warned that I'll start secretly removing chairs from your office until you write the tests.  ;)
Comment 7 Chris Evans 2011-04-29 15:30:49 PDT
Created attachment 91751 [details]
Patch
Comment 8 Chris Evans 2011-04-29 15:35:52 PDT
Re: "We probably need to check both before and after willSendRequest in that case."

I believe the current patch is correct. If we take a redirect, we'll re-enter willSendRequest (and again and again etc. as we take more and more redirects). I confirmed this with a bit of debugging.

So we'll always do the check before sending out the request for every redirect in the chain (and block and stop at the first instance of any https -> http transition).

Of course, this is all indicating a great test. I don't think the existing mixed-content-load tests check for the https -> <iframe src="https"> -> 302 http case. I will add coverage for that alongside the upcoming new tests.
Comment 9 Adam Barth 2011-04-29 15:37:25 PDT
Comment on attachment 91751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91751&action=review

> Source/WebCore/ChangeLog:6
> +        Add WebCore::Setting to block displaying and/or running insecure content on secure pages
> +        https://bugs.webkit.org/show_bug.cgi?id=58378

It would be nice to add information to the ChangeLog about how you plan to wire up tests in a followup patch.

> Source/WebCore/loader/MainResourceLoader.cpp:215
> +    Frame* top = m_frame->tree()->top();
> +    if (top != m_frame) {
> +        if (!frameLoader()->checkIfDisplayInsecureContent(top->document()->securityOrigin(), newRequest.url())) {
> +            cancel();
> +            return;
> +        }
> +    }

I thought we decided to do this check both before and after willSendRequest...
Comment 10 Adam Barth 2011-04-29 15:43:53 PDT
Comment on attachment 91751 [details]
Patch

Ok.  That sounds reasonable.  I look forward to the tests.  (The comments above would also be great to add to the ChangeLog.)
Comment 11 Chris Evans 2011-04-29 16:08:36 PDT
Created attachment 91762 [details]
Patch
Comment 12 WebKit Commit Bot 2011-04-29 18:16:43 PDT
Comment on attachment 91762 [details]
Patch

Clearing flags on attachment: 91762

Committed r85378: <http://trac.webkit.org/changeset/85378>
Comment 13 WebKit Commit Bot 2011-04-29 18:16:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Chris Evans 2011-04-29 18:19:16 PDT
Re-opening; the new option per-platform wiring and tests are in-progress.
Comment 15 WebKit Commit Bot 2011-04-29 18:59:10 PDT
The commit-queue encountered the following flaky tests while processing attachment 91762 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: dumi@chromium.org)
security/block-test.html bug 55741 (authors: beidson@apple.com, mrowe@apple.com, and sam@webkit.org)
The commit-queue is continuing to process your patch.
Comment 16 Chris Evans 2011-05-09 18:19:19 PDT
Created attachment 92901 [details]
Plumbing...
Comment 17 Chris Evans 2011-05-09 18:19:59 PDT
Using EWS to make sure plumbing in the settings hasn't bust the ports... not soliciting review until they're green :)
Comment 18 Chris Evans 2011-05-09 19:36:56 PDT
Created attachment 92909 [details]
Plumbing...
Comment 19 Collabora GTK+ EWS bot 2011-05-09 23:19:18 PDT
Comment on attachment 92909 [details]
Plumbing...

Attachment 92909 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8646859
Comment 20 Chris Evans 2011-05-10 15:16:00 PDT
Created attachment 93024 [details]
Plumbing #1
Comment 21 Adam Barth 2011-05-10 15:20:44 PDT
Comment on attachment 93024 [details]
Plumbing #1

This looks fine, but we need a changelog.
Comment 22 Chris Evans 2011-05-10 16:26:31 PDT
Created attachment 93041 [details]
Plumbing #1
Comment 23 WebKit Commit Bot 2011-05-10 18:24:27 PDT
Comment on attachment 93041 [details]
Plumbing #1

Clearing flags on attachment: 93041

Committed r86201: <http://trac.webkit.org/changeset/86201>
Comment 24 WebKit Commit Bot 2011-05-10 18:24:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Chris Evans 2011-05-10 18:25:28 PDT
More to do.
Comment 26 WebKit Review Bot 2011-05-10 18:48:23 PDT
http://trac.webkit.org/changeset/86201 might have broken SnowLeopard Intel Release (Build)
Comment 27 Chris Evans 2011-05-11 13:29:09 PDT
Created attachment 93167 [details]
Plumbing #2
Comment 28 Adam Barth 2011-05-11 14:44:51 PDT
Comment on attachment 93167 [details]
Plumbing #2

View in context: https://bugs.webkit.org/attachment.cgi?id=93167&action=review

> Source/WebKit/qt/Api/qwebsettings.h:82
> -        HyperlinkAuditingEnabled
> +        HyperlinkAuditingEnabled,
> +        PagesCanDisplayInsecureContent,
> +        PagesCanRunInsecureContent

We shouldn't add APIs to Qt without approval from the Qt folks.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3289
> +    settings->setAllowDisplayOfInsecureContent(enableDisplayOfInsecureContent);
> +    settings->setAllowRunningOfInsecureContent(enableRunningOfInsecureContent);

GTK, I'm less sure what they'd like to do.  I'll CC folks.
Comment 29 Adam Barth 2011-05-11 14:45:31 PDT
Thoughts on Qt / GTK settings APIs?
https://bugs.webkit.org/attachment.cgi?id=93167&action=review
Comment 30 Chris Evans 2011-05-11 14:49:11 PDT
I'm basically copying the CL for adding the "allow file access from files" setting. We did help out the Qt / GTK folks in that CL.

I'm fine to not touch various ports, but won't that break the layout tests right out of the door due to missing layoutTestController.setAllowRunningOfInsecureContent() ?
Comment 31 Adam Barth 2011-05-11 14:53:12 PDT
> I'm fine to not touch various ports, but won't that break the layout tests right out of the door due to missing layoutTestController.setAllowRunningOfInsecureContent() ?

Yeah, the two choices are to skip the tests on Qt and GTK or to implement the settings API for them.  In this case, we probably want to implement the API.  I just want to make sure we're making the choice that folks interested in those ports want us to make.
Comment 32 Martin Robinson 2011-05-11 15:12:18 PDT
(In reply to comment #31)
> > I'm fine to not touch various ports, but won't that break the layout tests right out of the door due to missing layoutTestController.setAllowRunningOfInsecureContent() ?
> 
> Yeah, the two choices are to skip the tests on Qt and GTK or to implement the settings API for them.  In this case, we probably want to implement the API.  I just want to make sure we're making the choice that folks interested in those ports want us to make.

If you wish to have GTK+ pass the tests, but delay the decision about whether or not to add new API, you can simply add more methods to Source/WebKit/gtk/DumpRenderTreeSupportGtk.h class.
Comment 33 Chris Evans 2011-05-11 17:54:50 PDT
Given that it's going to take a while longer to iterate through the EWS bots (GTK in particular is v. slow), I'm tending towards waiting for comment from the QT / GTK ports on the full wiring-in of the new option. It basically just models wiring added to all the other ports.

I'd love to get comment quickly enough to land this tonight or tomorrow. I'm very eager to land the tests for the new blocking :)
Comment 34 Adam Barth 2011-05-11 18:15:50 PDT
What about the approach mrobinson suggests in Comment #32?
Comment 35 Chris Evans 2011-05-11 18:20:07 PDT
All it needs is an aye from a GTK dev and then we can roll with the existing patch and no-one has to revisit later.
Comment 36 Antonio Gomes 2011-05-12 07:17:56 PDT
> If you wish to have GTK+ pass the tests, but delay the decision about whether or not to add new API, you can simply add more methods to Source/WebKit/gtk/DumpRenderTreeSupportGtk.h class.

And similarly DumpRenderTreeSupportQt.h/cpp for Qt.
Comment 37 Chris Evans 2011-05-12 13:10:30 PDT
Created attachment 93326 [details]
Plumbing #2

This one should go all green.
Comment 38 Antonio Gomes 2011-05-12 21:05:16 PDT
Comment on attachment 93326 [details]
Plumbing #2

QtWebKit new APIs need discussion in qtwebkit mailing list before anything. Not sure about other ports ...
Comment 39 Eric Seidel (no email) 2011-06-18 13:33:55 PDT
Comment on attachment 91751 [details]
Patch

Cleared Adam Barth's review+ from obsolete attachment 91751 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 40 Eric Seidel (no email) 2011-06-18 13:34:00 PDT
Comment on attachment 93024 [details]
Plumbing #1

Cleared Adam Barth's review+ from obsolete attachment 93024 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 41 Marco Peereboom 2011-09-30 07:32:54 PDT
I was in the act of writing a similar patch when I ran across this.  Is there any reason why this isn't moving forward?  I really really want to be able to disable running remote scripts and downloading remote content.  This is one of the preconditions of BEAST and I want to be able to control that in my browser.
Comment 42 Adam Barth 2011-09-30 08:44:58 PDT
> Is there any reason why this isn't moving forward?

I think we just got stalled on getting approval from the GTK and Qt folks to add this API.
Comment 43 Martin Robinson 2011-09-30 08:46:48 PDT
(In reply to comment #42)
> > Is there any reason why this isn't moving forward?
> 
> I think we just got stalled on getting approval from the GTK and Qt folks to add this API.

GTK+ and Qt both have DumpRenderTreeSupport, which is like a hacky version of .internals. This patch can use that and GTK+/Qt API decisions until later.
Comment 44 Adam Barth 2011-09-30 08:50:36 PDT
> GTK+ and Qt both have DumpRenderTreeSupport, which is like a hacky version of .internals. This patch can use that and GTK+/Qt API decisions until later.

Sure, or we could just use window.internals itself.  I think marco was asking about exposing this feature to his application, which presumably uses GTK+ or Qt.
Comment 45 Marco Peereboom 2011-09-30 08:54:47 PDT
> Sure, or we could just use window.internals itself.  I think marco was asking about exposing this feature to his application, which presumably uses GTK+ or Qt.

Right, I use GTK+ and I really want it :-)
Comment 46 Loganaden Velvindron 2011-10-06 08:44:07 PDT
That would be really nice to have. It could
allow some nice security features to be developed :-)
Comment 47 Marco Peereboom 2012-01-04 16:26:58 PST
ping?
Comment 48 Marco Peereboom 2012-01-26 07:33:45 PST
Don't want to sound like a nag but is this patch going in?
Comment 49 Eric Seidel (no email) 2012-01-26 11:08:55 PST
It looks like this patch was long-ago abandoned and needs an updated version.  Anyone (not just Chris) could post such.
Comment 50 Marco Peereboom 2012-02-11 09:01:53 PST
I am willing to redo this patch if there is interest.  I tried it this morning and it isn't too badly out of whack.

The reason I really believe this should move forward is to be able to stave off things such as the BEAST attack.

So, if I redo the patch to apply to the current trunk does it stand a chance of getting committed?
Comment 51 Owais Lone 2012-06-27 23:25:29 PDT
(In reply to comment #50)
> I am willing to redo this patch if there is interest.  I tried it this morning and it isn't too badly out of whack.
> 
> The reason I really believe this should move forward is to be able to stave off things such as the BEAST attack.
> 
> So, if I redo the patch to apply to the current trunk does it stand a chance of getting committed?


That would be really nice of you. This is blocking a lot of possible features in Gtk/Qt apps.
Comment 52 Owais Lone 2012-06-27 23:38:04 PDT
Ping!
Comment 53 Josh Rickmar 2012-06-29 12:50:40 PDT
Created attachment 150237 [details]
Plumbing #3

Marco asked me to update this patch.  This is my first time doing anything in the WebKit codebase, so forgive me if my patch is wrong (and let me know how to fix it!)  This patch updates the last one and adds the knobs to the GTK API.
Comment 54 Josh Rickmar 2012-07-02 07:00:52 PDT
Created attachment 150416 [details]
Plumbing #3
Comment 55 Josh Rickmar 2012-07-02 07:01:25 PDT
Comment on attachment 150416 [details]
Plumbing #3

Fixed+updated patch
Comment 56 Build Bot 2012-07-02 07:21:08 PDT
Comment on attachment 150416 [details]
Plumbing #3

Attachment 150416 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13133109
Comment 57 Build Bot 2012-07-02 07:54:46 PDT
Comment on attachment 150416 [details]
Plumbing #3

Attachment 150416 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13133113
Comment 58 Josh Rickmar 2012-07-02 08:01:31 PDT
Created attachment 150422 [details]
Plumbing #3
Comment 59 Gyuyoung Kim 2012-07-02 08:04:43 PDT
Comment on attachment 150422 [details]
Plumbing #3

Attachment 150422 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13137083
Comment 60 Josh Rickmar 2012-07-02 08:26:38 PDT
Created attachment 150427 [details]
Plumbing #3
Comment 61 Raphael Kubo da Costa (:rakuco) 2012-07-02 20:55:56 PDT
You probably need to mention which version these new webkit-gtk settings were introduced in. Other than that, it's up to the GTK+ port maintainers to take a look at this, but I don't know if they're actively working (or accepting new features) into their WebKit1 API.
Comment 62 Marco Peereboom 2012-07-03 06:56:14 PDT
This knob is pretty important in order to prevent the BEAST attack.  I spoke with some other folks doing GTK based web browsers and basically everyone wants this.  So I'd like to plead with the GTK folk to push this one in.
Comment 63 Josh Rickmar 2012-07-03 08:13:57 PDT
Created attachment 150612 [details]
Plumbing #3

After some discussion with mrobinson, here's another patch without any of the DumpRenderTree stuff since it didn't introduce any new tests.
Comment 64 Martin Robinson 2012-07-03 08:22:16 PDT
Comment on attachment 150427 [details]
Plumbing #3

View in context: https://bugs.webkit.org/attachment.cgi?id=150427&action=review

If you'd like your patch you be reviewed, when posting it you should change the "r" field to say "r?"

I'm not sure why you're adding hooks for this into DumpRenderTree, but we talked about that yesterday. Perhaps it would be better to add unit tests for GTK+ in Source/WebKit/gtk/tests.

The final thing is that maybe this should be only a single setting instead of two. Something like allow-display-and-running-of-insecure-content. I'm not sure about that.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1020
> +    * Boolean property to control whether insecure (non-https) images / frames 
> +    * can be loaded from https pages.

Nit: Should be HTTPS not https. It's probably not necessary to say this a boolean property, you could simply write.

Whether is it possible for pages loaded via HTTPS to display subresources, such as images and frames, from non-HTTPS URLs.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1026
> +                                                         _("Controls load of non-https display resources from https pages"),

How about:

Whether non-HTTPS resources can display on HTTPS pages.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1034
> +    * Boolean property to control whether insecure (non-https) script / CSS / 
> +    * plug-ins can be loaded from https pages.

Same here you could write something like:

Whether is it possible for pages loaded via HTTPS to run subresources, such as CSS, scripts and plugin, from non-HTTPS URLs.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1040
> +                                                         _("Controls load of non-https script / plug-in resources from https pages."),

Whether non-HTTPS resources can run on HTTPS pages.
Comment 65 Marco Peereboom 2012-07-03 08:31:04 PDT
> The final thing is that maybe this should be only a single setting instead of two. Something like allow-display-and-running-of-insecure-content. I'm not sure about that.

I'd prefer two settings in order to be able to fine tune the behavior.
Comment 66 Martin Robinson 2012-07-03 09:13:12 PDT
(In reply to comment #64)

> I'm not sure why you're adding hooks for this into DumpRenderTree, but we talked about that yesterday. Perhaps it would be better to add unit tests for GTK+ in Source/WebKit/gtk/tests.

It seems I was wrong about this, but the tests that exist are platform-specific. Not sure what the right approach is...perhaps moving them to a platform independent directory and skipping them on all other platforms.

platform/chromium/http/tests/security/mixedContent/insecure-image-in-main-frame-allowed.html
11:    testRunner.setAllowDisplayOfInsecureContent(true);

platform/chromium/http/tests/security/mixedContent/insecure-iframe-in-main-frame-allowed.html
11:    testRunner.setAllowDisplayOfInsecureContent(true);
Comment 67 Josh Rickmar 2012-07-05 06:03:00 PDT
Created attachment 150931 [details]
Plumbing #3

No tests on this one, but this includes all the other fixes that have been mentioned.
Comment 68 Josh Rickmar 2012-07-26 10:42:42 PDT
Bump... can we get another look at this patch?
Comment 69 Gyuyoung Kim 2012-07-26 20:30:52 PDT
Comment on attachment 150931 [details]
Plumbing #3

View in context: https://bugs.webkit.org/attachment.cgi?id=150931&action=review

> Source/WebKit/gtk/ChangeLog:3
> +        Reviewed by NOBODY (OOPS!).

Generally, "Reviewed by NOBODY" is placed below bug title and url.

> Source/WebKit/gtk/ChangeLog:5
> +        [GTK] Add WebCore::Setting to block displaying and/or running

There is no "[GTK]" in bug title. Generally, bug title has been written in a line.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1029
> +                                                         TRUE,

Though I'm not export for GTK port, I wonder if WK1 GTK port enables insecure-content by default. Does *TRUE* mean this feature is enabled by default ?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1046
> +                                                         TRUE,

ditto.
Comment 70 Josh Rickmar 2012-08-01 08:36:23 PDT
Created attachment 155814 [details]
Plumbing #3

Cooked up a new patch with Gyuyoung's suggestions.
Comment 71 Josh Rickmar 2012-08-01 08:37:55 PDT
> Though I'm not export for GTK port, I wonder if WK1 GTK port enables insecure-content by default. Does *TRUE* mean this feature is enabled by default ?

That is correct.  True is the default value, and keeps the current behavior.
Comment 72 Marco Peereboom 2012-08-08 05:39:50 PDT
Can we please get this in?
Comment 73 Eric Seidel (no email) 2012-10-31 22:47:27 PDT
Comment on attachment 155814 [details]
Plumbing #3

I'm happy to rubber-stamp this Gtk patch.  It looks reasonable.  If Gtk folks disagree, feel free to r-.
Comment 74 Martin Robinson 2012-11-01 06:10:28 PDT
Comment on attachment 155814 [details]
Plumbing #3

Cannot use the commit-queue unfortunately since the "Since:" annotation is incorrect now. This looks good to me, but API changes need review from at least two GTK+ reviewers.
Comment 75 Gustavo Noronha (kov) 2012-11-01 06:24:29 PDT
Comment on attachment 155814 [details]
Plumbing #3

The new API looks OK to me, let's wait for a second review, I'll set r- instead of setting cq+ because of the required changes above.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1020
> +    * Whether is it possible for pages loaded via HTTPS to display
> +    * subresources, such as images and frames, from non-HTTPS URLs.

Can we reword this to "Whether pages loaded via HTTPS should load subresources such as images and frames from non-HTTPS URLs."?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1022
> +    * Since: 1.9.5

Since tags should say 2.0, since that's going to be the next stable release.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1037
> +    * Whether is it possible for pages loaded via HTTPS to run
> +    * subresources, such as CSS, scripts and plugins, from non-HTTPS
> +    * URLs.

Same rewording here would be good.
Comment 76 Josh Rickmar 2012-11-12 06:15:21 PST
Created attachment 173635 [details]
Plumbing #3
Comment 77 WebKit Review Bot 2012-11-12 06:18:38 PST
Attachment 173635 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:995:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:997:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:998:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:999:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1000:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1013:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1014:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1015:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1016:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 10 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 78 Martin Robinson 2012-11-12 08:28:14 PST
(In reply to comment #77)
> Attachment 173635 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:995:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:997:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:998:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:999:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1000:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1013:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1014:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1015:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1016:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 10 in 4 files
> 

Do you mind fixing these style errors? The indentation rules changed on us. Now things need to look like this:

function(argument
    argument,
    argument);

or
function(
    argument,
    argument,
    argument);
Comment 79 Josh Rickmar 2012-11-12 14:24:49 PST
Created attachment 173727 [details]
plumbing #3

fixed indenting
Comment 80 WebKit Review Bot 2012-11-12 18:48:17 PST
Comment on attachment 173727 [details]
plumbing #3

Clearing flags on attachment: 173727

Committed r134339: <http://trac.webkit.org/changeset/134339>
Comment 81 WebKit Review Bot 2012-11-12 18:48:25 PST
All reviewed patches have been landed.  Closing bug.