Remove some code, an obsolete criteria, from SecurityOrigin test for uniqueness.

Remove some code, an obsolete criteria, from SecurityOrigin test for uniqueness.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201504

[michaeln, 2015-08-27 19:53:28]

michaeln@chromium.org changed reviewers:
+ palmer@chromium.org, sigbjornf@opera.com

[michaeln, 2015-08-27 19:55:04]

ptal, wdyt?

see https://codereview.chromium.org/1294933004/ review log for history and
motivation

[palmer, 2015-08-27 22:59:03]

LGTM once you add a couple more test cases and a comment.

https://codereview.chromium.org/1320793002/diff/20001/Source/platform/weborig...
File Source/platform/weborigin/KURLTest.cpp (right):

https://codereview.chromium.org/1320793002/diff/20001/Source/platform/weborig...
Source/platform/weborigin/KURLTest.cpp:426: kurl = KURL(KURL(), "http://");
Maybe try some weird ones, like http:///noodles/pancakes.php (note 3 leading
/s).

https://codereview.chromium.org/1320793002/diff/20001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (left):

https://codereview.chromium.org/1320793002/diff/20001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:96: if (!url.isValid())
Add a comment here saying that URLs with schemes that require an authority but
which don't have one will have failed this test.

[michaeln, 2015-08-28 23:15:51]

https://codereview.chromium.org/1320793002/diff/20001/Source/platform/weborig...
File Source/platform/weborigin/KURLTest.cpp (right):

https://codereview.chromium.org/1320793002/diff/20001/Source/platform/weborig...
Source/platform/weborigin/KURLTest.cpp:426: kurl = KURL(KURL(), "http://");
On 2015/08/27 22:59:03, palmer wrote:
> Maybe try some weird ones, like http:///noodles/pancakes.php (note 3 leading
> /s).

Done, KURL interprets http:///noodles/pancakes.php as
http://noodles/pancakes.php

https://codereview.chromium.org/1320793002/diff/20001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (left):

https://codereview.chromium.org/1320793002/diff/20001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:96: if (!url.isValid())
On 2015/08/27 22:59:03, palmer wrote:
> Add a comment here saying that URLs with schemes that require an authority but
> which don't have one will have failed this test.

Done, also said it in an ASSERT

[michaeln, 2015-08-28 23:23:15]

michaeln@chromium.org changed reviewers:
+ tsepez@chromium.org

[michaeln, 2015-08-28 23:23:15]

michaeln@chromium.org changed required reviewers:
+ tsepez@chromium.org

[michaeln, 2015-08-28 23:23:16]

tsepez@, ptal for owners review

[sof, 2015-08-30 06:38:46]

(+Cc: mkwst@ )

Thanks for jumping in & handling this; lgtm % assert condition.

shouldTreatAsUniqueOrigin() could additionally be exposed as a static private
method on SecurityOrigin, and unit tested, I suppose. But fine to have it like
this for invalid URL unit testing.

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:102: // will have failed the
isValid() test; i.e. valid HTTP URLs must have a host.
nit: s/i.e./e.g./

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:103:
ASSERT(!((relevantURL.protocolIsInHTTPFamily() || relevantURL.protocolIs("ftp")
&& relevantURL.host().isEmpty())));
Close parens typo? !((A || B) && C) is the condition to test.

[Tom Sepez, 2015-08-31 16:20:28]

OWNERS LGTM once you fix the nits.

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
File Source/platform/weborigin/KURLTest.cpp (right):

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
Source/platform/weborigin/KURLTest.cpp:411: TEST(KURLTest,
ValidHttpAndFtpUrlsHaveHosts)
nit: HTTP, FTP, and URL should be capitialized as they are initials.

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:102: // will have failed the
isValid() test; i.e. valid HTTP URLs must have a host.
nit: 80 cols

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:103:
ASSERT(!((relevantURL.protocolIsInHTTPFamily() || relevantURL.protocolIs("ftp")
&& relevantURL.host().isEmpty())));
nit: 80 cols.

[michaeln, 2015-08-31 20:25:57]

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1320793002/diff/60001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:103:
ASSERT(!((relevantURL.protocolIsInHTTPFamily() || relevantURL.protocolIs("ftp")
&& relevantURL.host().isEmpty())));
On 2015/08/30 06:38:46, sof wrote:
> Close parens typo? !((A || B) && C) is the condition to test.

yikes!!! fixed, thank you

[michaeln, 2015-08-31 20:38:22]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2015-08-31 20:38:22]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
tsepez@chromium.org, sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1320793002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-08-31 20:38:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320793002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320793002/100001

[commit-bot: I haz the power, 2015-08-31 21:08:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-31 21:08:42]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[michaeln, 2015-08-31 21:39:42]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2015-08-31 21:39:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320793002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320793002/100001

[commit-bot: I haz the power, 2015-08-31 22:14:54]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201504