Network traffic annotation added to URLLoaderImpl.

Network traffic annotation added to URLLoaderImpl.

Network traffic annotation is added to network request of:
content/network/url_loader_impl.cc

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2893233002
Cr-Commit-Position: refs/heads/master@{#478588}
Committed: https://chromium.googlesource.com/chromium/src/+/4cda417b683318c0c8ca8a0ef2a94ebc4ee5bea8

[Ramin Halavati, 2017-05-22 08:15:24]

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

[Ramin Halavati, 2017-05-22 08:15:25]

Matt,

Could you please suggest annotations for this class? Or should it be moved to
caller functions?

[mmenke, 2017-05-22 15:26:54]

On 2017/05/22 08:15:25, Ramin Halavati wrote:
> Matt,
> 
> Could you please suggest annotations for this class? Or should it be moved to
> caller functions?

Annotating this one doesn't make sense - it should take its annotation through
its Mojo interfaces.  Eventually, the plan is to get the network stack out of
process, and then all browser and renderer-initiated requests will go through
here.  That's a long way off, though.

[Ramin Halavati, 2017-05-22 16:30:56]

On 2017/05/22 15:26:54, mmenke wrote:
> On 2017/05/22 08:15:25, Ramin Halavati wrote:
> > Matt,
> > 
> > Could you please suggest annotations for this class? Or should it be moved
to
> > caller functions?
> 
> Annotating this one doesn't make sense - it should take its annotation through
> its Mojo interfaces.  Eventually, the plan is to get the network stack out of
> process, and then all browser and renderer-initiated requests will go through
> here.  That's a long way off, though.

Thank you, what do you suggest we do with it? I am working on presubmit check
and it will ban all un-annotated usages in Chrome. Should this be an exception,
can we add a NOT_ANNOTATED_YET tag and a comment that it's moving, put a generic
annotation, or something else?

[mmenke, 2017-05-22 16:40:00]

On 2017/05/22 16:30:56, Ramin Halavati wrote:
> On 2017/05/22 15:26:54, mmenke wrote:
> > On 2017/05/22 08:15:25, Ramin Halavati wrote:
> > > Matt,
> > > 
> > > Could you please suggest annotations for this class? Or should it be moved
> to
> > > caller functions?
> > 
> > Annotating this one doesn't make sense - it should take its annotation
through
> > its Mojo interfaces.  Eventually, the plan is to get the network stack out
of
> > process, and then all browser and renderer-initiated requests will go
through
> > here.  That's a long way off, though.
> 
> Thank you, what do you suggest we do with it? I am working on presubmit check
> and it will ban all un-annotated usages in Chrome. Should this be an
exception,
> can we add a NOT_ANNOTATED_YET tag and a comment that it's moving, put a
generic
> annotation, or something else?

You could add something to the mojo interface, maybe, and add a pre-submit check
for that interface as well, or just make it a mandatory argument when starting a
request via UrlLoader?  Then pass in the argument it gets from the loader.

That way, when we eventually switch over, everything will have to pick up the
existing annotations and pass them along.

[Ramin Halavati, 2017-05-22 16:44:03]

On 2017/05/22 16:40:00, mmenke wrote:
> On 2017/05/22 16:30:56, Ramin Halavati wrote:
> > On 2017/05/22 15:26:54, mmenke wrote:
> > > On 2017/05/22 08:15:25, Ramin Halavati wrote:
> > > > Matt,
> > > > 
> > > > Could you please suggest annotations for this class? Or should it be
moved
> > to
> > > > caller functions?
> > > 
> > > Annotating this one doesn't make sense - it should take its annotation
> through
> > > its Mojo interfaces.  Eventually, the plan is to get the network stack out
> of
> > > process, and then all browser and renderer-initiated requests will go
> through
> > > here.  That's a long way off, though.
> > 
> > Thank you, what do you suggest we do with it? I am working on presubmit
check
> > and it will ban all un-annotated usages in Chrome. Should this be an
> exception,
> > can we add a NOT_ANNOTATED_YET tag and a comment that it's moving, put a
> generic
> > annotation, or something else?
> 
> You could add something to the mojo interface, maybe, and add a pre-submit
check
> for that interface as well, or just make it a mandatory argument when starting
a
> request via UrlLoader?  Then pass in the argument it gets from the loader.
> 
> That way, when we eventually switch over, everything will have to pick up the
> existing annotations and pass them along.

Thank you.
So just to confirm, I add a NetworkTrafficAnnotationTag as an argument to
NetworkServiceURLLoaderFactoryImpl::CreateLoaderAndStart, and add annotations to
all it's callers?

[mmenke, 2017-05-22 16:46:05]

On 2017/05/22 16:44:03, Ramin Halavati wrote:
> On 2017/05/22 16:40:00, mmenke wrote:
> > On 2017/05/22 16:30:56, Ramin Halavati wrote:
> > > On 2017/05/22 15:26:54, mmenke wrote:
> > > > On 2017/05/22 08:15:25, Ramin Halavati wrote:
> > > > > Matt,
> > > > > 
> > > > > Could you please suggest annotations for this class? Or should it be
> moved
> > > to
> > > > > caller functions?
> > > > 
> > > > Annotating this one doesn't make sense - it should take its annotation
> > through
> > > > its Mojo interfaces.  Eventually, the plan is to get the network stack
out
> > of
> > > > process, and then all browser and renderer-initiated requests will go
> > through
> > > > here.  That's a long way off, though.
> > > 
> > > Thank you, what do you suggest we do with it? I am working on presubmit
> check
> > > and it will ban all un-annotated usages in Chrome. Should this be an
> > exception,
> > > can we add a NOT_ANNOTATED_YET tag and a comment that it's moving, put a
> > generic
> > > annotation, or something else?
> > 
> > You could add something to the mojo interface, maybe, and add a pre-submit
> check
> > for that interface as well, or just make it a mandatory argument when
starting
> a
> > request via UrlLoader?  Then pass in the argument it gets from the loader.
> > 
> > That way, when we eventually switch over, everything will have to pick up
the
> > existing annotations and pass them along.
> 
> Thank you.
> So just to confirm, I add a NetworkTrafficAnnotationTag as an argument to
> NetworkServiceURLLoaderFactoryImpl::CreateLoaderAndStart, and add annotations
to
> all it's callers?

It would actually need to be added to
content/common/url_loader_factory.mojom.h;s CreateLoaderAndStart method (And all
subclasses implementing it, and all callers of it).

[Ramin Halavati, 2017-05-23 12:22:35]

Description was changed from

==========
Network traffic annotation added to URLLoaderImpl.

Network traffic annotation is added to network request of:
content/network/url_loader_impl.cc

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

to

==========
Network traffic annotation added to URLLoaderImpl.

Network traffic annotation is added to network request of:
content/network/url_loader_impl.cc

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Ramin Halavati, 2017-05-23 12:40:38]

Patchset #2 (id:20001) has been deleted

[Ramin Halavati, 2017-05-23 12:40:48]

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

[commit-bot: I haz the power, 2017-05-23 12:41:10]

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-23 12:45:12]

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

[commit-bot: I haz the power, 2017-05-23 12:45:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ramin Halavati, 2017-05-23 12:53:49]

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

[commit-bot: I haz the power, 2017-05-23 12:54:08]

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-23 13:11:44]

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

[commit-bot: I haz the power, 2017-05-23 13:11:45]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[Ramin Halavati, 2017-05-23 13:50:47]

Patchset #2 (id:40001) has been deleted

[Ramin Halavati, 2017-05-23 13:50:54]

Patchset #2 (id:60001) has been deleted

[Ramin Halavati, 2017-05-23 13:53:19]

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

[Ramin Halavati, 2017-05-23 13:53:20]

Matt,

Changed the CL to get the annotation from CreateLoaderAndStart method. To avoid
requiring many reviewers to add annotations in this CL, added
NO_TRAFFIC_ANNOTATION_YET wherever it was needed and will create separate CLs
for them after this is landed.
Please review.


Emily,

I have added net::NetworkTrafficAnnotationTag and
net::PartialNetworkTrafficAnnotationTag as new parameters for url_loader.mojom
and it seems that require serialization. Please review cc/ipc/*

[Ramin Halavati, 2017-05-25 10:09:53]

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

[commit-bot: I haz the power, 2017-05-25 10:10:04]

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-25 10:36:14]

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

[commit-bot: I haz the power, 2017-05-25 10:36:15]

Dry run: 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_...)

[mmenke, 2017-05-25 19:31:07]

On 2017/05/23 13:53:20, Ramin Halavati wrote:
> Matt,
> 
> Changed the CL to get the annotation from CreateLoaderAndStart method. To
avoid
> requiring many reviewers to add annotations in this CL, added
> NO_TRAFFIC_ANNOTATION_YET wherever it was needed and will create separate CLs
> for them after this is landed.
> Please review.
> 
> 
> Emily,
> 
> I have added net::NetworkTrafficAnnotationTag and
> net::PartialNetworkTrafficAnnotationTag as new parameters for url_loader.mojom
> and it seems that require serialization. Please review cc/ipc/*

Sorry for the delay, this LGTM, but it looks like you have a compile error?

[Ramin Halavati, 2017-05-26 05:26:52]

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

[commit-bot: I haz the power, 2017-05-26 05:27:39]

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 05:59:18]

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

[commit-bot: I haz the power, 2017-05-26 05:59:19]

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-26 07:38:14]

Patchset #4 (id:120001) has been deleted

[Ramin Halavati, 2017-05-26 07:43:05]

On 2017/05/25 19:31:07, mmenke wrote:
> On 2017/05/23 13:53:20, Ramin Halavati wrote:
> > Matt,
> > 
> > Changed the CL to get the annotation from CreateLoaderAndStart method. To
> avoid
> > requiring many reviewers to add annotations in this CL, added
> > NO_TRAFFIC_ANNOTATION_YET wherever it was needed and will create separate
CLs
> > for them after this is landed.
> > Please review.
> > 
> > 
> > Emily,
> > 
> > I have added net::NetworkTrafficAnnotationTag and
> > net::PartialNetworkTrafficAnnotationTag as new parameters for
url_loader.mojom
> > and it seems that require serialization. Please review cc/ipc/*
> 
> Sorry for the delay, this LGTM, but it looks like you have a compile error?

Thank you Matt.


Emily,

The gen/content/common/url_loader_factory.mojom.cc file that is generated
includes an uninitialized variable of type net::NetworkTrafficAnnotationTag and
that results in compile error. Do I have any way except adding a default value
for net::NetworkTrafficAnnotationTag? (I prefer not to do it because if I do so,
I will need to add a check to its use cases that it is properly initialized.)
Also please review cc/ipc/*.

[Ramin Halavati, 2017-05-30 05:08:05]

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

[commit-bot: I haz the power, 2017-05-30 05:08:39]

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-30 05:14:03]

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

[commit-bot: I haz the power, 2017-05-30 05:14:04]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ramin Halavati, 2017-05-30 05:23:41]

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

[commit-bot: I haz the power, 2017-05-30 05:24:07]

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-30 05:34:54]

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

[commit-bot: I haz the power, 2017-05-30 05:34:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ramin Halavati, 2017-05-30 05:41:52]

Patchset #5 (id:160001) has been deleted

[Ramin Halavati, 2017-05-30 05:42:10]

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

[commit-bot: I haz the power, 2017-05-30 05:42:23]

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-30 06:04:23]

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

[commit-bot: I haz the power, 2017-05-30 06:04:24]

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_...)

[estark, 2017-06-01 00:22:47]

estark@chromium.org changed reviewers:
+ blundell@chromium.org

[estark, 2017-06-01 00:22:48]

cc/ipc lgtm for security, but I'm not sure about your default initialization
question. Adding blundell@ who might be able to help.

[Ramin Halavati, 2017-06-01 09:39:27]

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

[commit-bot: I haz the power, 2017-06-01 09:39:44]

Dry run: CQ is trying da patch.

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

[Ramin Halavati, 2017-06-01 10:24:31]

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

[Ramin Halavati, 2017-06-01 10:24:32]

Thank you Emily.

Colin,
Do you have any suggestions for an alternative solution which doesn't require
adding a default value for net::NetworkTrafficAnnotationTag?

Dominic,
Could you please take a look at network_traffic_annotation.h?

[blundell, 2017-06-01 11:26:52]

blundell@chromium.org changed reviewers:
+ yzshen@chromium.org

[blundell, 2017-06-01 11:26:53]

blundell@ -> yzshen@

Yuzhu, there's the following question on this CL:

"The gen/content/common/url_loader_factory.mojom.cc file that is generated
includes an uninitialized variable of type net::NetworkTrafficAnnotationTag and
that results in compile error. Do I have any way except adding a default value
for net::NetworkTrafficAnnotationTag? (I prefer not to do it because if I do so,
I will need to add a check to its use cases that it is properly initialized.)"

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

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

[commit-bot: I haz the power, 2017-06-01 11:33:34]

Dry run: This issue passed the CQ dry run.

[battre, 2017-06-01 12:36:36]

LGTM with some comments. I think this is not pretty but the best workaround I
can come up with.

https://codereview.chromium.org/2893233002/diff/200001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2893233002/diff/200001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:1074: l->append("(");
drop previous line.

https://codereview.chromium.org/2893233002/diff/200001/content/common/url_loa...
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/2893233002/diff/200001/content/common/url_loa...
content/common/url_loader_factory.mojom:27: NetworkTrafficAnnotationTag
traffic_annotation);
Shouldn't this be MutableNetworkTrafficAnnotationTag? I see how you did the
naming in the typemap but I wonder whether this makes things more complicated to
understand.

https://codereview.chromium.org/2893233002/diff/200001/net/traffic_annotation...
File net/traffic_annotation/network_traffic_annotation.h (right):

https://codereview.chromium.org/2893233002/diff/200001/net/traffic_annotation...
net/traffic_annotation/network_traffic_annotation.h:205:
MutableNetworkTrafficAnnotationTag(
explicit MutableNetworkTrafficAnnotationTag

same below

https://codereview.chromium.org/2893233002/diff/200001/net/traffic_annotation...
net/traffic_annotation/network_traffic_annotation.h:208: int32_t
unique_id_hash_code = TRAFFIC_ANNOTATION_UNINITIALIZED;
isn't this default value duplicative given the constructor?

[Ramin Halavati, 2017-06-01 13:14:03]

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

[Ramin Halavati, 2017-06-01 13:14:27]

Thank you Dominic, all comments addressed.

https://codereview.chromium.org/2893233002/diff/200001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2893233002/diff/200001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:1074: l->append("(");
On 2017/06/01 12:36:36, battre wrote:
> drop previous line.

It had to be a ", ".

https://codereview.chromium.org/2893233002/diff/200001/content/common/url_loa...
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/2893233002/diff/200001/content/common/url_loa...
content/common/url_loader_factory.mojom:27: NetworkTrafficAnnotationTag
traffic_annotation);
On 2017/06/01 12:36:36, battre wrote:
> Shouldn't this be MutableNetworkTrafficAnnotationTag? I see how you did the
> naming in the typemap but I wonder whether this makes things more complicated
to
> understand.

Done.

https://codereview.chromium.org/2893233002/diff/200001/net/traffic_annotation...
File net/traffic_annotation/network_traffic_annotation.h (right):

https://codereview.chromium.org/2893233002/diff/200001/net/traffic_annotation...
net/traffic_annotation/network_traffic_annotation.h:205:
MutableNetworkTrafficAnnotationTag(
On 2017/06/01 12:36:36, battre wrote:
> explicit MutableNetworkTrafficAnnotationTag
> 
> same below

Done.

https://codereview.chromium.org/2893233002/diff/200001/net/traffic_annotation...
net/traffic_annotation/network_traffic_annotation.h:208: int32_t
unique_id_hash_code = TRAFFIC_ANNOTATION_UNINITIALIZED;
On 2017/06/01 12:36:36, battre wrote:
> isn't this default value duplicative given the constructor?

Done.

[commit-bot: I haz the power, 2017-06-01 13:14:28]

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-06-01 15:08:10]

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

[commit-bot: I haz the power, 2017-06-01 15:08:11]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-06-01 16:03:54]

On 2017/06/01 11:26:53, blundell wrote:
> blundell@ -> yzshen@
> 
> Yuzhu, there's the following question on this CL:
> 
> "The gen/content/common/url_loader_factory.mojom.cc file that is generated
> includes an uninitialized variable of type net::NetworkTrafficAnnotationTag
and
> that results in compile error. Do I have any way except adding a default value
> for net::NetworkTrafficAnnotationTag? (I prefer not to do it because if I do
so,
> I will need to add a check to its use cases that it is properly initialized.)"

I think you mean a default constructor?
Yes, during deserialization the generated code creates a default constructed
object and then passes to the ParamTraits/StructTraits as an output parameter.
That is the only way at the moment.

[yzshen1, 2017-06-01 16:04:27]

Some drive-by comments:

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:1016: new (p)
net::MutableNetworkTrafficAnnotationTag({unique_id_hash_code});
This means the destructor of the original contents is not called. It may not be
an issue for now, but may cause confusion if the structure's destructor later
becomes meaningful.

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:1059: new (p)
net::MutablePartialNetworkTrafficAnnotationTag(
ditto

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.h
File cc/ipc/cc_param_traits.h (right):

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.h:158: struct CC_IPC_EXPORT
ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
why some structure from net/ has its param traits defined in cc/?

[Ramin Halavati, 2017-06-01 19:24:08]

On 2017/06/01 16:04:27, yzshen1 wrote:
> Some drive-by comments:
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.cc
> File cc/ipc/cc_param_traits.cc (right):
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
> cc/ipc/cc_param_traits.cc:1016: new (p)
> net::MutableNetworkTrafficAnnotationTag({unique_id_hash_code});
> This means the destructor of the original contents is not called. It may not
be
> an issue for now, but may cause confusion if the structure's destructor later
> becomes meaningful.
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
> cc/ipc/cc_param_traits.cc:1059: new (p)
> net::MutablePartialNetworkTrafficAnnotationTag(
> ditto
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.h
> File cc/ipc/cc_param_traits.h (right):
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
> cc/ipc/cc_param_traits.h:158: struct CC_IPC_EXPORT
> ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
> why some structure from net/ has its param traits defined in cc/?

Thanks Yuzhu,

I will apply the two comments on the .cc file tomorrow, but I didn't quite get
if by the question 
"why some structure from net/ has its param traits defined in cc/?"
you mean you are objecting this approach or suggesting moving it somewhere else.

NetworkTrafficAnnotation describes a network request and is transferred to each
network request function.
As it needs to be added to a .mojom file, I have defined it here.
Do you suggest its param trait to be defined somewhere else, or totally object
this approach?

[Ramin Halavati, 2017-06-02 07:47:06]

Yuzhu,

All comments addressed, please review.

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:1016: new (p)
net::MutableNetworkTrafficAnnotationTag({unique_id_hash_code});
On 2017/06/01 16:04:27, yzshen1 wrote:
> This means the destructor of the original contents is not called. It may not
be
> an issue for now, but may cause confusion if the structure's destructor later
> becomes meaningful.

Done.

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:1059: new (p)
net::MutablePartialNetworkTrafficAnnotationTag(
On 2017/06/01 16:04:26, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.h
File cc/ipc/cc_param_traits.h (right):

https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.h:158: struct CC_IPC_EXPORT
ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
On 2017/06/01 16:04:27, yzshen1 wrote:
> why some structure from net/ has its param traits defined in cc/?

NetworkTrafficAnnotation describes a network request and is transferred to each
network request function.
As it needs to be added to a .mojom file, I have defined it here.
Do you suggest I create a net_param_treits and move these lines there, or
totally object this approach?

[blundell, 2017-06-02 10:54:58]

On 2017/06/02 07:47:06, Ramin Halavati wrote:
> Yuzhu,
> 
> All comments addressed, please review.
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.cc
> File cc/ipc/cc_param_traits.cc (right):
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
> cc/ipc/cc_param_traits.cc:1016: new (p)
> net::MutableNetworkTrafficAnnotationTag({unique_id_hash_code});
> On 2017/06/01 16:04:27, yzshen1 wrote:
> > This means the destructor of the original contents is not called. It may not
> be
> > an issue for now, but may cause confusion if the structure's destructor
later
> > becomes meaningful.
> 
> Done.
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
> cc/ipc/cc_param_traits.cc:1059: new (p)
> net::MutablePartialNetworkTrafficAnnotationTag(
> On 2017/06/01 16:04:26, yzshen1 wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.h
> File cc/ipc/cc_param_traits.h (right):
> 
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
> cc/ipc/cc_param_traits.h:158: struct CC_IPC_EXPORT
> ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
> On 2017/06/01 16:04:27, yzshen1 wrote:
> > why some structure from net/ has its param traits defined in cc/?
> 
> NetworkTrafficAnnotation describes a network request and is transferred to
each
> network request function.
> As it needs to be added to a .mojom file, I have defined it here.
> Do you suggest I create a net_param_treits and move these lines there, or
> totally object this approach?

I'm 99% sure Yuzhu is just suggesting the former, i.e., it's hard to see any
reason why these param traits are related to //cc.

[Ramin Halavati, 2017-06-02 12:38:27]

Patchset #10 (id:280001) has been deleted

[Ramin Halavati, 2017-06-02 12:38:38]

Patchset #9 (id:260001) has been deleted

[Ramin Halavati, 2017-06-02 12:39:05]

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

[commit-bot: I haz the power, 2017-06-02 12:39:24]

Dry run: CQ is trying da patch.

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

[Ramin Halavati, 2017-06-02 12:40:33]

On 2017/06/02 10:54:58, blundell wrote:
> On 2017/06/02 07:47:06, Ramin Halavati wrote:
> > Yuzhu,
> > 
> > All comments addressed, please review.
> > 
> >
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.cc
> > File cc/ipc/cc_param_traits.cc (right):
> > 
> >
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
> > cc/ipc/cc_param_traits.cc:1016: new (p)
> > net::MutableNetworkTrafficAnnotationTag({unique_id_hash_code});
> > On 2017/06/01 16:04:27, yzshen1 wrote:
> > > This means the destructor of the original contents is not called. It may
not
> > be
> > > an issue for now, but may cause confusion if the structure's destructor
> later
> > > becomes meaningful.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
> > cc/ipc/cc_param_traits.cc:1059: new (p)
> > net::MutablePartialNetworkTrafficAnnotationTag(
> > On 2017/06/01 16:04:26, yzshen1 wrote:
> > > ditto
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits.h
> > File cc/ipc/cc_param_traits.h (right):
> > 
> >
>
https://codereview.chromium.org/2893233002/diff/220001/cc/ipc/cc_param_traits...
> > cc/ipc/cc_param_traits.h:158: struct CC_IPC_EXPORT
> > ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
> > On 2017/06/01 16:04:27, yzshen1 wrote:
> > > why some structure from net/ has its param traits defined in cc/?
> > 
> > NetworkTrafficAnnotation describes a network request and is transferred to
> each
> > network request function.
> > As it needs to be added to a .mojom file, I have defined it here.
> > Do you suggest I create a net_param_treits and move these lines there, or
> > totally object this approach?
> 
> I'm 99% sure Yuzhu is just suggesting the former, i.e., it's hard to see any
> reason why these param traits are related to //cc.

Thank you Colin,

network_traffic_annotation_param_traits.* moved to content/common.
Please review.

[commit-bot: I haz the power, 2017-06-02 12:41:37]

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

[commit-bot: I haz the power, 2017-06-02 12:41:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ramin Halavati, 2017-06-02 13:00:58]

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

[commit-bot: I haz the power, 2017-06-02 13:01:12]

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-06-02 14:33:07]

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

[commit-bot: I haz the power, 2017-06-02 14:33:09]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[yzshen1, 2017-06-02 15:06:18]

Thanks Colin! That was what I meant. :)

https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
File content/common/network_traffic_annotation_param_traits.h (right):

https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
content/common/network_traffic_annotation_param_traits.h:20: struct
CC_IPC_EXPORT ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
Have you considered defining a mojom type and then use StructTraits, instead of
marking it as "Native" and use the old-style ParamTraits? We sometimes reuse
ParamTraits from the legacy IPCs, but seldom introduce new ones.

I notice that net::PartialNetworkTrafficAnnotationTag has different fields
depending on whether we are in debug mode. mojom type doesn't support
preprocessor directives, so maybe it makes sense to use ParamTraits. But please
see my other comment below.

https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
content/common/network_traffic_annotation_param_traits.h:32:
ParamTraits<net::MutablePartialNetworkTrafficAnnotationTag> {
Is this going to be used in future CLs?
I notice net::MutablePartialNetworkTrafficAnnotationTag is not used by mojo
interfaces.

[Ramin Halavati, 2017-06-02 16:46:03]

Thank you. Comments addressed, please review.

https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
File content/common/network_traffic_annotation_param_traits.h (right):

https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
content/common/network_traffic_annotation_param_traits.h:20: struct
CC_IPC_EXPORT ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
On 2017/06/02 15:06:18, yzshen1 wrote:
> Have you considered defining a mojom type and then use StructTraits, instead
of
> marking it as "Native" and use the old-style ParamTraits? We sometimes reuse
> ParamTraits from the legacy IPCs, but seldom introduce new ones.
> 
> I notice that net::PartialNetworkTrafficAnnotationTag has different fields
> depending on whether we are in debug mode. mojom type doesn't support
> preprocessor directives, so maybe it makes sense to use ParamTraits. But
please
> see my other comment below.

Acknowledged.

https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
content/common/network_traffic_annotation_param_traits.h:32:
ParamTraits<net::MutablePartialNetworkTrafficAnnotationTag> {
On 2017/06/02 15:06:18, yzshen1 wrote:
> Is this going to be used in future CLs?
> I notice net::MutablePartialNetworkTrafficAnnotationTag is not used by mojo
> interfaces.

It is not used now, and I am not sure when we would use it. I just thought as
they might be replaced with each other occasionally, it's better to define both.
Do you prefer if I remove this and change the other one to StructTraits?

[yzshen1, 2017-06-05 16:56:00]

On 2017/06/02 16:46:03, Ramin Halavati wrote:
> Thank you. Comments addressed, please review.
> 
>
https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
> File content/common/network_traffic_annotation_param_traits.h (right):
> 
>
https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
> content/common/network_traffic_annotation_param_traits.h:20: struct
> CC_IPC_EXPORT ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
> On 2017/06/02 15:06:18, yzshen1 wrote:
> > Have you considered defining a mojom type and then use StructTraits, instead
> of
> > marking it as "Native" and use the old-style ParamTraits? We sometimes reuse
> > ParamTraits from the legacy IPCs, but seldom introduce new ones.
> > 
> > I notice that net::PartialNetworkTrafficAnnotationTag has different fields
> > depending on whether we are in debug mode. mojom type doesn't support
> > preprocessor directives, so maybe it makes sense to use ParamTraits. But
> please
> > see my other comment below.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
> content/common/network_traffic_annotation_param_traits.h:32:
> ParamTraits<net::MutablePartialNetworkTrafficAnnotationTag> {
> On 2017/06/02 15:06:18, yzshen1 wrote:
> > Is this going to be used in future CLs?
> > I notice net::MutablePartialNetworkTrafficAnnotationTag is not used by mojo
> > interfaces.
> 
> It is not used now, and I am not sure when we would use it. I just thought as
> they might be replaced with each other occasionally, it's better to define
both.
> Do you prefer if I remove this and change the other one to StructTraits?

I lean towards that (but could be persuaded otherwise). Thanks!

[yzshen1, 2017-06-05 16:56:01]

On 2017/06/02 16:46:03, Ramin Halavati wrote:
> Thank you. Comments addressed, please review.
> 
>
https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
> File content/common/network_traffic_annotation_param_traits.h (right):
> 
>
https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
> content/common/network_traffic_annotation_param_traits.h:20: struct
> CC_IPC_EXPORT ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
> On 2017/06/02 15:06:18, yzshen1 wrote:
> > Have you considered defining a mojom type and then use StructTraits, instead
> of
> > marking it as "Native" and use the old-style ParamTraits? We sometimes reuse
> > ParamTraits from the legacy IPCs, but seldom introduce new ones.
> > 
> > I notice that net::PartialNetworkTrafficAnnotationTag has different fields
> > depending on whether we are in debug mode. mojom type doesn't support
> > preprocessor directives, so maybe it makes sense to use ParamTraits. But
> please
> > see my other comment below.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
> content/common/network_traffic_annotation_param_traits.h:32:
> ParamTraits<net::MutablePartialNetworkTrafficAnnotationTag> {
> On 2017/06/02 15:06:18, yzshen1 wrote:
> > Is this going to be used in future CLs?
> > I notice net::MutablePartialNetworkTrafficAnnotationTag is not used by mojo
> > interfaces.
> 
> It is not used now, and I am not sure when we would use it. I just thought as
> they might be replaced with each other occasionally, it's better to define
both.
> Do you prefer if I remove this and change the other one to StructTraits?

I lean towards that (but could be persuaded otherwise). Thanks!

[blundell, 2017-06-06 07:33:52]

On 2017/06/05 16:56:00, yzshen1 wrote:
> On 2017/06/02 16:46:03, Ramin Halavati wrote:
> > Thank you. Comments addressed, please review.
> > 
> >
>
https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
> > File content/common/network_traffic_annotation_param_traits.h (right):
> > 
> >
>
https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
> > content/common/network_traffic_annotation_param_traits.h:20: struct
> > CC_IPC_EXPORT ParamTraits<net::MutableNetworkTrafficAnnotationTag> {
> > On 2017/06/02 15:06:18, yzshen1 wrote:
> > > Have you considered defining a mojom type and then use StructTraits,
instead
> > of
> > > marking it as "Native" and use the old-style ParamTraits? We sometimes
reuse
> > > ParamTraits from the legacy IPCs, but seldom introduce new ones.
> > > 
> > > I notice that net::PartialNetworkTrafficAnnotationTag has different fields
> > > depending on whether we are in debug mode. mojom type doesn't support
> > > preprocessor directives, so maybe it makes sense to use ParamTraits. But
> > please
> > > see my other comment below.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/2893233002/diff/320001/content/common/network...
> > content/common/network_traffic_annotation_param_traits.h:32:
> > ParamTraits<net::MutablePartialNetworkTrafficAnnotationTag> {
> > On 2017/06/02 15:06:18, yzshen1 wrote:
> > > Is this going to be used in future CLs?
> > > I notice net::MutablePartialNetworkTrafficAnnotationTag is not used by
mojo
> > > interfaces.
> > 
> > It is not used now, and I am not sure when we would use it. I just thought
as
> > they might be replaced with each other occasionally, it's better to define
> both.
> > Do you prefer if I remove this and change the other one to StructTraits?
> 
> I lean towards that (but could be persuaded otherwise). Thanks!

+1

[Ramin Halavati, 2017-06-06 07:51:39]

Patchset #11 (id:340001) has been deleted

[Ramin Halavati, 2017-06-06 08:50:24]

Patchset #11 (id:360001) has been deleted

[Ramin Halavati, 2017-06-06 08:50:58]

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

[commit-bot: I haz the power, 2017-06-06 08:51:21]

Dry run: CQ is trying da patch.

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

[Ramin Halavati, 2017-06-06 10:25:40]

Thank you Colin, Yuzhu,

Removed partial and change ParamTraits to StructTraits.

Please review.

[commit-bot: I haz the power, 2017-06-06 10:49:47]

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

[commit-bot: I haz the power, 2017-06-06 10:49:48]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-06-06 15:54:32]

On 2017/06/06 10:25:40, Ramin Halavati wrote:
> Thank you Colin, Yuzhu,
> 
> Removed partial and change ParamTraits to StructTraits.
> 
> Please review.

LGTM

[Ramin Halavati, 2017-06-06 16:29:07]

The CQ bit was checked by rhalavati@chromium.org

[Ramin Halavati, 2017-06-06 16:29:08]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
estark@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/2893233002/#ps380001
(title: "Changed ParamTraits to StructTraits.")

[commit-bot: I haz the power, 2017-06-06 16:29:28]

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-06 16:39:01]

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

[commit-bot: I haz the power, 2017-06-06 16:39:06]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

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

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

[Ramin Halavati, 2017-06-07 05:03:50]

Thank you.

Jochen,

Please take a look, I've add annotation to CreateLoaderAndStart (through Mojo
interface), and updated the interface of all of its usages. I've added
NO_TRAFFIC_ANNOTATION_YET to every required position and will create CLs to
replace them after this one lands.

[jochen (gone - plz use gerrit), 2017-06-08 17:00:58]

https://codereview.chromium.org/2893233002/diff/380001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2893233002/diff/380001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:100:
net::MutableNetworkTrafficAnnotationTag(NO_TRAFFIC_ANNOTATION_YET));
why is this not using traffic_annotation_?

https://codereview.chromium.org/2893233002/diff/380001/content/browser/loader...
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/2893233002/diff/380001/content/browser/loader...
content/browser/loader/resource_message_filter.cc:99:
static_cast<net::NetworkTrafficAnnotationTag>(traffic_annotation));
just wondering, why doesn't this have to be const
net::NetworkTrafficAnnotationTag?

https://codereview.chromium.org/2893233002/diff/380001/content/common/mutable...
File content/common/mutable_network_traffic_annotation_tag.mojom (right):

https://codereview.chromium.org/2893233002/diff/380001/content/common/mutable...
content/common/mutable_network_traffic_annotation_tag.mojom:10: 
nit. no empty line

https://codereview.chromium.org/2893233002/diff/380001/content/common/mutable...
File content/common/mutable_network_traffic_annotation_tag.typemap (right):

https://codereview.chromium.org/2893233002/diff/380001/content/common/mutable...
content/common/mutable_network_traffic_annotation_tag.typemap:10: type_mappings
= [
"content.mojom.MutableNetworkTrafficAnnotationTag=net::MutableNetworkTrafficAnnotationTag"
]
is there a styleguide for typemaps, e.g. deps seems to be wrapped, while
type_mappings is not?

https://codereview.chromium.org/2893233002/diff/380001/net/traffic_annotation...
File net/traffic_annotation/network_traffic_annotation.h (right):

https://codereview.chromium.org/2893233002/diff/380001/net/traffic_annotation...
net/traffic_annotation/network_traffic_annotation.h:217: #if defined(_DEBUG) ||
defined(DCHECK_ALWAYS_ON)
why _DEBUG?

[Ramin Halavati, 2017-06-09 07:05:28]

Patchset #13 (id:420001) has been deleted

[Ramin Halavati, 2017-06-09 07:08:53]

Thank Jochen,

All comments addressed, please review.

https://codereview.chromium.org/2893233002/diff/380001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2893233002/diff/380001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:100:
net::MutableNetworkTrafficAnnotationTag(NO_TRAFFIC_ANNOTATION_YET));
On 2017/06/08 17:00:58, jochen wrote:
> why is this not using traffic_annotation_?

Done.

https://codereview.chromium.org/2893233002/diff/380001/content/browser/loader...
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/2893233002/diff/380001/content/browser/loader...
content/browser/loader/resource_message_filter.cc:99:
static_cast<net::NetworkTrafficAnnotationTag>(traffic_annotation));
On 2017/06/08 17:00:58, jochen wrote:
> just wondering, why doesn't this have to be const
> net::NetworkTrafficAnnotationTag?

Done.

https://codereview.chromium.org/2893233002/diff/380001/content/common/mutable...
File content/common/mutable_network_traffic_annotation_tag.mojom (right):

https://codereview.chromium.org/2893233002/diff/380001/content/common/mutable...
content/common/mutable_network_traffic_annotation_tag.mojom:10: 
On 2017/06/08 17:00:58, jochen wrote:
> nit. no empty line

Done.

https://codereview.chromium.org/2893233002/diff/380001/content/common/mutable...
File content/common/mutable_network_traffic_annotation_tag.typemap (right):

https://codereview.chromium.org/2893233002/diff/380001/content/common/mutable...
content/common/mutable_network_traffic_annotation_tag.typemap:10: type_mappings
= [
"content.mojom.MutableNetworkTrafficAnnotationTag=net::MutableNetworkTrafficAnnotationTag"
]
On 2017/06/08 17:00:58, jochen wrote:
> is there a styleguide for typemaps, e.g. deps seems to be wrapped, while
> type_mappings is not?

I didn't find anything in style guides, but in this sample it is not wrapped:
https://cs.chromium.org/chromium/src/gpu/ipc/common/gpu_info.typemap

Also 'git cl format' doesn't allow moving the long strings of lines 6 and 10 to
next line.

https://codereview.chromium.org/2893233002/diff/380001/net/traffic_annotation...
File net/traffic_annotation/network_traffic_annotation.h (right):

https://codereview.chromium.org/2893233002/diff/380001/net/traffic_annotation...
net/traffic_annotation/network_traffic_annotation.h:217: #if defined(_DEBUG) ||
defined(DCHECK_ALWAYS_ON)
On 2017/06/08 17:00:58, jochen wrote:
> why _DEBUG? 

Replaced it with '#ifndef NDEBUG' for all cases.

[Ramin Halavati, 2017-06-09 07:09:04]

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

[commit-bot: I haz the power, 2017-06-09 07:09:18]

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-06-09 08:57:51]

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

[commit-bot: I haz the power, 2017-06-09 08:57:52]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-06-12 08:34:25]

lgtm with comment

https://codereview.chromium.org/2893233002/diff/440001/net/traffic_annotation...
File net/traffic_annotation/network_traffic_annotation.h (right):

https://codereview.chromium.org/2893233002/diff/440001/net/traffic_annotation...
net/traffic_annotation/network_traffic_annotation.h:41: #ifndef NDEBUG
can you keep the DCHECK_ALWAYS_ON check? i.e.

!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)

[Ramin Halavati, 2017-06-12 09:04:55]

Thank you Jochen, comment addressed, landing.

https://codereview.chromium.org/2893233002/diff/440001/net/traffic_annotation...
File net/traffic_annotation/network_traffic_annotation.h (right):

https://codereview.chromium.org/2893233002/diff/440001/net/traffic_annotation...
net/traffic_annotation/network_traffic_annotation.h:41: #ifndef NDEBUG
On 2017/06/12 08:34:25, jochen wrote:
> can you keep the DCHECK_ALWAYS_ON check? i.e.
> 
> !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)

Done.

[Ramin Halavati, 2017-06-12 09:05:04]

The CQ bit was checked by rhalavati@chromium.org

[Ramin Halavati, 2017-06-12 09:05:05]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
estark@chromium.org, battre@chromium.org, mmenke@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2893233002/#ps460001
(title: "Comment addressed, Merged.")

[commit-bot: I haz the power, 2017-06-12 09:05:16]

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-12 11:00:29]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1497258304909490,
"parent_rev": "63a16add8253bc6a138329c648ba82a5492f11f0", "commit_rev":
"4cda417b683318c0c8ca8a0ef2a94ebc4ee5bea8"}

[commit-bot: I haz the power, 2017-06-12 11:00:41]

Description was changed from

==========
Network traffic annotation added to URLLoaderImpl.

Network traffic annotation is added to network request of:
content/network/url_loader_impl.cc

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Network traffic annotation added to URLLoaderImpl.

Network traffic annotation is added to network request of:
content/network/url_loader_impl.cc

BUG=656607
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2893233002
Cr-Commit-Position: refs/heads/master@{#478588}
Committed:
https://chromium.googlesource.com/chromium/src/+/4cda417b683318c0c8ca8a0ef2a9...
==========

[commit-bot: I haz the power, 2017-06-12 11:00:43]

Committed patchset #14 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/4cda417b683318c0c8ca8a0ef2a9...