Bug 68079 - Set but unused variables cleanup in v8 bindings (gcc 4.6)
Summary: Set but unused variables cleanup in v8 bindings (gcc 4.6)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-14 07:04 PDT by ph34r
Modified: 2011-09-26 16:20 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.50 KB, patch)
2011-09-14 09:02 PDT, ph34r
no flags Details | Formatted Diff | Diff
Patch (4.08 KB, patch)
2011-09-16 02:13 PDT, ph34r
no flags Details | Formatted Diff | Diff
Patch (7.13 KB, patch)
2011-09-26 05:31 PDT, ph34r
no flags Details | Formatted Diff | Diff
Patch (9.80 KB, patch)
2011-09-26 06:55 PDT, ph34r
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ph34r 2011-09-14 07:04:43 PDT
Set but unused variables cleanup in v8 bindings (gcc 4.6)
Comment 1 ph34r 2011-09-14 09:02:19 PDT
Created attachment 107336 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Adam Barth 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?
Comment 4 ph34r 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?
Comment 5 Adam Barth 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.
Comment 6 ph34r 2011-09-16 02:13:43 PDT
Created attachment 107626 [details]
Patch
Comment 7 ph34r 2011-09-16 02:15:51 PDT
Got it, PTAL.
Comment 8 Adam Barth 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.
Comment 9 ph34r 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.
Comment 10 Adam Barth 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.
Comment 11 ph34r 2011-09-26 05:31:17 PDT
Created attachment 108655 [details]
Patch
Comment 12 ph34r 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.
Comment 13 ph34r 2011-09-26 06:55:53 PDT
Created attachment 108661 [details]
Patch
Comment 14 Adam Barth 2011-09-26 11:53:39 PDT
Comment on attachment 108661 [details]
Patch

Looks good.  Thanks for iterating on this patch!
Comment 15 WebKit Review Bot 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>
Comment 16 WebKit Review Bot 2011-09-26 16:20:53 PDT
All reviewed patches have been landed.  Closing bug.