Chromium Code Reviews
Help | Chromium Project | Sign in
(28)

Issue 2562813003: Merge logic for SameDomainOrHost for GURLs and Origins (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 3 weeks ago by Charlie Harrison
Modified:
3 months, 1 week ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Merge logic for SameDomainOrHost for GURLs and Origins Previously, we used to just call: SameDomainOrHost(origin1.GetURL(), origin2.GetURL()) This isn't necessary, as we have all the information in the Origin itself. Additionally, GetURL() isn't free. We do a bunch of string copies, allocations, and some non-cheap policy decisions. This patch moves GURL::HostIsIPAddress logic to url_util, and reuses that code for both GURLs and Origins. BUG=664174, 348655 Committed: https://crrev.com/475851dad085b226d5459363e4daf152eb4cfc77 Cr-Commit-Position: refs/heads/master@{#439287}

Patch Set 1 #

Total comments: 2

Patch Set 2 : pkasting review (length() -> empty()) #

Total comments: 2

Patch Set 3 : brettw review #

Messages

Total messages: 19 (10 generated)
Charlie Harrison
Brett, Peter: PTAL at this patch? In my opinion it makes things conceptually simpler, as ...
3 months, 3 weeks ago (2016-12-08 22:43:19 UTC) #4
Peter Kasting
LGTM https://codereview.chromium.org/2562813003/diff/1/net/base/registry_controlled_domains/registry_controlled_domain.cc File net/base/registry_controlled_domains/registry_controlled_domain.cc (right): https://codereview.chromium.org/2562813003/diff/1/net/base/registry_controlled_domains/registry_controlled_domain.cc#newcode316 net/base/registry_controlled_domains/registry_controlled_domain.cc:316: if (!host1.length() || !host2.length()) Nit: Prefer .empty() to ...
3 months, 3 weeks ago (2016-12-09 00:17:59 UTC) #7
Charlie Harrison
Thanks! https://codereview.chromium.org/2562813003/diff/1/net/base/registry_controlled_domains/registry_controlled_domain.cc File net/base/registry_controlled_domains/registry_controlled_domain.cc (right): https://codereview.chromium.org/2562813003/diff/1/net/base/registry_controlled_domains/registry_controlled_domain.cc#newcode316 net/base/registry_controlled_domains/registry_controlled_domain.cc:316: if (!host1.length() || !host2.length()) On 2016/12/09 00:17:59, Peter ...
3 months, 3 weeks ago (2016-12-09 01:14:01 UTC) #8
Charlie Harrison
brettw: friendly ping
3 months, 2 weeks ago (2016-12-14 23:22:08 UTC) #9
brettw (plz ping after 24h)
lgtm https://codereview.chromium.org/2562813003/diff/20001/url/url_util.h File url/url_util.h (right): https://codereview.chromium.org/2562813003/diff/20001/url/url_util.h#newcode149 url/url_util.h:149: // Hosts ----------------------------------------------------------------------- I'm not sure these section ...
3 months, 2 weeks ago (2016-12-15 18:45:32 UTC) #10
Charlie Harrison
Thanks! https://codereview.chromium.org/2562813003/diff/20001/url/url_util.h File url/url_util.h (right): https://codereview.chromium.org/2562813003/diff/20001/url/url_util.h#newcode149 url/url_util.h:149: // Hosts ----------------------------------------------------------------------- On 2016/12/15 18:45:32, brettw (ping ...
3 months, 1 week ago (2016-12-16 22:48:32 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2562813003/40001
3 months, 1 week ago (2016-12-16 22:49:20 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001)
3 months, 1 week ago (2016-12-17 02:20:52 UTC) #17
commit-bot: I haz the power
3 months, 1 week ago (2016-12-17 02:26:22 UTC) #19
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/475851dad085b226d5459363e4daf152eb4cfc77
Cr-Commit-Position: refs/heads/master@{#439287}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46