Network traffic annotation added to appcache_update_job.

Network traffic annotation added to appcache_update_job.

Network traffic annotation is added to network requests of appcache_update_job
and it's unittests.

BUG=656607
Review-Url: https://codereview.chromium.org/2799133005
Cr-Commit-Position: refs/heads/master@{#473162}
Committed: https://chromium.googlesource.com/chromium/src/+/1e41ff6a3d29ebd4fb42b3540464a723ae3cbef9

[Ramin Halavati, 2017-04-07 13:57:08]

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

[Ramin Halavati, 2017-04-07 13:57:09]

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 appcache_update_job.cc and added 2 annotation template to it.
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.

Network requests in unittests just need a tag specifying that they are created
for tests, so you don't need to do anything about them and please just review
them.

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.

[michaeln, 2017-04-07 20:37:00]

thnx for all the pointers, i have to take a closer look to understand how those
fields should be used before i can take a stab at suggesting values

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:73: constexpr
net::NetworkTrafficAnnotationTag kTrafficAnnotation =
"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 explain what you mean by that?

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:442: url_,
net::DEFAULT_PRIORITY, this, traffic_annotation);
Why not use kTrafficAnnotation here too? Do you want a distinct annotation for
each distinct CreateRequest callsite?

[Ramin Halavati, 2017-04-10 05:50:31]

Comments addressed. Review please.

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:73: constexpr
net::NetworkTrafficAnnotationTag kTrafficAnnotation =
On 2017/04/07 20:36:59, michaeln wrote:
> "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 explain what you mean by that?

Sometimes, based on the inputs to the function that includes the network
request, it does different tasks that require different annotations. In this
cases we add annotations to the functions that call this function and just pass
it to here.
E.g. if your URLFetcher has different tasks that cannot be described as one
annotation, I can change the CL so that annotation would be an input to 
AppCacheUpdateJob::URLFetcher::URLFetcher.

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:442: url_,
net::DEFAULT_PRIORITY, this, traffic_annotation);
On 2017/04/07 20:36:59, michaeln wrote:
> Why not use kTrafficAnnotation here too? Do you want a distinct annotation for
> each distinct CreateRequest callsite?

If both network requests can be described with one annotation, we can use
kTrafficAnnotation here as well.

[Ramin Halavati, 2017-04-18 11:38:26]

On 2017/04/10 05:50:31, Ramin Halavati wrote:
> Comments addressed. Review please.
> 
>
https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
> File content/browser/appcache/appcache_update_job.cc (right):
> 
>
https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
> content/browser/appcache/appcache_update_job.cc:73: constexpr
> net::NetworkTrafficAnnotationTag kTrafficAnnotation =
> On 2017/04/07 20:36:59, michaeln wrote:
> > "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 explain what you mean by that?
> 
> Sometimes, based on the inputs to the function that includes the network
> request, it does different tasks that require different annotations. In this
> cases we add annotations to the functions that call this function and just
pass
> it to here.
> E.g. if your URLFetcher has different tasks that cannot be described as one
> annotation, I can change the CL so that annotation would be an input to 
> AppCacheUpdateJob::URLFetcher::URLFetcher.
> 
>
https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
> content/browser/appcache/appcache_update_job.cc:442: url_,
> net::DEFAULT_PRIORITY, this, traffic_annotation);
> On 2017/04/07 20:36:59, michaeln wrote:
> > Why not use kTrafficAnnotation here too? Do you want a distinct annotation
for
> > each distinct CreateRequest callsite?
> 
> If both network requests can be described with one annotation, we can use
> kTrafficAnnotation here as well.

Hi Michael,

A gentle reminder on this.

[michaeln, 2017-04-20 19:54:14]

right, thnx for the ping

[michaeln, 2017-04-20 20:22:00]

I don't know how to fill out smoe of the policy section?

Cookies are allowed and the cookies store used is the same one pages loaded into
tabs use. 

Is this the right way to express that?
cookies_store: "Profile cookie store"

I don't know what to say about these?
chrome_policy {}
policy_exception_justification: ""

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:93: })");
constexpr net::NetworkTrafficAnnotationTag kTrafficAnnotation =
    net::DefineNetworkTrafficAnnotation("appcache_update_job", R"(
      semantics {
        sender: "HTML5 AppCache System"
        description:
          "Web pages can include a link to a manifest file which lists "
          "resources to be cached for offline access. The AppCache system"
          "retrieves those resources in the background."
        trigger:
          "User visits a web page containing a <html manifest=manifestUrl> tag,
"
          "or navigates to a document retrived from an existing appcache."
        data: "None"
        destination: WEBSITE
      }
      policy {
        cookies_allowed: true
        cookies_store: "Profile cookie store"
        setting: "The Content Cookies settings enable or disable this feature."
        
        // I don't know how to fill these out?
        chrome_policy {}
        policy_exception_justification: ""
      })");

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:442: url_,
net::DEFAULT_PRIORITY, this, traffic_annotation);
On 2017/04/10 05:50:31, Ramin Halavati wrote:
> On 2017/04/07 20:36:59, michaeln wrote:
> > Why not use kTrafficAnnotation here too? Do you want a distinct annotation
for
> > each distinct CreateRequest callsite?
> 
> If both network requests can be described with one annotation, we can use
> kTrafficAnnotation here as well.

Both can be described with the same annotation.

[Ramin Halavati, 2017-04-21 05:41:14]

Thank you very much, I updated the annotation and merged them. Please review.

About the cookies, I think it's correct. We might change it to 'profile' later
but now this matches the other annotations.

For policy, I looked in the list
(http://dev.chromium.org/administrators/policy-list-3) and didn't find anything
that feels relevant to your code, but if there is a policy that blocks these
requests, please point me to it and I will add it in the annotation.

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:93: })");
On 2017/04/20 20:22:00, michaeln wrote:
> constexpr net::NetworkTrafficAnnotationTag kTrafficAnnotation =
>     net::DefineNetworkTrafficAnnotation("appcache_update_job", R"(
>       semantics {
>         sender: "HTML5 AppCache System"
>         description:
>           "Web pages can include a link to a manifest file which lists "
>           "resources to be cached for offline access. The AppCache system"
>           "retrieves those resources in the background."
>         trigger:
>           "User visits a web page containing a <html manifest=manifestUrl>
tag,
> "
>           "or navigates to a document retrived from an existing appcache."
>         data: "None"
>         destination: WEBSITE
>       }
>       policy {
>         cookies_allowed: true
>         cookies_store: "Profile cookie store"
>         setting: "The Content Cookies settings enable or disable this
feature."
>         
>         // I don't know how to fill these out?
>         chrome_policy {}
>         policy_exception_justification: ""
>       })");

Done.

https://codereview.chromium.org/2799133005/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:442: url_,
net::DEFAULT_PRIORITY, this, traffic_annotation);
On 2017/04/20 20:22:00, michaeln wrote:
> On 2017/04/10 05:50:31, Ramin Halavati wrote:
> > On 2017/04/07 20:36:59, michaeln wrote:
> > > Why not use kTrafficAnnotation here too? Do you want a distinct annotation
> for
> > > each distinct CreateRequest callsite?
> > 
> > If both network requests can be described with one annotation, we can use
> > kTrafficAnnotation here as well.
> 
> Both can be described with the same annotation.

Done.

[michaeln, 2017-04-21 20:17:47]

lgtm % maybe there is something to put in the "policy exception" section?

https://codereview.chromium.org/2799133005/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:93:
policy_exception_justification: "Not Implemented."
The cookies settings affect the entire appcache feature including these
requests.

DefaultCookiesSetting
CokiesAllowedForUrls
CookiesBlockedForUrls

If policy settings can fiddle those content settings, they can indirectly
enable/disable these requests.

[Ramin Halavati, 2017-04-24 05:46:20]

Comment addressed, please review.

https://codereview.chromium.org/2799133005/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:93:
policy_exception_justification: "Not Implemented."
On 2017/04/21 20:17:47, michaeln wrote:
> The cookies settings affect the entire appcache feature including these
> requests.
> 
> DefaultCookiesSetting
> CokiesAllowedForUrls
> CookiesBlockedForUrls
> 
> If policy settings can fiddle those content settings, they can indirectly
> enable/disable these requests.

I am not sure if I correctly got it. You mean if cookies are generally or
partially disabled, these request and other requests related to appcache will be
disabled?

[michaeln, 2017-04-24 20:38:10]

i don't see a new snapshot, although i'm sure it would lgtm

https://codereview.chromium.org/2799133005/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:93:
policy_exception_justification: "Not Implemented."
On 2017/04/24 05:46:20, Ramin Halavati wrote:
> On 2017/04/21 20:17:47, michaeln wrote:
> > The cookies settings affect the entire appcache feature including these
> > requests.
> > 
> > DefaultCookiesSetting
> > CokiesAllowedForUrls
> > CookiesBlockedForUrls
> > 
> > If policy settings can fiddle those content settings, they can indirectly
> > enable/disable these requests.
> 
> I am not sure if I correctly got it. You mean if cookies are generally or
> partially disabled, these request and other requests related to appcache will
be
> disabled?

Think of an appcache as a big cookie, if you disable cookies you disable
appcaches across the board. If you disable cookies for a single site, you
disable appcaches for just that single site. And if you disable the feature, you
disable these requests.

[Ramin Halavati, 2017-04-26 05:49:19]

Thank you very much, annotation updated, please review.

https://codereview.chromium.org/2799133005/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:93:
policy_exception_justification: "Not Implemented."
On 2017/04/24 20:38:10, michaeln wrote:
> On 2017/04/24 05:46:20, Ramin Halavati wrote:
> > On 2017/04/21 20:17:47, michaeln wrote:
> > > The cookies settings affect the entire appcache feature including these
> > > requests.
> > > 
> > > DefaultCookiesSetting
> > > CokiesAllowedForUrls
> > > CookiesBlockedForUrls
> > > 
> > > If policy settings can fiddle those content settings, they can indirectly
> > > enable/disable these requests.
> > 
> > I am not sure if I correctly got it. You mean if cookies are generally or
> > partially disabled, these request and other requests related to appcache
will
> be
> > disabled?
> 
> Think of an appcache as a big cookie, if you disable cookies you disable
> appcaches across the board. If you disable cookies for a single site, you
> disable appcaches for just that single site. And if you disable the feature,
you
> disable these requests.

Thank you for clarification.

[michaeln, 2017-04-26 19:11:47]

still lgtm

[Ramin Halavati, 2017-04-27 05:15:50]

rhalavati@chromium.org changed reviewers:
+ battre@chromium.org, msramek@chromium.org

[Ramin Halavati, 2017-04-27 05:15:50]

Thank you very much.

Dominic, Martin,
Any comments?

[msramek, 2017-05-04 10:48:02]

https://codereview.chromium.org/2799133005/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:83: "tag, or navigates to a
document retrived from an existing appcache."
typo: retrieved

https://codereview.chromium.org/2799133005/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:83: "tag, or navigates to a
document retrived from an existing appcache."
Also, this is a bit unclear to me. If I retrieved the document from an existing
appcache, then presumably I'm offline, and also everything is already cached, so
there's no need to fetch something?

Michael, could you please clarify?

https://codereview.chromium.org/2799133005/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:95:
policy_exception_justification: "Not Implemented."
typo: *i*mplemented

[michaeln, 2017-05-04 17:27:51]

https://codereview.chromium.org/2799133005/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:83: "tag, or navigates to a
document retrived from an existing appcache."
On 2017/05/04 10:48:01, msramek (recovering) wrote:
> Also, this is a bit unclear to me. If I retrieved the document from an
existing
> appcache, then presumably I'm offline, and also everything is already cached,
so
> there's no need to fetch something?
> 
> Michael, could you please clarify?

Appcached resources certainly can be utilized while online, and when online, the
set of resources may be updated.

[Ramin Halavati, 2017-05-05 07:51:15]

Patchset #6 (id:100001) has been deleted

[Ramin Halavati, 2017-05-05 07:51:24]

Patchset #5 (id:80001) has been deleted

[Ramin Halavati, 2017-05-05 07:58:55]

All comments addressed, please review.

https://codereview.chromium.org/2799133005/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:83: "tag, or navigates to a
document retrived from an existing appcache."
On 2017/05/04 17:27:51, michaeln wrote:
> On 2017/05/04 10:48:01, msramek (recovering) wrote:
> > Also, this is a bit unclear to me. If I retrieved the document from an
> existing
> > appcache, then presumably I'm offline, and also everything is already
cached,
> so
> > there's no need to fetch something?
> > 
> > Michael, could you please clarify?
> 
> Appcached resources certainly can be utilized while online, and when online,
the
> set of resources may be updated.

Done.

https://codereview.chromium.org/2799133005/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:95:
policy_exception_justification: "Not Implemented."
On 2017/05/04 10:48:02, msramek (recovering) wrote:
> typo: *i*mplemented

Done.

[msramek, 2017-05-15 13:36:11]

Thanks for the explanation and the update. I just noticed two more things.

https://codereview.chromium.org/2799133005/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_update_job.cc:90: cookies_store: "Profile
cookie store"
I think we say "user" in most other annotations.

https://codereview.chromium.org/2799133005/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_update_job.cc:96:
policy_exception_justification: "Not implemented."
Based on the "setting" field, this would be the DefaultCookiesSetting policy.

[Ramin Halavati, 2017-05-15 14:01:03]

Thank you Martin, comments addressed, please review.

Michaeln,
Still LGTM?

https://codereview.chromium.org/2799133005/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2799133005/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_update_job.cc:90: cookies_store: "Profile
cookie store"
On 2017/05/15 13:36:11, msramek wrote:
> I think we say "user" in most other annotations.

Done.

https://codereview.chromium.org/2799133005/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_update_job.cc:96:
policy_exception_justification: "Not implemented."
On 2017/05/15 13:36:11, msramek wrote:
> Based on the "setting" field, this would be the DefaultCookiesSetting policy.

Done.

[Ramin Halavati, 2017-05-17 06:01:50]

On 2017/05/15 14:01:03, Ramin Halavati wrote:
> Thank you Martin, comments addressed, please review.
> 
> Michaeln,
> Still LGTM?
> 
>
https://codereview.chromium.org/2799133005/diff/120001/content/browser/appcac...
> File content/browser/appcache/appcache_update_job.cc (right):
> 
>
https://codereview.chromium.org/2799133005/diff/120001/content/browser/appcac...
> content/browser/appcache/appcache_update_job.cc:90: cookies_store: "Profile
> cookie store"
> On 2017/05/15 13:36:11, msramek wrote:
> > I think we say "user" in most other annotations.
> 
> Done.
> 
>
https://codereview.chromium.org/2799133005/diff/120001/content/browser/appcac...
> content/browser/appcache/appcache_update_job.cc:96:
> policy_exception_justification: "Not implemented."
> On 2017/05/15 13:36:11, msramek wrote:
> > Based on the "setting" field, this would be the DefaultCookiesSetting
policy.
> 
> Done.

Michael,

Sorry to remind right after vacation, but is this still LGTM?

[michaeln, 2017-05-19 00:33:02]

yes, still lgtm

[Ramin Halavati, 2017-05-19 05:49:33]

On 2017/05/19 00:33:02, michaeln wrote:
> yes, still lgtm

Thank you Michael.

Martin?

[msramek, 2017-05-19 08:34:54]

LGTM

[Ramin Halavati, 2017-05-19 08:35:41]

The CQ bit was checked by rhalavati@chromium.org

[commit-bot: I haz the power, 2017-05-19 08:36:11]

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-19 10:40:32]

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

[commit-bot: I haz the power, 2017-05-19 10:40:33]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ramin Halavati, 2017-05-19 10:41:42]

The CQ bit was checked by rhalavati@chromium.org

[commit-bot: I haz the power, 2017-05-19 10:42:00]

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-19 11:41:21]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495190502873270,
"parent_rev": "7d364e7096cb53bf7217f6cc3a6718eea976578b", "commit_rev":
"1e41ff6a3d29ebd4fb42b3540464a723ae3cbef9"}

[commit-bot: I haz the power, 2017-05-19 11:41:44]

Description was changed from

==========
Network traffic annotation added to appcache_update_job.

Network traffic annotation is added to network requests of appcache_update_job
and it's unittests.

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

to

==========
Network traffic annotation added to appcache_update_job.

Network traffic annotation is added to network requests of appcache_update_job
and it's unittests.

BUG=656607

Review-Url: https://codereview.chromium.org/2799133005
Cr-Commit-Position: refs/heads/master@{#473162}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e41ff6a3d29ebd4fb42b3540464...
==========

[commit-bot: I haz the power, 2017-05-19 11:41:46]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/1e41ff6a3d29ebd4fb42b3540464...