Bug 219257

Created attachment 414801 [details] Tentative patch (untested)

Comment on attachment 414801 [details] Tentative patch (untested) It looks like the API tests pass after this change, so I'm asking review now. Will update the changelog later, but basically this is doing Michael's suggestion in bug 218623 comment 15, so that the test won't be broken when we stop treating loopback IP address as mixed content.

Comment on attachment 414801 [details] Tentative patch (untested) View in context: https://bugs.webkit.org/attachment.cgi?id=414801&action=review > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:35 > + ServerNonLocalHost = 1 << 3, If you call it ServerNonLoopback instead, this would be a lot less weird. Because passing ServerNonLocalHost to cause the server to use localhost is weird. ;)

Comment on attachment 414801 [details] Tentative patch (untested) View in context: https://bugs.webkit.org/attachment.cgi?id=414801&action=review LGTM. I see TestSSL now needs two HTTP servers instead of one, but that's fine. I bet Carlos Garcia will want to review this before it lands. > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:38 > - WebKitTestServer(ServerOptions = ServerHTTP); > + WebKitTestServer(uint8_t options = ServerHTTP); I don't like this... why are you removing its nice type?

(In reply to Michael Catanzaro from comment #3) > Comment on attachment 414801 [details] > Tentative patch (untested) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=414801&action=review > > > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:35 > > + ServerNonLocalHost = 1 << 3, > > If you call it ServerNonLoopback instead, this would be a lot less weird. > Because passing ServerNonLocalHost to cause the server to use localhost is > weird. ;) Right, but ultimately I want to call it ServerNonLocalHost as we want to stop treating localhost as mixed content too. I'm fine keeping this name for now... but maybe people would add new tests assuming it's just not loopback. (In reply to Michael Catanzaro from comment #4) > Comment on attachment 414801 [details] > Tentative patch (untested) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=414801&action=review > > LGTM. I see TestSSL now needs two HTTP servers instead of one, but that's > fine. Actually three (2 HTTPS and HTTP). I can also change the tests to always use the new version HTTPS version, but not sure that really makes a difference. > I bet Carlos Garcia will want to review this before it lands. > > > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:38 > > - WebKitTestServer(ServerOptions = ServerHTTP); > > + WebKitTestServer(uint8_t options = ServerHTTP); > > I don't like this... why are you removing its nice type? ServerOptions is currently an enum of bit flags, so it can't accept a combination ServerHttps | ServerNonLoopback.

(In reply to Frédéric Wang (:fredw) from comment #5) > ServerOptions is currently an enum of bit flags, so it can't accept a > combination ServerHttps | ServerNonLoopback. Try: static_cast<ServerOptions>(ServerHttps | ServerNonLoopback)

Or better: try std::bitset!

Created attachment 414835 [details] Patch (reverted)

(In reply to Michael Catanzaro from comment #3) > If you call it ServerNonLoopback instead, this would be a lot less weird. > Because passing ServerNonLocalHost to cause the server to use localhost is > weird. ;) Done (In reply to Michael Catanzaro from comment #4) > LGTM. I see TestSSL now needs two HTTP servers instead of one, but that's > fine. Merged to use only one HTTPS server, let's see if that does not break any test... (In reply to Michael Catanzaro from comment #7) > Or better: try std::bitset! Done.

Committed r270170: <https://trac.webkit.org/changeset/270170> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414835 [details].

Re-opened since this is blocked by bug 220922

<rdar://problem/73570185>

(In reply to Frédéric Wang (:fredw) from comment #9) > (In reply to Michael Catanzaro from comment #7) > > Or better: try std::bitset! > > Done. This change was problematic, std::bitset's constructor takes a bit mask but its setter/getter relies on the bit's position instead. I've extracted this change to bug 222273.

Created attachment 421409 [details] Patch I tried a bit to rebase, but was not able to make the certificate stuff work. Probably some special config is needed for the domain name...

Removing myself from assignee since I'm not working on this anymore.