Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2723563002:
Reporting: Implement uploader. (Closed)
|
Created:
3 years, 9 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: Implement uploader.
Reporting is a spec for delivering out-of-band reports from various
other parts of the browser. See http://wicg.github.io/reporting/.
The uploader handles the network side of report delivery: it takes the
assembled JSON and an endpoint URL, and handles sending the URL request
and categorizing the response according to the Reporting spec.
BUG=704259
Review-Url: https://codereview.chromium.org/2723563002
Cr-Commit-Position: refs/heads/master@{#460846}
Committed: https://chromium.googlesource.com/chromium/src/+/2f46b5780970e186eda8641be9138ec551e3f1db
Patch Set 1 #Patch Set 2 : Tidy things up a bit. #Patch Set 3 : rebase #Patch Set 4 : rebase #
Total comments: 10
Patch Set 5 : Make requested changes. #
Depends on Patchset: Messages
Total messages: 33 (21 generated)
Description was changed from ========== Reporting: Add Uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The Uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG= ========== to ========== Reporting: Create Uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The Uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG= ==========
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: This issue passed the CQ dry run.
Description was changed from ========== Reporting: Create Uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The Uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG= ========== to ========== Reporting: Create uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The Uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG= ==========
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.
Description was changed from ========== Reporting: Create uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The Uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG= ========== to ========== Reporting: Implement uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG= ==========
Description was changed from ========== Reporting: Implement uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG= ========== to ========== Reporting: Implement uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG=704259 ==========
https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... File net/reporting/reporting_uploader.cc (right): https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.cc:29: class ReportingUploaderImpl : public ReportingUploader, URLRequest::Delegate { Why is this class definition inside the anonymous namespace inside net? Can this be put directly in net namespace? https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.cc:67: // This inherently skips Service Worker, too. This question is for my own understanding: what is the significance of the servive worker related comment here? https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.cc:76: // URLRequest::Delegate implementation: new line after this comment as it applies to all the below functions. https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.cc:94: } Implementation of URLRequest::Delegate::OnSSLCertificateError? https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... File net/reporting/reporting_uploader.h (right): https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.h:21: enum Outcome { SUCCESS, REMOVE_ENDPOINT, FAILURE }; prefer enum class over enum for type safety.
PTAL, shivanisha. https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... File net/reporting/reporting_uploader.cc (right): https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.cc:29: class ReportingUploaderImpl : public ReportingUploader, URLRequest::Delegate { On 2017/03/28 19:57:14, shivanisha wrote: > Why is this class definition inside the anonymous namespace inside net? Can this > be put directly in net namespace? It could, but I consider it an implementation detail, so I wanted to keep it in the anonymous namespace. https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.cc:67: // This inherently skips Service Worker, too. On 2017/03/28 19:57:14, shivanisha wrote: > This question is for my own understanding: what is the significance of the > servive worker related comment here? The Reporting spec requires report delivery requests to skip Service Worker. https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.cc:76: // URLRequest::Delegate implementation: On 2017/03/28 19:57:14, shivanisha wrote: > new line after this comment as it applies to all the below functions. Done. https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.cc:94: } On 2017/03/28 19:57:14, shivanisha wrote: > Implementation of URLRequest::Delegate::OnSSLCertificateError? Done. https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... File net/reporting/reporting_uploader.h (right): https://codereview.chromium.org/2723563002/diff/60001/net/reporting/reporting... net/reporting/reporting_uploader.h:21: enum Outcome { SUCCESS, REMOVE_ENDPOINT, FAILURE }; On 2017/03/28 19:57:14, shivanisha wrote: > prefer enum class over enum for type safety. Done.
Ping?
On 2017/03/30 at 13:22:06, juliatuttle wrote: > Ping? On it.
On 2017/03/30 at 16:49:02, shivanisha wrote: > On 2017/03/30 at 13:22:06, juliatuttle wrote: > > Ping? > > On it. LGTM.
The CQ bit was checked by juliatuttle@chromium.org
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...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
juliatuttle@chromium.org changed reviewers: + rdsmith@chromium.org
PTAL, rdsmith for committer review.
LGTM based on Shivani's review.
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": 1490896565166490,
"parent_rev": "87c961a98632917bf273c28e7814495dd1a7cc66", "commit_rev":
"2f46b5780970e186eda8641be9138ec551e3f1db"}
Message was sent while issue was closed.
Description was changed from ========== Reporting: Implement uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG=704259 ========== to ========== Reporting: Implement uploader. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. BUG=704259 Review-Url: https://codereview.chromium.org/2723563002 Cr-Commit-Position: refs/heads/master@{#460846} Committed: https://chromium.googlesource.com/chromium/src/+/2f46b5780970e186eda8641be913... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2f46b5780970e186eda8641be913... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
