Refactoring DownloadManager::DownloadURL to add proper annotation.

Refactoring DownloadManager::DownloadURL to add proper annotation.

Refactoring DownloadManager::DownloadURL to add proper network traffic
annotation. All changed files have received a NO_TRAFFIC_ANNOTATION_YET
tag instead of proper annotation and will be annotated in next CLs.

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2860593003
Cr-Commit-Position: refs/heads/master@{#476312}
Committed: https://chromium.googlesource.com/chromium/src/+/0fbaeef38e204c0a97c1792689a2d0fd4fb784ea

[Ramin Halavati, 2017-05-03 07:49:29]

rhalavati@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Ramin Halavati, 2017-05-03 07:49:29]

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 2 files and added annotation template to them. Please review them
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.

[Ramin Halavati, 2017-05-10 07:09:45]

On 2017/05/03 07:49:29, Ramin Halavati wrote:
> 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 2 files and added annotation template to them. Please review
them
> 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.

Hi David,

A gentle reminder on this.

[David Trainor- moved to gerrit, 2017-05-10 20:09:12]

On 2017/05/10 07:09:45, Ramin Halavati wrote:
> On 2017/05/03 07:49:29, Ramin Halavati wrote:
> > 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 2 files and added annotation template to them. Please review
> them
> > 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.
> 
> Hi David,
> 
> A gentle reminder on this.

Hey sorry!  Thanks for reminding me :).  Will work on figuring out the correct
values today.

[David Trainor- moved to gerrit, 2017-05-11 20:27:52]

might need to add some plumbing to do this right.  some internal services will
use this to hit APIs, but in general websites will use this for general web
access.

To do it right I'll have to persist things to the database as we recreate the
requests on restart if they weren't finished yet.  Will investigate further
today.  Sorry for the delays!

[Ramin Halavati, 2017-05-18 05:23:06]

On 2017/05/11 20:27:52, David Trainor-ping if over 24h wrote:
> might need to add some plumbing to do this right.  some internal services will
> use this to hit APIs, but in general websites will use this for general web
> access.
> 
> To do it right I'll have to persist things to the database as we recreate the
> requests on restart if they weren't finished yet.  Will investigate further
> today.  Sorry for the delays!

Hi David,

Can I help on this?
Would you prefer if the annotations would be given to this class as an argument?

[David Trainor- moved to gerrit, 2017-05-23 01:07:40]

Yeah we probably have to push these out to the callers.  That's a good idea.  I
went through the call sites for the first file and it blew up to a bunch of
possible calls.  I'd just DCHECK that we have a valid
net::NetworkTrafficAnnotationTag in DownloadUrlParameters when we're going to
create the request so all future callers must provide one.

Fleshed out some examples below for the trigger.  Will try to clean up some of
the text later tonight/tomorrow.  Haven't looked at SaveFileManager yet but that
looks like there are a bunch of paths to worry about as well.

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
File content/browser/download/download_request_core.cc (right):

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
content/browser/download/download_request_core.cc:121:
net::DefineNetworkTrafficAnnotation("...", R"(
download_manager_url_request

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
content/browser/download/download_request_core.cc:123: sender: "..."
download_manager

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
content/browser/download/download_request_core.cc:124: description: "..."
A download request that can come from a variety of sources (see trigger) and
will download a file and save it to disk.

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
content/browser/download/download_request_core.cc:125: trigger: "..."
This needs to be filled in based on who is building the DownloadUrlParameters. 
I'd suggest (from the call sites I can find).  Let me know if you think there
are better naming suggestions!

downloads_api.cc: "extension download API"
webstore_installer.cc: "extension install"
render_view_context_menu.cc: "save link as"
background_fetch_job_controller.cc: "background fetch API"
drag_download_file.cc: "drag file to save"
render_frame_message_filter.cc: "user navigation triggered download"
indexed_db_internals_ui.cc: "saving internal indexed db data"
web_contents_impl.cc: "user saved page"
download_worker.cc: "parallel download request"
download_manager_impl.cc: "download resumption"

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
content/browser/download/download_request_core.cc:126: data: "..."
Could you clarify, do we want to describe the data we're sending or receiving?

If it's sending, this probably also has a lot to do with the request.  Many of
these we aren't sure because they're triggered by an extension or a site.

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
content/browser/download/download_request_core.cc:127: destination:
WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL
WEBSITE.  Although in the future we'll need GOOGLE_OWNED_SERVICE too.  We can
defer that for now and make sure it gets added once DownloadService gets built
out.  This is the field that we probably need to persist beyond restarts.

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
content/browser/download/download_request_core.cc:130: cookies_allowed:
false/true
true.  It depends on the caller and which UrlRequestContext gets built.  We
might need to pass this in as well if we want specifics.  Let me know if we need
to be more detailed here.  Will look into the call sites listed above for
"trigger" for this as well.

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
content/browser/download/download_request_core.cc:131: cookies_store: "..."
Same as above.  Probably depends on the call site.

https://codereview.chromium.org/2860593003/diff/1/content/browser/download/do...
content/browser/download/download_request_core.cc:132: setting: "..."
Also depends on the call site.  Sadly it looks like a bunch of the above might
:(.  It might be best to require the DownloadManager::DownloadUrl (and download
internal call sites) to build a few pieces of this.  Or maybe a helper function
where they pass in all the optional parameters and we build the annotation for
them.

[Ramin Halavati, 2017-05-26 09:28:21]

Description was changed from

==========
Network traffic annotation added to content/browser/download.

Network traffic annotation is added to network request of:
content/browser/download/download_request_core.cc
content/browser/download/save_file_manager.cc

BUG=656607
==========

to

==========
Network traffic annotation added to content/browser/download.

Network traffic annotation is added to network request of:
content/browser/download/download_request_core.cc
content/browser/download/save_file_manager.cc

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ramin Halavati, 2017-05-26 09:33:40]

The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 09:33:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 09:37:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 09:37:43]

Dry run: Try jobs failed on following builders:
  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...)

[Ramin Halavati, 2017-05-26 09:44:33]

Patchset #2 (id:20001) has been deleted

[Ramin Halavati, 2017-05-26 09:44:46]

The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 09:45:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 10:12:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 10:12:01]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ramin Halavati, 2017-05-29 07:11:16]

Patchset #3 (id:60001) has been deleted

[Ramin Halavati, 2017-05-29 07:11:25]

Patchset #2 (id:40001) has been deleted

[Ramin Halavati, 2017-05-29 07:11:33]

Patchset #2 (id:80001) has been deleted

[Ramin Halavati, 2017-05-29 07:14:23]

Patchset #2 (id:100001) has been deleted

[Ramin Halavati, 2017-05-29 07:14:32]

The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 07:14:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 08:34:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-29 08:34:17]

Dry run: This issue passed the CQ dry run.

[Ramin Halavati, 2017-05-29 08:36:49]

Hi David,

I moved the annotation to DownloadManager::DownloadURL, also removed
save_file_manager.cc from this CL to separate annotating and refactoring.

Please review.

[Ramin Halavati, 2017-05-29 08:43:32]

Description was changed from

==========
Network traffic annotation added to content/browser/download.

Network traffic annotation is added to network request of:
content/browser/download/download_request_core.cc
content/browser/download/save_file_manager.cc

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactoring DownloadManager::DownloadURL to add proper annotation.

Refactoring DownloadManager::DownloadURL to add proper network traffic
annotation. All changed files have received a NO_TRAFFIC_ANNOTATION_YET
tag instead of proper annotation and will be annotated in next CLs.

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[David Trainor- moved to gerrit, 2017-05-30 16:35:56]

dtrainor@chromium.org changed reviewers:
+ peter@chromium.org

[David Trainor- moved to gerrit, 2017-05-30 16:35:57]

lgtm % nit.  Adding peter@ for bgfetch.

https://codereview.chromium.org/2860593003/diff/120001/content/public/browser...
File content/public/browser/download_manager.h (right):

https://codereview.chromium.org/2860593003/diff/120001/content/public/browser...
content/public/browser/download_manager.h:126: const
net::NetworkTrafficAnnotationTag& traffic_annotation) = 0;
Should the be part of DownloadUrlParameters?  We can default it to
NO_TRAFFIC_ANNOTATION_YET.  I don't feel too strongly either way, especially
since we're going to mandate this for every request, but it might make the
plumbing cleaner.

[Ramin Halavati, 2017-05-31 05:16:01]

rhalavati@chromium.org changed reviewers:
+ jochen@chromium.org

[Ramin Halavati, 2017-05-31 05:16:01]

Thank you David, comment addressed, please review.

Jochen, Peter,
I've added network traffic annotation to DownloadManager and to avoid making
this CL more hairy, I've added NO_TRAFFIC_ANNOTATION_YET tags to every call site
that needs annotation and will send separate CLs to the owners after this one
lands.

Peter,
Please review content/browser/background_fetch/*

Jochen,
Please review:
chrome/browser/extensions/api/downloads/downloads_api.cc
chrome/browser/extensions/api/streams_private/streams_private_apitest.cc
chrome/browser/extensions/webstore_installer.cc
chrome/browser/renderer_context_menu/render_view_context_menu.cc
content/browser/frame_host/render_frame_message_filter.cc
content/browser/indexed_db/indexed_db_internals_ui.cc
content/browser/web_contents/web_contents_impl.cc
content/public/browser/download_manager.h

https://codereview.chromium.org/2860593003/diff/120001/content/public/browser...
File content/public/browser/download_manager.h (right):

https://codereview.chromium.org/2860593003/diff/120001/content/public/browser...
content/public/browser/download_manager.h:126: const
net::NetworkTrafficAnnotationTag& traffic_annotation) = 0;
On 2017/05/30 16:35:57, David Trainor-ping if over 24h wrote:
> Should the be part of DownloadUrlParameters?  We can default it to
> NO_TRAFFIC_ANNOTATION_YET.  I don't feel too strongly either way, especially
> since we're going to mandate this for every request, but it might make the
> plumbing cleaner.

We will gradually remove all NO_TRAFFIC_ANNOTATION_YET tags, they are temporary
until all call sites are properly annotated.
I am also not strongly towards current approach, but this is more similar to the
rest of repository and if we add a default UNASSIGNED value, we would need to
have online checks to find if it is properly annotated.

[David Trainor- moved to gerrit, 2017-05-31 06:42:36]

On 2017/05/31 05:16:01, Ramin Halavati wrote:
> Thank you David, comment addressed, please review.
> 
> Jochen, Peter,
> I've added network traffic annotation to DownloadManager and to avoid making
> this CL more hairy, I've added NO_TRAFFIC_ANNOTATION_YET tags to every call
site
> that needs annotation and will send separate CLs to the owners after this one
> lands.
> 
> Peter,
> Please review content/browser/background_fetch/*
> 
> Jochen,
> Please review:
> chrome/browser/extensions/api/downloads/downloads_api.cc
> chrome/browser/extensions/api/streams_private/streams_private_apitest.cc
> chrome/browser/extensions/webstore_installer.cc
> chrome/browser/renderer_context_menu/render_view_context_menu.cc
> content/browser/frame_host/render_frame_message_filter.cc
> content/browser/indexed_db/indexed_db_internals_ui.cc
> content/browser/web_contents/web_contents_impl.cc
> content/public/browser/download_manager.h
> 
>
https://codereview.chromium.org/2860593003/diff/120001/content/public/browser...
> File content/public/browser/download_manager.h (right):
> 
>
https://codereview.chromium.org/2860593003/diff/120001/content/public/browser...
> content/public/browser/download_manager.h:126: const
> net::NetworkTrafficAnnotationTag& traffic_annotation) = 0;
> On 2017/05/30 16:35:57, David Trainor-ping if over 24h wrote:
> > Should the be part of DownloadUrlParameters?  We can default it to
> > NO_TRAFFIC_ANNOTATION_YET.  I don't feel too strongly either way, especially
> > since we're going to mandate this for every request, but it might make the
> > plumbing cleaner.
> 
> We will gradually remove all NO_TRAFFIC_ANNOTATION_YET tags, they are
temporary
> until all call sites are properly annotated.
> I am also not strongly towards current approach, but this is more similar to
the
> rest of repository and if we add a default UNASSIGNED value, we would need to
> have online checks to find if it is properly annotated.

Ok I'm fine with this then :).

[jochen (gone - plz use gerrit), 2017-06-01 13:21:08]

lgtm

[Peter Beverloo, 2017-06-01 14:17:06]

lgtm

https://codereview.chromium.org/2860593003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2860593003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_job_controller.cc:262:
NO_TRAFFIC_ANNOTATION_YET));
should this be |traffic_annotation|? In the longer term I imagine we have to
cache it as a class member since multiple requests will be spawned, but that can
happen later.

[Ramin Halavati, 2017-06-01 14:35:54]

Thank you Jochen, Peter.

All comments addressed, landing.

https://codereview.chromium.org/2860593003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2860593003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_job_controller.cc:262:
NO_TRAFFIC_ANNOTATION_YET));
On 2017/06/01 14:17:06, Peter Beverloo wrote:
> should this be |traffic_annotation|? In the longer term I imagine we have to
> cache it as a class member since multiple requests will be spawned, but that
can
> happen later.

Done, I will start adding CLs to remove NO_TRAFFIC_ANNOTATION_YET tags after
this CL lands. I will change the class they way you see fit then.

[Ramin Halavati, 2017-06-01 14:36:08]

The CQ bit was checked by rhalavati@chromium.org

[Ramin Halavati, 2017-06-01 14:36:08]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
dtrainor@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2860593003/#ps140001
(title: "Comment addressed.")

[commit-bot: I haz the power, 2017-06-01 14:36:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 15:56:33]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496327768141500,
"parent_rev": "9f09049c45755baeee9ffd7e22c4e430ca96c2d4", "commit_rev":
"0fbaeef38e204c0a97c1792689a2d0fd4fb784ea"}

[commit-bot: I haz the power, 2017-06-01 15:56:44]

Description was changed from

==========
Refactoring DownloadManager::DownloadURL to add proper annotation.

Refactoring DownloadManager::DownloadURL to add proper network traffic
annotation. All changed files have received a NO_TRAFFIC_ANNOTATION_YET
tag instead of proper annotation and will be annotated in next CLs.

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactoring DownloadManager::DownloadURL to add proper annotation.

Refactoring DownloadManager::DownloadURL to add proper network traffic
annotation. All changed files have received a NO_TRAFFIC_ANNOTATION_YET
tag instead of proper annotation and will be annotated in next CLs.

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2860593003
Cr-Commit-Position: refs/heads/master@{#476312}
Committed:
https://chromium.googlesource.com/chromium/src/+/0fbaeef38e204c0a97c1792689a2...
==========

[commit-bot: I haz the power, 2017-06-01 15:56:46]

Committed patchset #3 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0fbaeef38e204c0a97c1792689a2...