[webkit-reviews] review granted: [Bug 68684] spec change - option.label should be reflected like option.value : [Attachment 109285] updated patch - Incorporating review comments related to white space stripping and fixing styling errors

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sachin Puranik
<jcqt43 at motorola.com>'s request for review:
Bug 68684: spec change - option.label should be reflected like option.value
https://bugs.webkit.org/show_bug.cgi?id=68684

Attachment 109285: updated patch - Incorporating review comments related to
white space stripping and fixing styling errors
https://bugs.webkit.org/attachment.cgi?id=109285&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=109285&action=review


commit-queue- because the comments still need a little work and the copyrights
are a bit wrong too

> Source/WebCore/dom/OptionElement.cpp:3
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.


Should not add copyright since you are just re-ordering includes and removing a
space in a comment.

> Source/WebCore/dom/OptionElement.h:3
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.


Doesn’t make sense to add copyright just for moving private down a line.

> Source/WebCore/html/HTMLOptionElement.cpp:8
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.


Would be better if your copyright matched the format of the others. Also, 2011
is the year of publication here, not 2010. Copyright should cite year of
publication.

> Source/WebCore/html/HTMLOptionElement.cpp:225
> +    // Use the text if the Label wasn't set. Also strip leading and trailing
html whitespace.

This comment says what we are doing, but not why. Comments need to say why we
do things, not just repeat what the code does and say what we are doing.

Also, "label" should not be capitalized, "HTML" should be capitalized.

> Source/WebCore/html/HTMLOptionElement.cpp:228
> +    // we want to collapse our html whitespace within the text too.

This comment says what we are doing, but not why. Comments need to say why we
do things, not just repeat what the code does and say what we are doing.

Also, "We" should be capitalized and "HTML" should be capitalized.

> Source/WebCore/html/HTMLOptionElement.h:7
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.


The year is 2011. Copyright dates are based on year of publication. Also I’m
not sure that just adding a single-line function declaration justifies adding a
copyright line here.