WebRequest API cleanup

WebRequest API cleanup

Cleanup changes:
- Moved the event dictionary generation logic to a new class
  (WebRequestEventDetails), to deduplicate logic and make it easier to
  manipulate the event details.
- Remove some unused aliases, forward-declarations and includes.
- Fixed a few typos.
- Moved ExtraInfoSpec to web_request_api_helpers.
- Remove ExtractRequestInfoDetails.
- Solved TODOs from https://codereview.chromium.org/1413853005/
- Avoid unnecessary copying of event details (request body and headers,
  response headers). Previously these dictionaries were always copied
  and removed later, which is wasteful.

Visible changes:
- When a request has no response headers, the status code defaults to
  -1 (from URLRequestJob) instead of a dummy value of 200. This affects
  the webRequest.onResponseStarted and webRequest.onCompleted events for
  file: and ftp: requests. The new behavior is consistent with the
  webRequest.onBeforeRedirect event (which also reports -1 for unknown
  statuses).
- requestBody is only included if set in extraInfoSpec (bug 542719).

BUG=432875, 542719
TEST=./browser_tests --gtest_filter=ExtensionWebRequestApiTest.*
     ./unit_tests --gtest_filter=ExtensionWebRequestTest.*

Committed: https://crrev.com/3eb7a397449be2eda5f8ad5ad4436ad8377563c6
Cr-Commit-Position: refs/heads/master@{#368979}

[robwu, 2016-01-10 23:56:08]

Patchset #1 (id:1) has been deleted

[robwu, 2016-01-10 23:56:15]

Patchset #1 (id:20001) has been deleted

[robwu, 2016-01-11 00:00:54]

rob@robwu.nl changed reviewers:
+ battre@chromium.org

[robwu, 2016-01-11 00:00:55]

Hi Dominic, PTAL.

Here is a brief overview of diff (see CL description for more info):

- The new test (ExtensionWebRequestTest.MinimalAccessRequestBodyData) is a
regression test for crbug.com/542719

- ExtensionWebRequestEventRouter::ExtraInfoSpec was moved to
web_request_api_helpers.{h,cc} to allow web_request_event_details.cc to use it
without introducing a cyclic dependency between web_request_api and
web_request_event_details.

- Most of the deleted logic in web_request_api moved to WebRequestEventDetail
(in a compressed form, because previously some logic was duplicated up to 5
times, e.g. setting the status line).

[battre, 2016-01-12 13:08:03]

LGTM, nice!

https://codereview.chromium.org/1577673002/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.h (right):

https://codereview.chromium.org/1577673002/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.h:78: void
SetBoolean(const std::string& key, bool value) {
I think the style guide still asks to put the body into the .cc file, doesn't
it?

[robwu, 2016-01-12 16:33:05]

https://codereview.chromium.org/1577673002/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.h (right):

https://codereview.chromium.org/1577673002/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.h:78: void
SetBoolean(const std::string& key, bool value) {
On 2016/01/12 13:08:03, battre wrote:
> I think the style guide still asks to put the body into the .cc file, doesn't
> it?

Trivial methods may be inlined. Since these SetBoolean methods are just aliases,
I inlined them.

https://google.github.io/styleguide/cppguide.html#Declaration_Order
https://google.github.io/styleguide/cppguide.html#Inline_Functions

I considered inheriting from DictionaryValue or making the internal dict public,
but then callers could erase keys or copy the result in an incorrect way, which
is something that I wanted to avoid.

[battre, 2016-01-12 19:05:21]

https://codereview.chromium.org/1577673002/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.h (right):

https://codereview.chromium.org/1577673002/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.h:78: void
SetBoolean(const std::string& key, bool value) {
On 2016/01/12 16:33:05, robwu wrote:
> On 2016/01/12 13:08:03, battre wrote:
> > I think the style guide still asks to put the body into the .cc file,
doesn't
> > it?
> 
> Trivial methods may be inlined. Since these SetBoolean methods are just
aliases,
> I inlined them.
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order
> https://google.github.io/styleguide/cppguide.html#Inline_Functions
> 
> I considered inheriting from DictionaryValue or making the internal dict
public,
> but then callers could erase keys or copy the result in an incorrect way,
which
> is something that I wanted to avoid.

Ok, SGTM to leave it as is.

[robwu, 2016-01-12 19:56:39]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-01-12 19:57:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577673002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577673002/60001

[commit-bot: I haz the power, 2016-01-12 20:06:58]

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

[commit-bot: I haz the power, 2016-01-12 20:06:59]

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

[robwu, 2016-01-12 20:10:10]

rob@robwu.nl changed reviewers:
+ rockot@chromium.org

[robwu, 2016-01-12 20:10:10]

Ken, can you give a rubber stamp for extensions/browser?

[Ken Rockot(use gerrit already), 2016-01-12 20:12:46]

rs lgtm

[robwu, 2016-01-12 20:13:30]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-01-12 20:14:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577673002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577673002/60001

[commit-bot: I haz the power, 2016-01-12 20:48:47]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-01-12 20:50:00]

Description was changed from

==========
WebRequest API cleanup

Cleanup changes:
- Moved the event dictionary generation logic to a new class
  (WebRequestEventDetails), to deduplicate logic and make it easier to
  manipulate the event details.
- Remove some unused aliases, forward-declarations and includes.
- Fixed a few typos.
- Moved ExtraInfoSpec to web_request_api_helpers.
- Remove ExtractRequestInfoDetails.
- Solved TODOs from https://codereview.chromium.org/1413853005/
- Avoid unnecessary copying of event details (request body and headers,
  response headers). Previously these dictionaries were always copied
  and removed later, which is wasteful.

Visible changes:
- When a request has no response headers, the status code defaults to
  -1 (from URLRequestJob) instead of a dummy value of 200. This affects
  the webRequest.onResponseStarted and webRequest.onCompleted events for
  file: and ftp: requests. The new behavior is consistent with the
  webRequest.onBeforeRedirect event (which also reports -1 for unknown
  statuses).
- requestBody is only included if set in extraInfoSpec (bug 542719).

BUG=432875,542719
TEST=./browser_tests --gtest_filter=ExtensionWebRequestApiTest.*
     ./unit_tests --gtest_filter=ExtensionWebRequestTest.*
==========

to

==========
WebRequest API cleanup

Cleanup changes:
- Moved the event dictionary generation logic to a new class
  (WebRequestEventDetails), to deduplicate logic and make it easier to
  manipulate the event details.
- Remove some unused aliases, forward-declarations and includes.
- Fixed a few typos.
- Moved ExtraInfoSpec to web_request_api_helpers.
- Remove ExtractRequestInfoDetails.
- Solved TODOs from https://codereview.chromium.org/1413853005/
- Avoid unnecessary copying of event details (request body and headers,
  response headers). Previously these dictionaries were always copied
  and removed later, which is wasteful.

Visible changes:
- When a request has no response headers, the status code defaults to
  -1 (from URLRequestJob) instead of a dummy value of 200. This affects
  the webRequest.onResponseStarted and webRequest.onCompleted events for
  file: and ftp: requests. The new behavior is consistent with the
  webRequest.onBeforeRedirect event (which also reports -1 for unknown
  statuses).
- requestBody is only included if set in extraInfoSpec (bug 542719).

BUG=432875,542719
TEST=./browser_tests --gtest_filter=ExtensionWebRequestApiTest.*
     ./unit_tests --gtest_filter=ExtensionWebRequestTest.*

Committed: https://crrev.com/3eb7a397449be2eda5f8ad5ad4436ad8377563c6
Cr-Commit-Position: refs/heads/master@{#368979}
==========

[commit-bot: I haz the power, 2016-01-12 20:50:01]

Patchset 2 (id:??) landed as
https://crrev.com/3eb7a397449be2eda5f8ad5ad4436ad8377563c6
Cr-Commit-Position: refs/heads/master@{#368979}