[webkit-reviews] review denied: [Bug 110521] Move setAutofilled from TestRunner to WebCore : [Attachment 189674] To move the setAutofilled from TestRunner to WebCore

Thu Feb 21 23:08:40 PST 2013

Benjamin Poulain <benjamin at webkit.org> has denied Jason Anderssen
<janderssen at exactal.com>'s request for review:
Bug 110521: Move setAutofilled from TestRunner to WebCore
https://bugs.webkit.org/show_bug.cgi?id=110521

Attachment 189674: To move the setAutofilled from TestRunner to WebCore
https://bugs.webkit.org/attachment.cgi?id=189674&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=189674&action=review

Congrats on making your first WebKit patch.

It looks like many files that should be left unmodified have been modified. I
suspect there is something up with the line endings.
You should revert those files and find what is modifying them on your system.

> Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherResource.h:-2
> -//{{NO_DEPENDENCIES}}
> -// Microsoft Visual C++ generated include file.

This file should not be modified.

> Tools/WinLauncher/WinLauncherLauncherResource.h:-2
> -//{{NO_DEPENDENCIES}}
> -// Microsoft Visual C++ generated include file.

Ditto.

> Source/WebCore/testing/Internals.cpp:819
> +    if (!element->hasTagName(inputTag)) {
> +	   ec = INVALID_ACCESS_ERR;
> +	   return;
> +    }

You don't need this part of the check. Node::toInputElement is polymorphic and
will return null for anything that is not an input element.

> Source/WebCore/testing/Internals.cpp:825
> +    inputElement->setAutofilled( enabled );

To follow the WebKit style, you should not have spaces around the argument.

> Source/WebCore/testing/Internals.h:212
> +    void setAutofilled(Element* element, bool enabled, ExceptionCode&);
> +

I would put this with the other functions acting on <input>: setSuggestedValue,
setEditingValue, etc.

The first parameter would probably better be named "inputElement".

> Source/WebCore/testing/Internals.idl:255
> +    
> +    void setAutofilled(in Element scope, in boolean enabled)
raises(DOMException);

Ditto.
"scope" is probably worse.

Keeping the style consistent, you need a space between raises and the exception
type.

> Source/WebCore/ChangeLog:6
> +	   Move setAutofilled from TestRunner to WebCore
> +	   https://bugs.webkit.org/show_bug.cgi?id=110521
> +
> +	   Reviewed by NOBODY (OOPS!).

In this changelog, you should put a word of description about the reason of the
change. See other patches to get a feeling of what kind of information is
important.

> Source/WebCore/ChangeLog:8
> +	   No new tests (OOPS!).

This line should always be removed.

Usually, you replace it by the list of tests covering the change. In this case,
there is no additional test, we are working on test, so you can just delete it.

> Source/ThirdParty/gtest/codegear/gtest_all.cc:-2
> -// Copyright 2009, Google Inc.
> -// All rights reserved.

This file should not be modified.

> Source/ThirdParty/gtest/codegear/gtest_link.cc:-2
> -// Copyright 2009, Google Inc.
> -// All rights reserved.

Ditto.

> Source/ThirdParty/ChangeLog:2
> +2013-02-21  Jason Anderssen	<janderssen at exactal.com>
> +

You can clear this ChangeLog once you revert the file to the original.

> LayoutTests/storage/indexeddb/set_version_blocked.html:-2
> -<html>
> -<head>

This should not be modified.

> LayoutTests/storage/indexeddb/transaction-read-only.html:-2
> -<html>
> -<head>

This should not be modified.

> LayoutTests/css2.1/20110323/support/eof-green.css:-2
> -/* eof-green.css */
> -div

This should not be modified.

> LayoutTests/css2.1/20110323/support/at-import-001.css:-2
> -div
> -{

This should not be modified.

> LayoutTests/css2.1/20110323/support/at-import-002.css:-2
> -div
> -{

This should not be modified.

> LayoutTests/css2.1/20110323/support/at-import-004.css:-2
> -div
> -{

This should not be modified.

> LayoutTests/css2.1/20110323/support/at-import-005.css:-2
> -div
> -{

This should not be modified.

> LayoutTests/css2.1/20110323/support/at-import-006.css:-2
> -div
> -{

This should not be modified.

> LayoutTests/css2.1/20110323/support/at-import-007.css:-2
> -div
> -{

This should not be modified.

> LayoutTests/inspector/storage-panel-dom-storage-update.html:-2
> -<html>
> -<head>

This should not be modified.

> LayoutTests/compositing/repaint/shrink-layer.html:-2
> -<html>
> -<head>

This should not be modified.