Set but unused variables cleanup in v8 bindings (gcc 4.6)
Created attachment 107336 [details] Patch
Comment on attachment 107336 [details] Patch Attachment 107336 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9661524
Comment on attachment 107336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107336&action=review > Source/WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp:105 > imp->setLength(value->Uint32Value(), ec); Are you sure we're not supposed to use newLength there? > Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:137 > - v8::Handle<v8::String> stringMessage = message->ToString(); > + message->ToString(); What's the point of calling ToString on this object and ignoring the result?
> > Source/WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp:105 > > imp->setLength(value->Uint32Value(), ec); > > Are you sure we're not supposed to use newLength there? Of course I'm not sure, as I'm not so familiar with the codebase :) My only goal was to fix about 500+ warnings that pops up during Chromium build. But as far as I could see there was no intended use of this variable, so why don't we add extra variables when it will be really needed? > > Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:137 > > - v8::Handle<v8::String> stringMessage = message->ToString(); > > + message->ToString(); > > What's the point of calling ToString on this object and ignoring the result? I thought the point was in the exception checking, as it was surrounded by "try catch" Any suggestions?
Comment on attachment 107336 [details] Patch You've definitely found parts of the code that have problems, but I don't think you've resolved them in quite the right way. For example, result = webSocket->send(toWebCoreString(message), ec) shouldn't call toString again. It should use the value stored in stringMessage.
Created attachment 107626 [details] Patch
Got it, PTAL.
Comment on attachment 107626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107626&action=review > Source/WebCore/bindings/v8/WorkerScriptDebugServer.cpp:-93 > WorkerContextExecutionProxy* proxy = workerContext->script()->proxy(); > if (!proxy) > return; > - v8::Handle<v8::Context> context = proxy->context(); Looks like proxy is almost entirely unused now. Maybe add a comment asking whether we should remove it? > Source/WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp:112 > - imp->setLength(value->Uint32Value(), ec); > + imp->setLength(newLength, ec); Can we write a test for this change? Maybe passing a length greater than UINT_MAX? > Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:140 > - result = webSocket->send(toWebCoreString(message), ec); > + result = webSocket->send(toWebCoreString(stringMessage), ec); This should also be testable by checking whether the toString method is called on message twice.
>Can we write a test for this change? Oh, I'd be happy to write them, but have no clue where to get started. I haven't found any common place in the source code where unit tests are supposed to be. I'll be very appreciated if you give me some tips.
> Oh, I'd be happy to write them, but have no clue where to get started. I haven't found any common place in the source code where unit tests are supposed to be. Almost all the tests are in the LayoutTests directory. They aren't unit tests, but rather medium-sized end-to-end tests. > I'll be very appreciated if you give me some tips. Here's some documentation about writing tests: http://www.webkit.org/quality/testing.html http://www.webkit.org/quality/testwriting.html For the WebSocket change, you'll probably want to add your test somewhere in this directory: http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/websocket/tests For the HTMLOptionsCollection, I'm not entirely sure which tests cover that object, but you can probably find some by looking at the history of the file. That should give you some inspiration about how and where to add the tests.
Created attachment 108655 [details] Patch
> > Source/WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp:112 > > - imp->setLength(value->Uint32Value(), ec); > > + imp->setLength(newLength, ec); > > Can we write a test for this change? Maybe passing a length greater than UINT_MAX? I think there is no point, because the length is limited to 10k ( http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/HTMLSelectElement.cpp&type=cs&l=479 ) and must have been covered by tests somewhere :) I've uploaded a new patch, take a look please.
Created attachment 108661 [details] Patch
Comment on attachment 108661 [details] Patch Looks good. Thanks for iterating on this patch!
Comment on attachment 108661 [details] Patch Clearing flags on attachment: 108661 Committed r96030: <http://trac.webkit.org/changeset/96030>
All reviewed patches have been landed. Closing bug.