Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779983002:
Reporting: Properly support endpoints with includeSubdomains. (Closed)
|
Created:
3 years, 8 months ago by Julia Tuttle Modified:
3 years, 8 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReporting: Properly support endpoints with includeSubdomains.
Reporting is a spec for delivering out-of-band reports from various
other parts of the browser. See http://wicg.github.io/reporting/ for
the spec, or https://goo.gl/pygX5I for details of the planned
implementation in Chromium.
This modifies the ReportingCache to look for superdomain matches when
asked for clients for an origin but no exact origin match is found.
BUG=704259
Review-Url: https://codereview.chromium.org/2779983002
Cr-Commit-Position: refs/heads/master@{#464097}
Committed: https://chromium.googlesource.com/chromium/src/+/e9b931b5db3e382b208607439ebc22ab0a7e5a68
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : rebase #
Total comments: 18
Patch Set 5 : Make requested changes. #
Messages
Total messages: 25 (17 generated)
juliatuttle@chromium.org changed reviewers: + shivanisha@chromium.org
PTAL, shivanisha.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:25: std::string GetSuperdomain(const std::string& domain) { A brief comment on this function with an example of input and output domains. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:105: else { For readability, if else has {} then include in the if block as well, and vice versa. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:108: DCHECK_EQ(1u, erased); Why removing the dcheck above? It is still applicable to pending_reports_ as well, right? https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:153: } Should there be a return in the case when clients_out is populated above? As per the comment below "If no clients were found..." , it seems like. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:158: std::string domain = origin.host(); I am not sure what origin.host returns. Does it return the complete host name without the port number, if any? https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:172: if (base::ContainsKey(clients_, origin) && Comment explaining why we are using the Wildcard related functions while setting the client. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:253: client->subdomains == ReportingClient::Subdomains::INCLUDE) { Should this be a dcheck instead of a condition since only clients with ReportingClient::Subdomains::INCLUDE should be included in wildcard_clients_? https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... File net/reporting/reporting_cache.h (right): https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.h:112: // For example, given the origin https://foo.bar.example.com/, the cache would This should be https://foo.bar.baz.com/ based on the domains below. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.h:182: wildcard_clients_; Does std::string has a hash to be able to make this an unordered_map?
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
PTAL, shivanisha. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:25: std::string GetSuperdomain(const std::string& domain) { On 2017/04/10 20:08:41, shivanisha wrote: > A brief comment on this function with an example of input and output domains. Done. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:105: else { On 2017/04/10 20:08:41, shivanisha wrote: > For readability, if else has {} then include in the if block as well, and vice > versa. Done. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:108: DCHECK_EQ(1u, erased); On 2017/04/10 20:08:41, shivanisha wrote: > Why removing the dcheck above? It is still applicable to pending_reports_ as > well, right? If a caller passes in a random Report pointer that's not in the cache, it won't be in pending_reports_, so it will execute the else block, but erased will be 0 because it's not in reports_ either, and then it'll trip the DCHECK_EQ here. I'm not concerned about internal errors in the cache that could leave reports in pending_reports_ but not reports_. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:153: } On 2017/04/10 20:08:41, shivanisha wrote: > Should there be a return in the case when clients_out is populated above? As per > the comment below "If no clients were found..." , it seems like. Nope, it'll get to the while loop and clients_out->empty() will be false, so it won't do anything else. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:158: std::string domain = origin.host(); On 2017/04/10 20:08:41, shivanisha wrote: > I am not sure what origin.host returns. Does it return the complete host name > without the port number, if any? Yes, it's just the hostname, no port number. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:172: if (base::ContainsKey(clients_, origin) && On 2017/04/10 20:08:41, shivanisha wrote: > Comment explaining why we are using the Wildcard related functions while setting > the client. Done. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.cc:253: client->subdomains == ReportingClient::Subdomains::INCLUDE) { On 2017/04/10 20:08:41, shivanisha wrote: > Should this be a dcheck instead of a condition since only clients with > ReportingClient::Subdomains::INCLUDE should be included in wildcard_clients_? Done. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... File net/reporting/reporting_cache.h (right): https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.h:112: // For example, given the origin https://foo.bar.example.com/, the cache would On 2017/04/10 20:08:41, shivanisha wrote: > This should be https://foo.bar.baz.com/ based on the domains below. Done. https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... net/reporting/reporting_cache.h:182: wildcard_clients_; On 2017/04/10 20:08:42, shivanisha wrote: > Does std::string has a hash to be able to make this an unordered_map? Oh, hey, it does. Good call.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/12 at 16:11:37, juliatuttle wrote: > PTAL, shivanisha. > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > File net/reporting/reporting_cache.cc (right): > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > net/reporting/reporting_cache.cc:25: std::string GetSuperdomain(const std::string& domain) { > On 2017/04/10 20:08:41, shivanisha wrote: > > A brief comment on this function with an example of input and output domains. > > Done. > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > net/reporting/reporting_cache.cc:105: else { > On 2017/04/10 20:08:41, shivanisha wrote: > > For readability, if else has {} then include in the if block as well, and vice > > versa. > > Done. > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > net/reporting/reporting_cache.cc:108: DCHECK_EQ(1u, erased); > On 2017/04/10 20:08:41, shivanisha wrote: > > Why removing the dcheck above? It is still applicable to pending_reports_ as > > well, right? > > If a caller passes in a random Report pointer that's not in the cache, it won't be in pending_reports_, so it will execute the else block, but erased will be 0 because it's not in reports_ either, and then it'll trip the DCHECK_EQ here. > > I'm not concerned about internal errors in the cache that could leave reports in pending_reports_ but not reports_. > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > net/reporting/reporting_cache.cc:153: } > On 2017/04/10 20:08:41, shivanisha wrote: > > Should there be a return in the case when clients_out is populated above? As per > > the comment below "If no clients were found..." , it seems like. > > Nope, it'll get to the while loop and clients_out->empty() will be false, so it won't do anything else. > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > net/reporting/reporting_cache.cc:158: std::string domain = origin.host(); > On 2017/04/10 20:08:41, shivanisha wrote: > > I am not sure what origin.host returns. Does it return the complete host name > > without the port number, if any? > > Yes, it's just the hostname, no port number. > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > net/reporting/reporting_cache.cc:172: if (base::ContainsKey(clients_, origin) && > On 2017/04/10 20:08:41, shivanisha wrote: > > Comment explaining why we are using the Wildcard related functions while setting > > the client. > > Done. > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > net/reporting/reporting_cache.cc:253: client->subdomains == ReportingClient::Subdomains::INCLUDE) { > On 2017/04/10 20:08:41, shivanisha wrote: > > Should this be a dcheck instead of a condition since only clients with > > ReportingClient::Subdomains::INCLUDE should be included in wildcard_clients_? > > Done. > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > File net/reporting/reporting_cache.h (right): > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > net/reporting/reporting_cache.h:112: // For example, given the origin https://foo.bar.example.com/, the cache would > On 2017/04/10 20:08:41, shivanisha wrote: > > This should be https://foo.bar.baz.com/ based on the domains below. > > Done. > > https://codereview.chromium.org/2779983002/diff/60001/net/reporting/reporting... > net/reporting/reporting_cache.h:182: wildcard_clients_; > On 2017/04/10 20:08:42, shivanisha wrote: > > Does std::string has a hash to be able to make this an unordered_map? > > Oh, hey, it does. Good call. lgtm
juliatuttle@chromium.org changed reviewers: + csharrison@chromium.org
Thanks, shivanisha! PTAL csharrison for committer signoff.
RS LGTM
The CQ bit was unchecked by juliatuttle@chromium.org
The CQ bit was checked by juliatuttle@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492020085087820,
"parent_rev": "bc24ac56413d35a83928c20bb3ccb087086b4b44", "commit_rev":
"e9b931b5db3e382b208607439ebc22ab0a7e5a68"}
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492020085087820,
"parent_rev": "bc24ac56413d35a83928c20bb3ccb087086b4b44", "commit_rev":
"e9b931b5db3e382b208607439ebc22ab0a7e5a68"}
Message was sent while issue was closed.
Description was changed from ========== Reporting: Properly support endpoints with includeSubdomains. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. This modifies the ReportingCache to look for superdomain matches when asked for clients for an origin but no exact origin match is found. BUG=704259 ========== to ========== Reporting: Properly support endpoints with includeSubdomains. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. This modifies the ReportingCache to look for superdomain matches when asked for clients for an origin but no exact origin match is found. BUG=704259 Review-Url: https://codereview.chromium.org/2779983002 Cr-Commit-Position: refs/heads/master@{#464097} Committed: https://chromium.googlesource.com/chromium/src/+/e9b931b5db3e382b208607439ebc... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/e9b931b5db3e382b208607439ebc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
