Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(511)

Issue 630973002: Map WebURLRequest::RequestContextPlugin to RESOURCE_TYPE_OBJECT. (Closed)

Created:
6 years, 2 months ago by Mike West
Modified:
6 years ago
CC:
chromium-reviews, darin-cc_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Map WebURLRequest::RequestContextPlugin to RESOURCE_TYPE_OBJECT. The WebRequest API should treat both object requests, and requests made from those objects (e.g. Flash requesting an .mp4 resource) as type Object. We used to do this, then I broke it. :( Now I'm fixing it. :) BUG=410382 R=vabr@chromium.org Committed: https://crrev.com/c3e50e6354188999da307094c5c037d5933f1bc3 Cr-Commit-Position: refs/heads/master@{#308787}

Patch Set 1 #

Patch Set 2 : A TEST! #

Patch Set 3 : Not really a PDF. #

Patch Set 4 : Failure? #

Patch Set 5 : Rebase #

Patch Set 6 : Dropping the test. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M content/child/web_url_request_util.cc View 1 2 3 4 2 chunks +1 line, -1 line 0 comments Download

Messages

Total messages: 36 (10 generated)
Mike West
Vaclav, mind taking a look? Jochen, perhaps you could rubberstamp content/?
6 years, 2 months ago (2014-10-06 20:19:37 UTC) #2
vabr (Chromium)
LGTM, Thanks for fixing this, Mike! One thing that worries me, is that this is ...
6 years, 2 months ago (2014-10-07 07:40:15 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/630973002/1
6 years, 2 months ago (2014-10-07 07:45:29 UTC) #5
Mike West
On 2014/10/07 07:40:15, vabr (Chromium) wrote: > LGTM, Thanks for fixing this, Mike! > > ...
6 years, 2 months ago (2014-10-07 07:45:36 UTC) #6
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/15939)
6 years, 2 months ago (2014-10-07 07:54:36 UTC) #9
jochen (gone - plz use gerrit)
how about adding at least a basic test that tries to load a non-existant plugin?
6 years, 2 months ago (2014-10-07 12:05:59 UTC) #10
Mike West
jam@: This test is crashing locally at the NOTREACHED in Instance::DidOpen. Do you have any ...
6 years, 2 months ago (2014-10-10 16:01:14 UTC) #12
Mike West
jam@: See the trace at http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_dbg/builds/618/steps/browser_tests%20%28with%20patch%29/logs/WebRequestTypes
6 years, 2 months ago (2014-10-10 18:48:46 UTC) #13
jochen (gone - plz use gerrit)
maybe try an existing pdf? chrome/test/data/pdf/test.pdf for example
6 years, 2 months ago (2014-10-13 08:12:28 UTC) #14
Mike West
On 2014/10/13 08:12:28, jochen wrote: > maybe try an existing pdf? chrome/test/data/pdf/test.pdf for example It ...
6 years, 2 months ago (2014-10-13 08:13:21 UTC) #15
Mike West
Huh. That worked. I'll file a bug for the crash.
6 years, 2 months ago (2014-10-13 09:02:26 UTC) #16
jochen (gone - plz use gerrit)
lgtm
6 years, 2 months ago (2014-10-13 09:03:24 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/630973002/220001
6 years, 2 months ago (2014-10-13 09:30:33 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_swarming/builds/18458)
6 years, 2 months ago (2014-10-13 11:00:32 UTC) #21
Mike West
Hrm. This passes locally. :(
6 years, 2 months ago (2014-10-13 12:01:35 UTC) #22
jochen (gone - plz use gerrit)
maybe you need to add the files to the browser_tests.isolate?
6 years, 2 months ago (2014-10-14 13:34:56 UTC) #23
Mike West
On 2014/10/14 13:34:56, jochen wrote: > maybe you need to add the files to the ...
6 years, 2 months ago (2014-10-14 13:42:53 UTC) #24
Mike West
On 2014/10/14 13:42:53, Mike West wrote: > On 2014/10/14 13:34:56, jochen wrote: > > maybe ...
6 years, 2 months ago (2014-10-14 13:56:12 UTC) #25
jam
On 2014/10/14 13:56:12, Mike West wrote: > On 2014/10/14 13:42:53, Mike West wrote: > > ...
6 years, 2 months ago (2014-10-15 16:08:28 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/630973002/580001
6 years ago (2014-11-25 14:45:33 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/2136)
6 years ago (2014-11-25 15:41:41 UTC) #30
dbdaniel42
Any reason the fix hasn't been pushed through by now? It's fine if the test ...
6 years ago (2014-12-04 02:51:52 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/630973002/600001
6 years ago (2014-12-17 11:36:35 UTC) #33
commit-bot: I haz the power
Committed patchset #6 (id:600001)
6 years ago (2014-12-17 12:31:10 UTC) #34
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/c3e50e6354188999da307094c5c037d5933f1bc3 Cr-Commit-Position: refs/heads/master@{#308787}
6 years ago (2014-12-17 12:32:48 UTC) #35
jianli
6 years ago (2014-12-17 19:15:09 UTC) #36
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:600001) has been created in
https://codereview.chromium.org/809103003/ by jianli@chromium.org.

The reason for reverting is: Speculative revert since it might cause browser
test PDFExtensionTest.BasicPlugin to fail.

http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests%20(dbg)/bu...
http://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/1...
http://build.chromium.org/p/chromium.webkit/builders/Win7%20Tests/builds/7530
.

Powered by Google App Engine
This is Rietveld 408576698