Add bufferSize argument of socket.read() and socket.recvFrom()

Add bufferSize argument of socket.read() and socket.recvFrom()


BUG=None
TEST=browser_tests --gtest_filter=SocketApiTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136040

[Peng, 2012-05-08 18:17:02]

Hi Mike,

I propose to add bufferSize argument to socket.read() and socker.recvFrom(). So
API users can choice right buffer size for different net applications. I think
it is more flexible than hardcoded buffer size. What do you think?

[miket_OOO, 2012-05-08 18:29:43]

For the proposed change, the implementation is fine. But would it make more
sense to (1) make this an optional argument for people who don't care about the
size, and (2) put it into an options dictionary so that we won't need to keep
changing the signature in the future?

If we had an optional options structure, then (for example) this change wouldn't
break the various existing apps out there. I know we're still experimental, but
if it's possible to avoid changing the signature, that's a good thing.

[Peng, 2012-05-08 19:36:55]

On 2012/05/08 18:29:43, miket wrote:
> For the proposed change, the implementation is fine. But would it make more
> sense to (1) make this an optional argument for people who don't care about
the
> size, and (2) put it into an options dictionary so that we won't need to keep
> changing the signature in the future?
> 
> If we had an optional options structure, then (for example) this change
wouldn't
> break the various existing apps out there. I know we're still experimental,
but
> if it's possible to avoid changing the signature, that's a good thing.

I updated the CL to make bufferSize optional. But I don't put it in options
dictionary. Because bufferSize is very common argument for read and recvFrom.
Putting it into an options dictionary looks very weird to me. And though it
changes the signature, the existing apps don't need any change.

Please review it again. Thanks.

[miket_OOO, 2012-05-08 19:43:27]

lgtm

https://chromiumcodereview.appspot.com/10387021/diff/1006/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://chromiumcodereview.appspot.com/10387021/diff/1006/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket_api.cc:149: buffer_size_ =
params->buffer_size.get() ? *params->buffer_size : 4096;
Thanks for switching over to new-style params.

[commit-bot: I haz the power, 2012-05-08 19:44:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10387021/1006

[commit-bot: I haz the power, 2012-05-09 00:51:33]

Try job failure for 10387021-1006 (retry) (retry) on win_rel for step "update".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-09 14:33:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10387021/9004

[commit-bot: I haz the power, 2012-05-09 15:58:32]

Change committed as 136040