Implement correct logic for URLPatternSet set operators.

Implement correct logic for URLPatternSet set operators.

URLPattern instances have "containment" relationships (as opposed to simple equality) that need to be taken into account when implementing set operations.
For example, {"<all_urls>"} INTERSECTION {"http://www.google.com/*"} = {"http://www.google.com/*"}

BUG=310815

[rpaquay, 2014-02-27 00:20:10]

I think I got all the special cases right. LMK.

[not at google - send to devlin, 2014-02-27 21:10:46]

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:34: bool keepPattern1 = true;
is this whole block just

if (!set.ContainsPattern(pattern1))
  out->InsertPattern(*pattern1)

? (in which case "pattern1" could be just "pattern" or "it")

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:37: if (*pattern2 == *pattern1 ||
pattern2->Contains(*pattern1)) {
Could you add the equality shortcut (assuming its useful) to
URLPattern::Contains and then just have pattern2->Contains(pattern1) here?

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:57: out->InsertPattern(*pattern1);
likewise above comment; this *pattern1 == *pattern2 check shouldn't be needed
and would be caught in the next condition.

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:103: }
see above comment(s) that apply to this function too. It also looks like a
candidate for calling a single function twice?

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:172: }
if (ContainsPattern(pattern))
  return false;

?

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:179: ++it;
nice

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
File extensions/common/url_pattern_set_unittest.cc (right):

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set_unittest.cc:405:
AddMatchAllURLsPattern(&all_urls);
presumably these sets are copyable. so you could have
URLPatternSet all_urls = GetAllURLsPattern();
then change AddMatchAllURLsPattern.

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set_unittest.cc:409: 
there are some other cases here that it might be good to test, like overlapping
sets, having "*://www.google.com" and "http://*.google.com" in different/same
sets, etc. you might consider breaking up into separate test cases if that makes
it any easier, not sure about that.

[rpaquay, 2014-02-28 01:46:47]

ptal

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:34: bool keepPattern1 = true;
On 2014/02/27 21:10:46, kalman wrote:
> is this whole block just
> 
> if (!set.ContainsPattern(pattern1))
>   out->InsertPattern(*pattern1)
> 
> ? (in which case "pattern1" could be just "pattern" or "it")

Done.

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:37: if (*pattern2 == *pattern1 ||
pattern2->Contains(*pattern1)) {
On 2014/02/27 21:10:46, kalman wrote:
> Could you add the equality shortcut (assuming its useful) to
> URLPattern::Contains and then just have pattern2->Contains(pattern1) here?

Done.

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:57: out->InsertPattern(*pattern1);
On 2014/02/27 21:10:46, kalman wrote:
> likewise above comment; this *pattern1 == *pattern2 check shouldn't be needed
> and would be caught in the next condition.

Done.

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:103: }
On 2014/02/27 21:10:46, kalman wrote:
> see above comment(s) that apply to this function too. It also looks like a
> candidate for calling a single function twice?
Actually, since I updated AddPattern to do the "right thing", I could simplify
the code even further.

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:172: }
On 2014/02/27 21:10:46, kalman wrote:
> if (ContainsPattern(pattern))
>   return false;
> 
> ?

Done.

https://codereview.chromium.org/181043006/diff/1/extensions/common/url_patter...
extensions/common/url_pattern_set.cc:179: ++it;
On 2014/02/27 21:10:46, kalman wrote:
> nice

http://stackoverflow.com/questions/2874441/deleting-elements-from-stl-set-whi...

:)

[not at google - send to devlin, 2014-02-28 17:40:19]

lgtm

[rpaquay, 2014-02-28 18:05:33]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-02-28 18:06:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/181043006/20001

[commit-bot: I haz the power, 2014-02-28 19:15:46]

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

[commit-bot: I haz the power, 2014-02-28 19:15:46]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[not at google - send to devlin, 2014-08-08 14:29:56]

why did this CL never go in? all of a sudden I need it :)