Extracting NetLog inner classes into their own classes.

Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.

4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
TBR=droger@chromium.org, hirono@chromium.org, rdevlin.cronin@chromium.org, skyostil@chromium.org, wez@chromium.org, juliatuttle@chromium.org, sergeyu@chromium.org, dgozman@chromium.org, hubbe@chromium.org, rdsmith@chromium.org, stevenjb@chromium.org, scheib@chromium.org, bengr@chromium.org, alokp@chromium.org, raymes@chromium.org, samuong@chromium.org, creis@chromium.org, scottbyer@chromium.org, zea@chromium.org, mmenke@chromium.org, eranm@chromium.org

Committed: https://crrev.com/f22f981b847bc8cf67e3165b47a3891c6066e70f
Cr-Commit-Position: refs/heads/master@{#422691}

[mikecirone, 2016-09-29 03:24:44]

Description was changed from

==========
Trying out new structuring for extraction of NetLog's inner classes.

BUG=473710
==========

to

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need to
include "net_log.h"; these files are no longer susceptible to having their
includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog, extracted and
NetLogCaptureMode classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

**Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

BUG=473710
==========

[mikecirone, 2016-09-29 03:26:03]

Description was changed from

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need to
include "net_log.h"; these files are no longer susceptible to having their
includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog, extracted and
NetLogCaptureMode classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

**Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

BUG=473710
==========

to

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  extracted and NetLogCaptureMode classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

BUG=473710
==========

[mikecirone, 2016-09-29 03:27:03]

Description was changed from

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  extracted and NetLogCaptureMode classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

BUG=473710
==========

to

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

BUG=473710
==========

[mikecirone, 2016-09-29 03:43:48]

Description was changed from

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

BUG=473710
==========

to

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
==========

[mikecirone, 2016-09-29 03:51:17]

mikecirone@gmail.com changed reviewers:
+ eroman@chromium.org, tfarina@chromium.org

[mikecirone, 2016-09-29 03:51:19]

The "Extracting NetLog inner classes into their own classes" work is ready for
review!

[mmenke, 2016-09-29 03:54:08]

On 2016/09/29 03:51:19, mikecirone wrote:
> The "Extracting NetLog inner classes into their own classes" work is ready for
> review!

Thanks so much for doing this!  I've run into more include-what-you-use issues
when removing net_log.h includes than any other file.

[eroman, 2016-09-29 04:50:44]

Thanks Mike!

I look forward to reviewing this, but probably won't get around to it until
Friday or Monday.

Cheers.

[mikecirone, 2016-09-29 05:29:45]

You bet!, glad to hear this will help out.

[mikecirone, 2016-09-29 05:32:06]

On 2016/09/29 04:50:44, eroman wrote:
> I look forward to reviewing this, but probably won't get around to it until
> Friday or Monday.

Ok, cool!

[mikecirone, 2016-09-30 06:12:25]

I decided to try out compiling/running tests on my Linux and Windows machines,
to preempt try bot errors on Linux/Windows-compile-only parts. (Everything
already compiled and the net unit tests passed on my Mac machine.)

Here's my report so far:

Linux:
- Chrome and all unit tests compile, after some fixes which are patch set 3.
- Ran all unit tests and 4 tests do not pass. 3 of 4 tests come from
*_unittest.cc files that I touched. Perhaps these tests are failing due to this
cl, although its hard to see how the refactoring would do that. I did not check
to see if the Linux build tree was all green last I rebase-update'd.

Windows:
- Chrome does not compile because of an error not related to this cl. The build
tree for Windows is now green, so looks like I'll have a little better luck tmw
in getting Chrome to compile and encountering any compilation errors because of
this cl ( e.g. - running into include-hiding).
- Compiled net unit tests. 2 failed but did not come from *_unittest.cc file
that I touched.

So, that is the down low.

Eric,
I leave it to your discretion to review when you were thinking you could get to
it, or perhaps later when I have my ducks a bit more in a row, now that I'm
compiling/testing for Linux/Windows.

[tfarina, 2016-09-30 19:45:56]

I will try to look at some files later tomorrow.

As just a note, I would have separated this in three patches, one for each pair
of .h/.cc you are extracting. That would make it smaller and probably easier to
spot build/test failures sooner.

[tfarina, 2016-09-30 19:46:43]

Description was changed from

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
==========

to

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
==========

[eroman, 2016-09-30 20:11:58]

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

[commit-bot: I haz the power, 2016-09-30 20:12:25]

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, 2016-09-30 20:36:05]

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

[commit-bot: I haz the power, 2016-09-30 20:36:07]

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

[mikecirone, 2016-09-30 21:27:40]

On 2016/09/30 19:45:56, tfarina wrote:
> I will try to look at some files later tomorrow.

Thanks!
 
> As just a note, I would have separated this in three patches, one for each
pair
> of .h/.cc you are extracting. That would make it smaller and probably easier
to
> spot build/test failures sooner.

Ok.

[mikecirone, 2016-10-01 05:49:56]

Just uploaded fixes for the compilation errors from the trybots.

One dealt with a file that needed the definition for NetLogWithSource by way of
NetLogWithSource&() ctor.
I found some other instances like this in codebase and updated.

Another dealt with std::string being include-hidden by "net_log.h".
I went through codebase somewhat extensively, found other same situations and
inserted <string> headers appropriately. I'm not trying to be a huge
"string-include" marshal but am also trying to pre-empt any further trybot
compilation errors due to <string> being include-hidden by "net_log.h".

[tfarina, 2016-10-03 15:29:11]

Changes to the files I looked at look fine.

Note for the comments below: they are intended for the line below them.

https://codereview.chromium.org/2333923004/diff/80001/net/dns/address_sorter_...
File net/dns/address_sorter_posix_unittest.cc (right):

https://codereview.chromium.org/2333923004/diff/80001/net/dns/address_sorter_...
net/dns/address_sorter_posix_unittest.cc:12: #include
"net/base/test_completion_callback.h"
Since the parameter is unused and it is a reference, can you forward declare
instead?

https://codereview.chromium.org/2333923004/diff/80001/net/dns/dns_transaction...
File net/dns/dns_transaction_unittest.cc (right):

https://codereview.chromium.org/2333923004/diff/80001/net/dns/dns_transaction...
net/dns/dns_transaction_unittest.cc:26: #include "net/dns/dns_util.h"
Can you foward declare? The parameter looks unused.

[mikecirone, 2016-10-03 17:33:29]

https://codereview.chromium.org/2333923004/diff/80001/net/dns/address_sorter_...
File net/dns/address_sorter_posix_unittest.cc (right):

https://codereview.chromium.org/2333923004/diff/80001/net/dns/address_sorter_...
net/dns/address_sorter_posix_unittest.cc:12: #include
"net/base/test_completion_callback.h"
On 2016/10/03 15:29:11, tfarina wrote:
> Since the parameter is unused and it is a reference, can you forward declare
> instead?

There is a NetLogWithSource instance on line 92.

https://codereview.chromium.org/2333923004/diff/80001/net/dns/dns_transaction...
File net/dns/dns_transaction_unittest.cc (right):

https://codereview.chromium.org/2333923004/diff/80001/net/dns/dns_transaction...
net/dns/dns_transaction_unittest.cc:26: #include "net/dns/dns_util.h"
On 2016/10/03 15:29:11, tfarina wrote:
> Can you foward declare? The parameter looks unused.

The include is needed. I spotted a NetLogWithSource ctor call on line 259.

[eroman, 2016-10-03 18:25:51]

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

[commit-bot: I haz the power, 2016-10-03 18:26:19]

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, 2016-10-03 18:33:32]

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

[commit-bot: I haz the power, 2016-10-03 18:33:34]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_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_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mikecirone, 2016-10-03 20:18:13]

Trybots failed because of merge conflicts in files committed after git
rebase-update earlier this morning.

I just rebase-updated again and handled the merge conflicts.

We should be good to go for tryserver dry run.

[eroman, 2016-10-03 20:23:40]

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

[commit-bot: I haz the power, 2016-10-03 20:24:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2016-10-03 21:40:51]

patchset 7 LGTM !

We can TBR the owners for stuff outside of //net

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_entry.h
File net/log/net_log_entry.h (right):

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_entry....
net/log/net_log_entry.h:40: // observe
join this line

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_entry....
net/log/net_log_entry.h:56: // Returns the parameters as a Value.  Returns NULL
if there are no
optional: change to "nullptr" while editing this.

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_parame...
File net/log/net_log_parameters_callback_typedef.h (right):

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_parame...
net/log/net_log_parameters_callback_typedef.h:5: #ifndef
NET_LOG_NET_LOG_PARAMETERS_CALLBACK_TYPEDEF_H_
nit on naming: I suggest dropping "_typedef" from the filename.
"typedef" reflects the implementation, which could just as easily be "using"
nowadays.

Feel free to leave as is and we can always discuss this change as a follow-up.

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_parame...
net/log/net_log_parameters_callback_typedef.h:22: // not at all.  May return
NULL.
optional: Can you change NULL --> nullptr while editing this?

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_source.h
File net/log/net_log_source.h (right):

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_source...
net/log/net_log_source.h:5: #ifndef NET_LOG_SOURCE_H_
NET_LOG_NET_LOG_SOURCE_H_

[eroman, 2016-10-03 21:50:59]

Description was changed from

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
==========

to

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
TBR=stevenjb@chromium.org,jam@chromium.org
==========

[eroman, 2016-10-03 21:53:24]

eroman@chromium.org changed reviewers:
+ jam@chromium.org, stevenjb@chromium.org

[eroman, 2016-10-03 21:53:27]

TBR stevenjb + jam (this CL does renames affecting many files)

stevenjb:
  chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc
  chromeos/dbus/services/proxy_resolution_service_provider.cc
  chromeos/network/host_resolver_impl_chromeos_unittest.cc
jam:
  Everything else outside of //net

[commit-bot: I haz the power, 2016-10-03 22:14:07]

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

[commit-bot: I haz the power, 2016-10-03 22:14:09]

Dry run: This issue passed the CQ dry run.

[mikecirone, 2016-10-03 23:38:25]

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_entry.h
File net/log/net_log_entry.h (right):

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_entry....
net/log/net_log_entry.h:40: // observe
On 2016/10/03 21:40:50, eroman wrote:
> join this line

Done.

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_entry....
net/log/net_log_entry.h:56: // Returns the parameters as a Value.  Returns NULL
if there are no
On 2016/10/03 21:40:50, eroman wrote:
> optional: change to "nullptr" while editing this.

Done.

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_parame...
File net/log/net_log_parameters_callback_typedef.h (right):

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_parame...
net/log/net_log_parameters_callback_typedef.h:5: #ifndef
NET_LOG_NET_LOG_PARAMETERS_CALLBACK_TYPEDEF_H_
On 2016/10/03 21:40:50, eroman wrote:
> nit on naming: I suggest dropping "_typedef" from the filename.
> "typedef" reflects the implementation, which could just as easily be "using"
> nowadays.
> 
> Feel free to leave as is and we can always discuss this change as a follow-up.

"net_log_parameters_callback_typedef.h" is now "net_log_parameters_callback.h"
and codebase refactored to this filename change. Change is in patch 8!

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_parame...
net/log/net_log_parameters_callback_typedef.h:22: // not at all.  May return
NULL.
On 2016/10/03 21:40:50, eroman wrote:
> optional: Can you change NULL --> nullptr while editing this?

Done.

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_source.h
File net/log/net_log_source.h (right):

https://codereview.chromium.org/2333923004/diff/120001/net/log/net_log_source...
net/log/net_log_source.h:5: #ifndef NET_LOG_SOURCE_H_
On 2016/10/03 21:40:51, eroman wrote:
> NET_LOG_NET_LOG_SOURCE_H_

Done, and I also added same "NET_LOG_" prefix for "net_log_with_source.h" and
"net_log_entry.h" since they had same issue.

[eroman, 2016-10-04 00:27:57]

patchset 8 LGTM, thanks!

[eroman, 2016-10-04 00:28:15]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-10-04 00:29:31]

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

[jam, 2016-10-04 00:32:30]

On 2016/10/03 21:53:27, eroman wrote:
> TBR stevenjb + jam (this CL does renames affecting many files)
> 
> stevenjb:
>   chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc
>   chromeos/dbus/services/proxy_resolution_service_provider.cc
>   chromeos/network/host_resolver_impl_chromeos_unittest.cc
> jam:
>   Everything else outside of //net

this is a lot of files, even though simple. please TBR to different owners of
top level directories

[eroman, 2016-10-04 00:38:03]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2016-10-04 00:38:18]

@jam: understood, I will update the TBRs accordingly.

[eroman, 2016-10-04 00:55:20]

Description was changed from

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
TBR=stevenjb@chromium.org,jam@chromium.org
==========

to

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
TBR=droger@chromium.org, hirono@chromium.org, rdevlin.cronin@chromium.org,
skyostil@chromium.org, wez@chromium.org, juliatuttle@chromium.org,
sergeyu@chromium.org, dgozman@chromium.org, hubbe@chromium.org,
rdsmith@chromium.org, stevenjb@chromium.org, scheib@chromium.org,
bengr@chromium.org, alokp@chromium.org, raymes@chromium.org,
samuong@chromium.org, creis@chromium.org, scottbyer@chromium.org,
zea@chromium.org, mmenke@chromium.org, eranm@chromium.org
==========

[eroman, 2016-10-04 00:55:20]

eroman@chromium.org changed reviewers:
+ alokp@chromium.org, bengr@chromium.org, creis@chromium.org,
dgozman@chromium.org, droger@chromium.org, eranm@chromium.org,
hirono@chromium.org, hubbe@chromium.org, juliatuttle@chromium.org,
mmenke@chromium.org, raymes@chromium.org, rdevlin.cronin@chromium.org,
rdsmith@chromium.org, samuong@chromium.org, scheib@chromium.org,
scottbyer@chromium.org, sergeyu@chromium.org, skyostil@chromium.org,
wez@chromium.org, zea@chromium.org

[eroman, 2016-10-04 01:03:21]

TBR:

droger:
  ios/chrome/browser/net/ios_chrome_network_delegate.cc
hirono:
  components/drive/service/fake_drive_service.h
rdevlin.cronin:
  chrome/browser/extensions/api/*
  extensions/browser/*
skyostil:
  headless/lib/browser/headless_devtools.cc
wez:
  blimp/net/*
  extensions/browser/api/cast_channel/*
  remoting/*
juliatuttle:
  components/domain_reliability/uploader_unittest.cc
  components/network_hints/browser/network_hints_message_filter.cc
sergeyu:
  chrome/browser/media/wv_test_license_server_config.cc
  content/browser/renderer_host/p2p/*
  jingle/glue/*
  remoting/base/vlog_net_log.cc
  remoting/protocol/*
dgozman:
  chrome/browser/devtools/*
  content/browser/devtools/*
hubbe:
  chrome/browser/extensions/api/cast_streaming/*
  chrome/browser/media/*
  content/browser/media/*
  media/cast/*
rdsmith:
  chrome/browser/loader/safe_browsing_resource_throttle.cc
  chrome/browser/loader/safe_browsing_resource_throttle.h
  components/domain_reliability/uploader_unittest.cc
  content/browser/download/*
  content/browser/loader/*
stevenjb:
  chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc
  chromeos/dbus/services/proxy_resolution_service_provider.cc
  chromeos/network/host_resolver_impl_chromeos_unittest.cc
scheib:
  device/bluetooth/*
bengr:
  components/data_reduction_proxy/*
alokp:
  chromecast/browser/devtools/remote_debugging_server.cc
raymes:
  content/browser/renderer_host/pepper/*
samuong:
  chrome/test/chromedriver/*
creis:
  content/* (not covered by others)
scottbyer:
  chrome/browser/printing/cloud_print/privet_traffic_detector.cc
zea:
  google_apis/gcm/*
  jingle/glue/chrome_async_socket_unittest.cc
  jingle/glue/fake_ssl_client_socket_unittest.cc
  jingle/glue/proxy_resolving_client_socket.h
mmenke:
  chrome/browser/loader/*
  chrome/browser/loader/*
  components/cronet/*
  content/browser/loader/*
eranm:
  components/certificate_transparency/log_dns_client.h
  components/certificate_transparency/log_dns_client_unittest.cc

[eroman, 2016-10-04 01:04:15]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-10-04 01:05:09]

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

[scheib, 2016-10-04 02:04:40]

LGTM

[commit-bot: I haz the power, 2016-10-04 03:40:48]

Description was changed from

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
TBR=droger@chromium.org, hirono@chromium.org, rdevlin.cronin@chromium.org,
skyostil@chromium.org, wez@chromium.org, juliatuttle@chromium.org,
sergeyu@chromium.org, dgozman@chromium.org, hubbe@chromium.org,
rdsmith@chromium.org, stevenjb@chromium.org, scheib@chromium.org,
bengr@chromium.org, alokp@chromium.org, raymes@chromium.org,
samuong@chromium.org, creis@chromium.org, scottbyer@chromium.org,
zea@chromium.org, mmenke@chromium.org, eranm@chromium.org
==========

to

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
TBR=droger@chromium.org, hirono@chromium.org, rdevlin.cronin@chromium.org,
skyostil@chromium.org, wez@chromium.org, juliatuttle@chromium.org,
sergeyu@chromium.org, dgozman@chromium.org, hubbe@chromium.org,
rdsmith@chromium.org, stevenjb@chromium.org, scheib@chromium.org,
bengr@chromium.org, alokp@chromium.org, raymes@chromium.org,
samuong@chromium.org, creis@chromium.org, scottbyer@chromium.org,
zea@chromium.org, mmenke@chromium.org, eranm@chromium.org
==========

[commit-bot: I haz the power, 2016-10-04 03:40:51]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-04 03:43:50]

Description was changed from

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.
   
4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
TBR=droger@chromium.org, hirono@chromium.org, rdevlin.cronin@chromium.org,
skyostil@chromium.org, wez@chromium.org, juliatuttle@chromium.org,
sergeyu@chromium.org, dgozman@chromium.org, hubbe@chromium.org,
rdsmith@chromium.org, stevenjb@chromium.org, scheib@chromium.org,
bengr@chromium.org, alokp@chromium.org, raymes@chromium.org,
samuong@chromium.org, creis@chromium.org, scottbyer@chromium.org,
zea@chromium.org, mmenke@chromium.org, eranm@chromium.org
==========

to

==========
Main benefits of NetLog inner classes extraction:
- Greater code organization.
- Promotion of files being "include-what-you-use" clean. Less files now need
  to include "net_log.h"; these files are no longer susceptible to having their
  includes hidden by the many inner includes of "net_log.h".
- All touched files are also now forward-clean re: NetLog,
  NetLogCaptureMode and the new, extracted classes.

Here is the new structuring:
  - NetLogSource (net_log_source.h/.cc)
  - NetLogEntry, NetLogEntryData (net_log_entry.h/.cc)
  - NetLogWithSource (net_log_with_source.h/.cc)

***Did not extract NetLog::ThreadSafeObserver into its own class since
   NetLog::ThreadSafeObserver and NetLog are interrelated enough to keep
   NetLog::ThreadSafeObserver as an inner class.

Further Notes:
1) On top of scripting, all files were checked manually, ensuring:
     - Forwards/including added only when appropriate. No extras added.
     - "net_log.h" include removals done correctly. No "net_log.h"-hiding
       to occur as a result of new changes.

2) Just as any .cc file will not do the include/forward if the paired .h file
   already has the include/forward, *_unittests.cc, *_browsertest.cc and
   *_test.cc files exhibit the same behavior when they have a paired .h file.

3) Override methods' types do not add to logic for inserting forwards, since it
   is guaranteed that the base class has the forward.
     - Saw this done in codebase several times, so making sure to
       follow the same rule.

4) A big change was the mass adjustment for the include-hiding of
   NetLogCaptureMode by net_log.h includes. Now the NetLogCaptureMode class is
   being properly forwarded/included in all files changed by the original
   NetLog inner classes extraction work.

5) Many NetLog/NetLogWithSource includes/forwards were added across codebase.
   All instances of files seeing their NetLog/NetLogWithSource include/forward
   being hidden by their own includes should now be gone. This is to promote
   greater flexibility in future changes upon the codebase.

BUG=473710
R=eroman@chromium.org,tfarina@chromium.org
TBR=droger@chromium.org, hirono@chromium.org, rdevlin.cronin@chromium.org,
skyostil@chromium.org, wez@chromium.org, juliatuttle@chromium.org,
sergeyu@chromium.org, dgozman@chromium.org, hubbe@chromium.org,
rdsmith@chromium.org, stevenjb@chromium.org, scheib@chromium.org,
bengr@chromium.org, alokp@chromium.org, raymes@chromium.org,
samuong@chromium.org, creis@chromium.org, scottbyer@chromium.org,
zea@chromium.org, mmenke@chromium.org, eranm@chromium.org

Committed: https://crrev.com/f22f981b847bc8cf67e3165b47a3891c6066e70f
Cr-Commit-Position: refs/heads/master@{#422691}
==========

[commit-bot: I haz the power, 2016-10-04 03:43:53]

Patchset 8 (id:??) landed as
https://crrev.com/f22f981b847bc8cf67e3165b47a3891c6066e70f
Cr-Commit-Position: refs/heads/master@{#422691}

[Randy Smith (Not in Mondays), 2016-10-04 12:15:33]

chrome/browser/loader/safe_browsing_resource_throttle.cc
chrome/browser/loader/safe_browsing_resource_throttle.h
components/domain_reliability/uploader_unittest.cc
content/browser/download/*
content/browser/loader/*
LGTM.

[Devlin, 2016-10-04 15:03:06]

lgtm with minor nit to be fixed in a followup

https://codereview.chromium.org/2333923004/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/log_private/log_private_api.h (right):

https://codereview.chromium.org/2333923004/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/log_private/log_private_api.h:93: void
OnAddEntry(const net::NetLogEntry& entry) override;
forward declaration needed

[mikecirone, 2016-10-04 17:38:35]

https://codereview.chromium.org/2333923004/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/log_private/log_private_api.h (right):

https://codereview.chromium.org/2333923004/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/log_private/log_private_api.h:93: void
OnAddEntry(const net::NetLogEntry& entry) override;
On 2016/10/04 15:03:06, Devlin wrote:
> forward declaration needed

I don't think the forward is needed because the override aspect guarantees that
the class is included/forwarded from the base class's file.

I was finding this forwarding behavior in files I touched, like
fuzzed_socket_factory.h:
https://cs.chromium.org/chromium/src/net/socket/fuzzed_socket_factory.h?gsn=t...

The FuzzedSocketFactory::CreateTransportClientSocket override method has an
AddressList ref and SocketPerformanceWatcher ptr parameters. Both AddressList
and SocketPerformanceWatcher are not included/forwarded in the file, but they
are included/forwarded in the base file, client_socket_factory.h.

[Devlin, 2016-10-04 17:59:22]

https://codereview.chromium.org/2333923004/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/log_private/log_private_api.h (right):

https://codereview.chromium.org/2333923004/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/log_private/log_private_api.h:93: void
OnAddEntry(const net::NetLogEntry& entry) override;
On 2016/10/04 17:38:35, mikecirone wrote:
> On 2016/10/04 15:03:06, Devlin wrote:
> > forward declaration needed
> 
> I don't think the forward is needed because the override aspect guarantees
that
> the class is included/forwarded from the base class's file.
> 
> I was finding this forwarding behavior in files I touched, like
> fuzzed_socket_factory.h:
>
https://cs.chromium.org/chromium/src/net/socket/fuzzed_socket_factory.h?gsn=t...
> 
> The FuzzedSocketFactory::CreateTransportClientSocket override method has an
> AddressList ref and SocketPerformanceWatcher ptr parameters. Both AddressList
> and SocketPerformanceWatcher are not included/forwarded in the file, but they
> are included/forwarded in the base file, client_socket_factory.h.

Ah, missed that this was an override.  Fair enough. :)

[raymes, 2016-10-05 03:12:47]

pepper lgtm

[hirono, 2016-10-05 03:33:20]

components/drive/service/fake_drive_service.h lgtm

[Eran Messeri, 2016-10-05 13:28:19]

components/certificate_transparency lgtm

[Sami, 2016-10-05 15:31:18]

headless/ lgtm.

[Charlie Reis, 2016-10-07 06:13:16]

content/ LGTM