Do not cancel request when an error occurs in a webRequest event handler

Do not cancel request when an error occurs in a webRequest event handler

BUG=355236
TEST=browser_tests: ExtensionWebRequestApiTest.WebRequestApi:ExtensionWebRequestApiTest.WebRequestBlocking

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259751

[robwu, 2014-03-26 11:26:30]

Changes:
- Renamed CancelWithError to RespondWithError
- Removed {cancel: true} from webRequest event handler
- Modify the response_headers / request_headers only after passing all
validation.
- Updated tests to match new expectations

I also added some TODOs in the test. I'm thinking of modifying framework.js to
include something like:

(function(consoleError) {
    console.error = function(msg) {
        lastError = msg;
        consoleError(msg);
    };
})(console.error);

and change expect() to include a fake event type, "javascript-error", so we can
test for the existence of expected messages.
WDYT?

[battre, 2014-03-26 15:38:22]

On 2014/03/26 11:26:30, robwu wrote:
> Changes:
> - Renamed CancelWithError to RespondWithError
> - Removed {cancel: true} from webRequest event handler
> - Modify the response_headers / request_headers only after passing all
> validation.
> - Updated tests to match new expectations

LGTM. Thanks for considering these cleanups as well and not changing the
webRequest event handler.

> I also added some TODOs in the test. I'm thinking of modifying framework.js to
> include something like:
> 
> (function(consoleError) {
>     console.error = function(msg) {
>         lastError = msg;
>         consoleError(msg);
>     };
> })(console.error);
> 
> and change expect() to include a fake event type, "javascript-error", so we
can
> test for the existence of expected messages.
> WDYT?

I think it would be great to test the existence of expected messages. It is not
100% clear to me how you are planning to do it, but if you have something in
mind, I'd be happy, again, to review it.

Best regards,
Dominic

[battre, 2014-03-26 15:38:57]

+kalman for OWNERs review of 
chrome/renderer/resources/extensions/web_request_internal_custom_bindings.js

[not at google - send to devlin, 2014-03-26 16:12:15]

lgtm

[battre, 2014-03-26 17:06:55]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:08:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob@robwu.nl/212693002/1

[commit-bot: I haz the power, 2014-03-26 19:21:52]

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

[commit-bot: I haz the power, 2014-03-26 19:21:53]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[battre, 2014-03-26 20:55:11]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2014-03-26 20:57:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob@robwu.nl/212693002/1

[commit-bot: I haz the power, 2014-03-27 00:10:59]

Change committed as 259751