Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770443002:
Reporting: Wrap context in service. (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: Wrap context in service.
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 wraps the context in a service that keeps the implementation
details of Reporting private while implementing and exposing the
high-level operations that other components require from it.
BUG=704259
Review-Url: https://codereview.chromium.org/2770443002
Cr-Commit-Position: refs/heads/master@{#462943}
Committed: https://chromium.googlesource.com/chromium/src/+/381d77efd050a4ef04eba2ce52f06741ac0d1107
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 6
Patch Set 3 : Make requested changes. #
Total comments: 4
Patch Set 4 : Make requested changes. #Patch Set 5 : rebase #
Depends on Patchset: Messages
Total messages: 30 (17 generated)
Description was changed from ========== Reporting: Wrap context in service This keeps the implementation details of Reporting private while implementing and exposing the high-level operations that other components need from it. BUG= ========== to ========== Reporting: Wrap context in service 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 wraps the context in a service that keeps the implementation details of Reporting private while implementing and exposing the high-level operations that other components require from it. BUG= ==========
juliatuttle@chromium.org changed reviewers: + shivanisha@chromium.org
Description was changed from ========== Reporting: Wrap context in service 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 wraps the context in a service that keeps the implementation details of Reporting private while implementing and exposing the high-level operations that other components require from it. BUG= ========== to ========== Reporting: Wrap context in service 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 wraps the context in a service that keeps the implementation details of Reporting private while implementing and exposing the high-level operations that other components require from it. BUG=704259 ==========
PTAL, shivanisha.
Description was changed from ========== Reporting: Wrap context in service 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 wraps the context in a service that keeps the implementation details of Reporting private while implementing and exposing the high-level operations that other components require from it. BUG=704259 ========== to ========== Reporting: Wrap context in service. 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 wraps the context in a service that keeps the implementation details of Reporting private while implementing and exposing the high-level operations that other components require from it. BUG=704259 ==========
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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2770443002/diff/20001/net/reporting/reporting... File net/reporting/reporting_service.h (right): https://codereview.chromium.org/2770443002/diff/20001/net/reporting/reporting... net/reporting/reporting_service.h:28: class NET_EXPORT ReportingService { Comment describing the overall purpose of the class. https://codereview.chromium.org/2770443002/diff/20001/net/reporting/reporting... net/reporting/reporting_service.h:35: std::unique_ptr<ReportingDelegate> delegate); Comment describing the arguments. https://codereview.chromium.org/2770443002/diff/20001/net/reporting/reporting... net/reporting/reporting_service.h:51: base::Callback<bool(const GURL&)> origin_filter) = 0; Comments for all of the above APIs.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
PTAL, shivanisha. https://codereview.chromium.org/2770443002/diff/20001/net/reporting/reporting... File net/reporting/reporting_service.h (right): https://codereview.chromium.org/2770443002/diff/20001/net/reporting/reporting... net/reporting/reporting_service.h:28: class NET_EXPORT ReportingService { On 2017/04/04 17:52:43, shivanisha wrote: > Comment describing the overall purpose of the class. Done. https://codereview.chromium.org/2770443002/diff/20001/net/reporting/reporting... net/reporting/reporting_service.h:35: std::unique_ptr<ReportingDelegate> delegate); On 2017/04/04 17:52:43, shivanisha wrote: > Comment describing the arguments. Done. https://codereview.chromium.org/2770443002/diff/20001/net/reporting/reporting... net/reporting/reporting_service.h:51: base::Callback<bool(const GURL&)> origin_filter) = 0; On 2017/04/04 17:52:43, shivanisha wrote: > Comments for all of the above APIs. Done.
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/2770443002/diff/40001/net/reporting/reporting... File net/reporting/reporting_service.cc (right): https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... net/reporting/reporting_service.cc:49: NOTIMPLEMENTED(); Is it being implemented in a future reporting CL (which I probably haven't looked at)? If not, then may be add a todo here mentioning its implementation is dependent on implementing <X>?
PTAL, shivanisha. https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... File net/reporting/reporting_service.cc (right): https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... net/reporting/reporting_service.cc:49: NOTIMPLEMENTED(); On 2017/04/05 16:59:30, shivanisha wrote: > Is it being implemented in a future reporting CL (which I probably haven't > looked at)? > If not, then may be add a todo here mentioning its implementation is dependent > on implementing <X>? Yes, it is being implemented in https://codereview.chromium.org/2752523005/. If you'd like, I can just remove the method until then.
https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... File net/reporting/reporting_service.cc (right): https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... net/reporting/reporting_service.cc:49: NOTIMPLEMENTED(); On 2017/04/06 at 17:09:49, Julia Tuttle wrote: > On 2017/04/05 16:59:30, shivanisha wrote: > > Is it being implemented in a future reporting CL (which I probably haven't > > looked at)? > > If not, then may be add a todo here mentioning its implementation is dependent > > on implementing <X>? > > Yes, it is being implemented in https://codereview.chromium.org/2752523005/. > > If you'd like, I can just remove the method until then. Having it in the same CL where it is being implemented might be better for clarity. Thanks.
PTAL, shivanisha. https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... File net/reporting/reporting_service.cc (right): https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... net/reporting/reporting_service.cc:49: NOTIMPLEMENTED(); On 2017/04/06 17:13:01, shivanisha wrote: > On 2017/04/06 at 17:09:49, Julia Tuttle wrote: > > On 2017/04/05 16:59:30, shivanisha wrote: > > > Is it being implemented in a future reporting CL (which I probably haven't > > > looked at)? > > > If not, then may be add a todo here mentioning its implementation is > dependent > > > on implementing <X>? > > > > Yes, it is being implemented in https://codereview.chromium.org/2752523005/. > > > > If you'd like, I can just remove the method until then. > > Having it in the same CL where it is being implemented might be better for > clarity. Thanks. Done.
On 2017/04/06 at 17:51:05, juliatuttle wrote: > PTAL, shivanisha. > > https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... > File net/reporting/reporting_service.cc (right): > > https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... > net/reporting/reporting_service.cc:49: NOTIMPLEMENTED(); > On 2017/04/06 17:13:01, shivanisha wrote: > > On 2017/04/06 at 17:09:49, Julia Tuttle wrote: > > > On 2017/04/05 16:59:30, shivanisha wrote: > > > > Is it being implemented in a future reporting CL (which I probably haven't > > > > looked at)? > > > > If not, then may be add a todo here mentioning its implementation is > > dependent > > > > on implementing <X>? > > > > > > Yes, it is being implemented in https://codereview.chromium.org/2752523005/. > > > > > > If you'd like, I can just remove the method until then. > > > > Having it in the same CL where it is being implemented might be better for > > clarity. Thanks. > > Done. lgtm
On 2017/04/06 17:59:40, shivanisha wrote: > On 2017/04/06 at 17:51:05, juliatuttle wrote: > > PTAL, shivanisha. > > > > > https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... > > File net/reporting/reporting_service.cc (right): > > > > > https://codereview.chromium.org/2770443002/diff/40001/net/reporting/reporting... > > net/reporting/reporting_service.cc:49: NOTIMPLEMENTED(); > > On 2017/04/06 17:13:01, shivanisha wrote: > > > On 2017/04/06 at 17:09:49, Julia Tuttle wrote: > > > > On 2017/04/05 16:59:30, shivanisha wrote: > > > > > Is it being implemented in a future reporting CL (which I probably > haven't > > > > > looked at)? > > > > > If not, then may be add a todo here mentioning its implementation is > > > dependent > > > > > on implementing <X>? > > > > > > > > Yes, it is being implemented in > https://codereview.chromium.org/2752523005/. > > > > > > > > If you'd like, I can just remove the method until then. > > > > > > Having it in the same CL where it is being implemented might be better for > > > clarity. Thanks. > > > > Done. > > lgtm Thanks!
juliatuttle@chromium.org changed reviewers: + csharrison@chromium.org
PTAL for committer signoff, csharrison.
RS LGTM
The CQ bit was checked by juliatuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shivanisha@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2770443002/#ps80001 (title: "rebase")
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": 1491586952326760,
"parent_rev": "68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b", "commit_rev":
"381d77efd050a4ef04eba2ce52f06741ac0d1107"}
Message was sent while issue was closed.
Description was changed from ========== Reporting: Wrap context in service. 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 wraps the context in a service that keeps the implementation details of Reporting private while implementing and exposing the high-level operations that other components require from it. BUG=704259 ========== to ========== Reporting: Wrap context in service. 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 wraps the context in a service that keeps the implementation details of Reporting private while implementing and exposing the high-level operations that other components require from it. BUG=704259 Review-Url: https://codereview.chromium.org/2770443002 Cr-Commit-Position: refs/heads/master@{#462943} Committed: https://chromium.googlesource.com/chromium/src/+/381d77efd050a4ef04eba2ce52f0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/381d77efd050a4ef04eba2ce52f0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
