Bug 68079 – Set but unused variables cleanup in v8 bindings (gcc 4.6)

• Component: New Bugs
• Status: RESOLVED FIXED

Set but unused variables cleanup in v8 bindings (gcc 4.6)

Created attachment 107336 [details]
Patch

[WebKit Review Bot]

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 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?

[Adam Barth]

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.

[Adam Barth]

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.

[Adam Barth]

> 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

[Adam Barth]

Comment on attachment 108661 [details]
Patch

Looks good.  Thanks for iterating on this patch!

[WebKit Review Bot]

Comment on attachment 108661 [details]
Patch

Clearing flags on attachment: 108661

Committed r96030: <http://trac.webkit.org/changeset/96030>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.