webRequest: Also identify frames for non-RDH-initiated requests

webRequest: Also identify frames for non-RDH-initiated requests

BUG=633737
TEST=./browser_tests --gtest_filter=ExtensionWebRequestApiTest.WebRequestTestOSDD

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

[robwu, 2016-08-13 11:15:20]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 11:15:33]

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

[robwu, 2016-08-13 11:17:28]

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

[robwu, 2016-08-13 11:17:28]

Dominic, PTAL.

I'll add an owner for the extensions/ stamp after passing your review.

[commit-bot: I haz the power, 2016-08-13 12:01:11]

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

[commit-bot: I haz the power, 2016-08-13 12:01:12]

Dry run: This issue passed the CQ dry run.

[robwu, 2016-08-14 02:54:37]

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

[robwu, 2016-08-14 02:54:37]

Dominic is OOO, could you review this Marijn?

[Marijn Kruisselbrink, 2016-08-15 22:02:15]

https://codereview.chromium.org/2238383003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2238383003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:164:
IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest, WebRequestTestOSDD) {
Maybe a comment mentioning what OSDD means and/or what this test tests here
would make sense, as I don't expect everybody who deals with extensions and the
webrequest API to also know what that abbreviation stands for.

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/framework.js:207: if
(details.type == "other" && !details.url.includes('dont-ignore-me') ||
nit: I think this would be clearer with an extra pair of parentheses

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webrequest/test_osdd.js (right):

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/test_osdd.js:7: // OSDD are only
loaded from "/".
Ah, is that why there is a ServeFilesFromDirectory call in the c++ code for this
test? Might be helpful to duplicate this comment there.

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/test_osdd.js:12: // Referenced
by |getPageWithOSDDURL()|, the response is not import
This comment seems to have been cut off?

Also I got a bit confused by the comment claiming that the getPageWithOSDDURL
method references this, while it actually is the document at the url returned by
that method which references the URL. But maybe that's just me.

[robwu, 2016-08-17 08:44:32]

https://codereview.chromium.org/2238383003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2238383003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:164:
IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest, WebRequestTestOSDD) {
On 2016/08/15 22:02:15, Marijn Kruisselbrink wrote:
> Maybe a comment mentioning what OSDD means and/or what this test tests here
> would make sense, as I don't expect everybody who deals with extensions and
the
> webrequest API to also know what that abbreviation stands for.

Done.

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/framework.js:207: if
(details.type == "other" && !details.url.includes('dont-ignore-me') ||
On 2016/08/15 22:02:15, Marijn Kruisselbrink wrote:
> nit: I think this would be clearer with an extra pair of parentheses

Done.

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webrequest/test_osdd.js (right):

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/test_osdd.js:7: // OSDD are only
loaded from "/".
On 2016/08/15 22:02:15, Marijn Kruisselbrink wrote:
> Ah, is that why there is a ServeFilesFromDirectory call in the c++ code for
this
> test? Might be helpful to duplicate this comment there.

Done.

https://codereview.chromium.org/2238383003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/test_osdd.js:12: // Referenced
by |getPageWithOSDDURL()|, the response is not import
On 2016/08/15 22:02:15, Marijn Kruisselbrink wrote:
> This comment seems to have been cut off?
> 
> Also I got a bit confused by the comment claiming that the getPageWithOSDDURL
> method references this, while it actually is the document at the url returned
by
> that method which references the URL. But maybe that's just me.

Done.

[Marijn Kruisselbrink, 2016-08-17 18:13:46]

thanks, lgtm

[robwu, 2016-08-18 08:25:26]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-08-18 08:25:41]

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-08-18 09:02:57]

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

[commit-bot: I haz the power, 2016-08-18 09:02:58]

Exceeded global retry quota

[robwu, 2016-08-24 06:48:44]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-08-24 06:49:15]

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-08-24 08:11:18]

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

[commit-bot: I haz the power, 2016-08-24 08:11:19]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[robwu, 2016-08-24 08:54:38]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-08-24 08:55:08]

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-08-24 09:35:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-24 09:37:35]

Description was changed from

==========
webRequest: Also identify frames for non-RDH-initiated requests

BUG=633737
TEST=./browser_tests
--gtest_filter=ExtensionWebRequestApiTest.WebRequestTestOSDD
==========

to

==========
webRequest: Also identify frames for non-RDH-initiated requests

BUG=633737
TEST=./browser_tests
--gtest_filter=ExtensionWebRequestApiTest.WebRequestTestOSDD

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

[commit-bot: I haz the power, 2016-08-24 09:37:36]

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