Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 9 months ago by KhNo Modified:
6 years, 9 months ago Reviewers:
jamesr, Raymond Toy, Ken Russell (switch to Gerrit) CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRemove unnecassary parameter "sampleRate" for loadAudioResource().
Parameter "sampleRate" is not required for loadAudioResource().
There is FIXME in AudioBus.cpp in blink
FIXME: the sampleRate parameter is ignored. It should be removed from the API.
It will be consist of 3 steps.
1. Removing "sampleRate" in chrome which is called by blink. (included deprecated method)
https://codereview.chromium.org/195043002/
2. Removing "sampleRate" in blink.
https://codereview.chromium.org/194753005/
3. Removing deprecated method from #1
https://codereview.chromium.org/199573004/
BUG=351284
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256800
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #
Created: 6 years, 9 months ago
Messages
Total messages: 35 (0 generated)
Please review this patch.
You didn't say what other patch, but for the record it is https://codereview.chromium.org/194753005/. Why are these separate patches? Why not combine them into one?
On 2014/03/11 16:49:00, Raymond Toy wrote: > You didn't say what other patch, but for the record it is > https://codereview.chromium.org/194753005/. > > Why are these separate patches? Why not combine them into one? Git path was different. Is there the way can upload togather?
On 2014/03/11 16:54:10, KhNo wrote: > On 2014/03/11 16:49:00, Raymond Toy wrote: > > You didn't say what other patch, but for the record it is > > https://codereview.chromium.org/194753005/. > > > > Why are these separate patches? Why not combine them into one? > > Git path was different. Is there the way can upload togather? Sorry, my fault. One is in blink and the other in chrome, so they have to be different. So this patch needs to be done first?
In the description, I think a link to the other patch is useful. A comment on which needs to be applied first is useful. https://codereview.chromium.org/195043002/diff/1/content/renderer/renderer_we... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/195043002/diff/1/content/renderer/renderer_we... content/renderer/renderer_webkitplatformsupport_impl.cc:797: size_t data_size) { Can you make sample_rate an optional parameter instead of adding a new deprecated method?
On 2014/03/11 17:20:05, Raymond Toy wrote: > On 2014/03/11 16:54:10, KhNo wrote: > > On 2014/03/11 16:49:00, Raymond Toy wrote: > > > You didn't say what other patch, but for the record it is > > > https://codereview.chromium.org/194753005/. > > > > > > Why are these separate patches? Why not combine them into one? > > > > Git path was different. Is there the way can upload togather? > > Sorry, my fault. One is in blink and the other in chrome, so they have to be > different. > > So this patch needs to be done first? Yes, It is the first. Thanks for adding dependency codereview url. Final goal is removing "sampleRate" parameter. It will be consist of 3 steps. 1. Removing "sampleRate" in chrome which is called by blink. (included deprecated method) 2. Removing "sampleRate" in blink. 3. Removing deprecated method from #1
On 2014/03/12 01:13:22, KhNo wrote: > On 2014/03/11 17:20:05, Raymond Toy wrote: > > On 2014/03/11 16:54:10, KhNo wrote: > > > On 2014/03/11 16:49:00, Raymond Toy wrote: > > > > You didn't say what other patch, but for the record it is > > > > https://codereview.chromium.org/194753005/. > > > > > > > > Why are these separate patches? Why not combine them into one? > > > > > > Git path was different. Is there the way can upload togather? > > > > Sorry, my fault. One is in blink and the other in chrome, so they have to be > > different. > > > > So this patch needs to be done first? > Yes, It is the first. Thanks for adding dependency codereview url. Final goal is removing "sampleRate" parameter. It will be consist of 3 steps. 1. Removing "sampleRate" in chrome which is called by blink. (included deprecated method) https://codereview.chromium.org/195043002/ 2. Removing "sampleRate" in blink. https://codereview.chromium.org/194753005/ 3. Removing deprecated method from #1
https://codereview.chromium.org/195043002/diff/1/content/renderer/renderer_we... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/195043002/diff/1/content/renderer/renderer_we... content/renderer/renderer_webkitplatformsupport_impl.cc:797: size_t data_size) { On 2014/03/11 18:03:07, Raymond Toy wrote: > Can you make sample_rate an optional parameter instead of adding a new > deprecated method? Actually I tried, but there are definations of loadAudioResource in both side. public/platform/Platform.h ( blink) virtual bool loadAudioResource(WebAudioBus* destinationBus, const char* audioFileData, size_t dataSize) { return false; } content/renderer/renderer_webkitplatformsupport_impl.h ( chrome ) virtual bool loadAudioResource( blink::WebAudioBus* destination_bus, const char* audio_file_data, size_t data_size, double sample_rate); I can not do it. :(
lgtm
On 2014/03/12 02:42:41, Raymond Toy wrote: > lgtm Ken Russell, James, Can I get OWNER's LGTM?
I'm not an OWNER in any of these directories. Please consult the OWNERS files in them to try to find an appropriate reviewer.
lgtm
The CQ bit was checked by keonho07.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/195043002/1
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The PRESUBMIT says: ** Presubmit Warnings ** keonho07.kim@samsung.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. I do see keonho07.kim@samsung.com in our CLA signers list, so you'll just have to add yourself to AUTHORS
The CQ bit was checked by keonho07.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/195043002/20001
On 2014/03/12 22:34:20, jamesr wrote: > The PRESUBMIT says: > > ** Presubmit Warnings ** > mailto:keonho07.kim@samsung.com is not in AUTHORS file. If you are a new contributor, > please visit > http://www.chromium.org/developers/contributing-code and read the "Legal" > section > If you are a chromite, verify the contributor signed the CLA. > > I do see mailto:keonho07.kim@samsung.com in our CLA signers list, so you'll just have > to add yourself to AUTHORS Thanks for kind your help. :)
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
The CQ bit was checked by keonho07.kim@samsung.com
The CQ bit was unchecked by keonho07.kim@samsung.com
The CQ bit was checked by keonho07.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/195043002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
On 2014/03/13 05:51:30, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/keonho07.kim%40samsung.com/195043002/4... Check the failure from the android_clang_dbg bot: http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui... This looks like a real failure that you need to fix.
The CQ bit was checked by keonho07.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/195043002/40001
Message was sent while issue was closed.
Change committed as 256800
Message was sent while issue was closed.
On 2014/03/13 05:59:35, Raymond Toy wrote: > On 2014/03/13 05:51:30, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > > https://chromium-status.appspot.com/cq/keonho07.kim%2540samsung.com/195043002... > > Check the failure from the android_clang_dbg bot: > http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui... > > This looks like a real failure that you need to fix. Sorry, make you concern. I missed "git add audio_decoder_android.h" when I uploaded the fist patch set. :( That was fixed and merged. I will keep upload next steps.
Message was sent while issue was closed.
On 2014/03/13 17:04:48, KhNo wrote: > On 2014/03/13 05:59:35, Raymond Toy wrote: > > On 2014/03/13 05:51:30, I haz the power (commit-bot) wrote: > > > CQ is trying da patch. Follow status at > > > > > > https://chromium-status.appspot.com/cq/keonho07.kim%252540samsung.com/1950430... > > > > Check the failure from the android_clang_dbg bot: > > > http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui... > > > > This looks like a real failure that you need to fix. > > Sorry, make you concern. I missed "git add audio_decoder_android.h" when I > uploaded the fist patch set. :( > That was fixed and merged. I will keep upload next steps. In general that probably should have required a further review, but I see the change was very simple. You may want to use "git cl try" next time to make sure things work, especially if you're making android changes and you don't build on android yourself.
Message was sent while issue was closed.
On 2014/03/13 18:01:45, Raymond Toy wrote: > On 2014/03/13 17:04:48, KhNo wrote: > > On 2014/03/13 05:59:35, Raymond Toy wrote: > > > On 2014/03/13 05:51:30, I haz the power (commit-bot) wrote: > > > > CQ is trying da patch. Follow status at > > > > > > > > > > https://chromium-status.appspot.com/cq/keonho07.kim%25252540samsung.com/19504... > > > > > > Check the failure from the android_clang_dbg bot: > > > > > > http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui... > > > > > > This looks like a real failure that you need to fix. > > > > Sorry, make you concern. I missed "git add audio_decoder_android.h" when I > > uploaded the fist patch set. :( > > That was fixed and merged. I will keep upload next steps. > > In general that probably should have required a further review, but I see the > change was very simple. > > You may want to use "git cl try" next time to make sure things work, especially > if you're making android changes and you don't build on android yourself. Oh, It is very nice. Thanks information, Now, I'm reading http://www.chromium.org/developers/testing/try-server-usage |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
