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

Issue 467173002: Allowing metadata to be read from blob urls (Closed)

Created:
6 years, 4 months ago by amogh.bihani
Modified:
6 years, 3 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, xhwang
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Allowing metadata to be read from blob urls Blob data is stored in the browser cache. This patch adds cache path to MediaResourceGetter so that it can read the metadata from the blob. BUG=227476 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289868

Patch Set 1 #

Total comments: 1

Patch Set 2 : Adding null check #

Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -0 lines) Patch
M content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java View 1 3 chunks +5 lines, -0 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/MediaResourceGetterTest.java View 1 1 chunk +40 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
amogh.bihani
PTAL. I tested on the URL mentioned in the bug.http://jsbin.com/oxarap/2 The audio doesn't work in ...
6 years, 4 months ago (2014-08-13 12:58:16 UTC) #1
xhwang
I am not familiar with this code. Add qinmin as the reviewer. qinmin: PTAL
6 years, 4 months ago (2014-08-13 15:54:53 UTC) #2
qinmin
lgtm % comment https://codereview.chromium.org/467173002/diff/1/content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/467173002/diff/1/content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java#newcode350 content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:350: result.add("/data/data/" + PACKAGE_NAME + "/cache/"); add ...
6 years, 4 months ago (2014-08-13 17:33:59 UTC) #3
amogh.bihani
+r: Avi@ OWNER's review please. :)
6 years, 4 months ago (2014-08-14 09:19:48 UTC) #4
Avi (use Gerrit)
lgtm stampity stamp
6 years, 4 months ago (2014-08-14 14:47:35 UTC) #5
amogh.bihani
On 2014/08/14 14:47:35, Avi wrote: > lgtm > > stampity stamp Thanks for the reviews ...
6 years, 4 months ago (2014-08-15 02:50:06 UTC) #6
amogh.bihani
The CQ bit was checked by amogh.bihani@samsung.com
6 years, 4 months ago (2014-08-15 02:50:10 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/467173002/20001
6 years, 4 months ago (2014-08-15 02:51:16 UTC) #8
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu ...
6 years, 4 months ago (2014-08-15 04:23:39 UTC) #9
commit-bot: I haz the power
Committed patchset #2 (20001) as 289868
6 years, 4 months ago (2014-08-15 15:29:01 UTC) #10
Andrew Hayden (chromium.org)
6 years, 3 months ago (2014-09-11 09:15:21 UTC) #11
Message was sent while issue was closed.
On 2014/08/15 15:29:01, I haz the power (commit-bot) wrote:
> Committed patchset #2 (20001) as 289868

For posterity, I do not believe this should have gone through as-is. The static
PACKAGE_NAME variable being modified by each request means that independent
requests may potentially interfere with each other, which is at *best* a benign
error that works by accident (because context.PackageName is almost certainly
going to return the same string every time). In the future, please do not do
this; either pass the Context object where it needs to go, or add some kind of
init() function which clearly and unambiguously gets called exactly one time and
sets whatever "permanent" state variables are required.

https://codereview.chromium.org/561743003/ fixes this as a side effect of
removing the hard-coding of /data/data.

Powered by Google App Engine
This is Rietveld 408576698