Bug 68079

Summary: Set but unused variables cleanup in v8 bindings (gcc 4.6)
Product: WebKit Reporter: mperepelitsyn
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

mperepelitsyn
Reported 2011-09-14 07:04:43 PDT
Set but unused variables cleanup in v8 bindings (gcc 4.6)
Attachments
Patch (4.50 KB, patch)
2011-09-14 09:02 PDT, mperepelitsyn
no flags
Patch (4.08 KB, patch)
2011-09-16 02:13 PDT, mperepelitsyn
no flags
Patch (7.13 KB, patch)
2011-09-26 05:31 PDT, mperepelitsyn
no flags
Patch (9.80 KB, patch)
2011-09-26 06:55 PDT, mperepelitsyn
no flags
mperepelitsyn
Comment 1 2011-09-14 09:02:19 PDT
WebKit Review Bot
Comment 2 2011-09-14 10:17:54 PDT
Comment on attachment 107336 [details] Patch Attachment 107336 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9661524
Adam Barth
Comment 3 2011-09-14 11:59:44 PDT
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?
mperepelitsyn
Comment 4 2011-09-14 22:57:09 PDT
> > 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?
Adam Barth
Comment 5 2011-09-15 11:15:08 PDT
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.
mperepelitsyn
Comment 6 2011-09-16 02:13:43 PDT
mperepelitsyn
Comment 7 2011-09-16 02:15:51 PDT
Got it, PTAL.
Adam Barth
Comment 8 2011-09-16 12:08:35 PDT
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.
mperepelitsyn
Comment 9 2011-09-19 08:41:09 PDT
>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.
Adam Barth
Comment 10 2011-09-19 10:54:15 PDT
> 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.
mperepelitsyn
Comment 11 2011-09-26 05:31:17 PDT
mperepelitsyn
Comment 12 2011-09-26 05:40:09 PDT
> > 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.
mperepelitsyn
Comment 13 2011-09-26 06:55:53 PDT
Adam Barth
Comment 14 2011-09-26 11:53:39 PDT
Comment on attachment 108661 [details] Patch Looks good. Thanks for iterating on this patch!
WebKit Review Bot
Comment 15 2011-09-26 16:20:48 PDT
Comment on attachment 108661 [details] Patch Clearing flags on attachment: 108661 Committed r96030: <http://trac.webkit.org/changeset/96030>
WebKit Review Bot
Comment 16 2011-09-26 16:20:53 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.