Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860693002:
Network traffic annotation added to indexed_db. (Closed)
|
Created:
3 years, 7 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, jam, jsbell+idb_chromium.org, darin-cc_chromium.org, cmumford, battre Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to indexed_db.
Network traffic annotation is added to network request of:
content/browser/indexed_db/indexed_db_backing_store.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2860693002
Cr-Commit-Position: refs/heads/master@{#470337}
Committed: https://chromium.googlesource.com/chromium/src/+/45f9f0d070c4f8162cd6358c64659161d452694f
Patch Set 1 #
Total comments: 17
Patch Set 2 : Annotation updated. #Patch Set 3 : Cookies disabled. #
Total comments: 4
Patch Set 4 : Comments addressed. #Patch Set 5 : Minor syntax bug. #Messages
Total messages: 26 (11 generated)
rhalavati@chromium.org changed reviewers: + cmumford@chromium.org
We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified one file and added annotation template to it. Please review it and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
jsbell@chromium.org changed reviewers: + jsbell@chromium.org
https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2560: net::NetworkTrafficAnnotationTag traffic_annotation = The request is for a content of a local file ("blob") resource. This will not actually hit the network. The code is copying data already held in Chrome - referenced by a blob URL - into database storage.
On 2017/05/03 15:51:56, jsbell wrote: > https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... > File content/browser/indexed_db/indexed_db_backing_store.cc (right): > > https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... > content/browser/indexed_db/indexed_db_backing_store.cc:2560: > net::NetworkTrafficAnnotationTag traffic_annotation = > The request is for a content of a local file ("blob") resource. This will not > actually hit the network. > > The code is copying data already held in Chrome - referenced by a blob URL - > into database storage. You may want to define a common annotation somewhere that this and other users of "blob:" URLs can reference.
Comment addressed, please review. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2560: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/05/03 15:51:56, jsbell wrote: > The request is for a content of a local file ("blob") resource. This will not > actually hit the network. > > The code is copying data already held in Chrome - referenced by a blob URL - > into database storage. > Thank you for clarification. As a presubmit checker will be added for all network requests that are in codes which are compiled during building Chrome, we need to annotate this as well. But we can choose destination as LOCAL and don't need policy.
How's this? https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2561: net::DefineNetworkTrafficAnnotation("...", R"( persist_blob_to_indexeddb https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2563: sender: "..." Indexed DB https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2564: description: "..." A web page's script has created a Blob (or File) object (either directly via constructors, or by using file upload to a form, or via a fetch()). The script has then made a request to store data including the Blob via the Indexed DB API. As part of committing the database transaction, the content of the Blob is being copied into a file in the database's directory. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2565: trigger: "..." The script has then made a request to store data including a Blob via the Indexed DB API. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2566: data: "..." A Blob or File object referenced by script, either created directly via constructors, or by using file upload to a form, or drag/dop, or via a fetch() or other APIs that produce Blobs. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2567: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL LOCAL
jsbell@chromium.org changed reviewers: + dmurph@chromium.org
+dmurph who has made similar comments on other patches re: blob reading
Thank you Joshua, annotation updated, please review. I also have an inline question. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2561: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/05/04 15:53:16, jsbell wrote: > persist_blob_to_indexeddb Done. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2563: sender: "..." On 2017/05/04 15:53:16, jsbell wrote: > Indexed DB Done. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2564: description: "..." On 2017/05/04 15:53:16, jsbell wrote: > A web page's script has created a Blob (or File) object (either directly via > constructors, or by using file upload to a form, or via a fetch()). The script > has then made a request to store data including the Blob via the Indexed DB API. > As part of committing the database transaction, the content of the Blob is being > copied into a file in the database's directory. Done. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2565: trigger: "..." On 2017/05/04 15:53:16, jsbell wrote: > The script has then made a request to store data including a Blob via the > Indexed DB API. Done. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2566: data: "..." On 2017/05/04 15:53:17, jsbell wrote: > A Blob or File object referenced by script, either created directly via > constructors, or by using file upload to a form, or drag/dop, or via a fetch() > or other APIs that produce Blobs. Done. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2567: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/04 15:53:16, jsbell wrote: > LOCAL Done. https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2570: cookies_allowed: false/true Cookies are enabled by default, it seems that you are not using them, can I add a CL that adds net::LOAD_DO_NOT_SAVE_COOKIES and net::LOAD_DO_NOT_SEND_COOKIES load flags?
lgtm https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2570: cookies_allowed: false/true On 2017/05/05 08:43:22, Ramin Halavati wrote: > Cookies are enabled by default, it seems that you are not using them, can I add > a CL that adds net::LOAD_DO_NOT_SAVE_COOKIES and > net::LOAD_DO_NOT_SEND_COOKIES load flags? Yes please!
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thanks Joshua, Martin, Any comments? https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2860693002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_backing_store.cc:2570: cookies_allowed: false/true On 2017/05/05 18:23:50, jsbell wrote: > On 2017/05/05 08:43:22, Ramin Halavati wrote: > > Cookies are enabled by default, it seems that you are not using them, can I > add > > a CL that adds net::LOAD_DO_NOT_SAVE_COOKIES and > > net::LOAD_DO_NOT_SEND_COOKIES load flags? > > Yes please! Done, in CL https://codereview.chromium.org/2864243002
LGTM % nits https://codereview.chromium.org/2860693002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2860693002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_backing_store.cc:2578: "drag/dop, or via a fetch() or other APIs that produce Blobs." typo: drop https://codereview.chromium.org/2860693002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_backing_store.cc:2583: setting: "This feature cannot be stoped by settings." s/stoped/disabled/ ? (also, "stoped" seems to only be defined by Urban Dictionary :) )
Thank you, comments addressed, landing. https://codereview.chromium.org/2860693002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2860693002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_backing_store.cc:2578: "drag/dop, or via a fetch() or other APIs that produce Blobs." On 2017/05/09 11:54:09, msramek (recovering) wrote: > typo: drop Done. https://codereview.chromium.org/2860693002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_backing_store.cc:2583: setting: "This feature cannot be stoped by settings." On 2017/05/09 11:54:09, msramek (recovering) wrote: > s/stoped/disabled/ ? > > (also, "stoped" seems to only be defined by Urban Dictionary :) ) Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2860693002/#ps60001 (title: "Comments addressed.")
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
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/2860693002/#ps80001 (title: "Minor syntax bug.")
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": 1494336000747470,
"parent_rev": "cf0772c92c9909b3ad0f107541a533c567bfda75", "commit_rev":
"45f9f0d070c4f8162cd6358c64659161d452694f"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to indexed_db. Network traffic annotation is added to network request of: content/browser/indexed_db/indexed_db_backing_store.cc BUG=656607 ========== to ========== Network traffic annotation added to indexed_db. Network traffic annotation is added to network request of: content/browser/indexed_db/indexed_db_backing_store.cc BUG=656607 Review-Url: https://codereview.chromium.org/2860693002 Cr-Commit-Position: refs/heads/master@{#470337} Committed: https://chromium.googlesource.com/chromium/src/+/45f9f0d070c4f8162cd6358c6465... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/45f9f0d070c4f8162cd6358c6465... |
