[webkit-reviews] review granted: [Bug 39697] Make more HTML DOM members private, especially constructors : [Attachment 57053] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 39697: Make more HTML DOM members private, especially constructors
https://bugs.webkit.org/show_bug.cgi?id=39697

Attachment 57053: Patch
https://bugs.webkit.org/attachment.cgi?id=57053&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> Index: WebCore/ChangeLog
> ===================================================================
> +	   Made constructors and virtual function overrides private, added
create functions.
> +	   Made constructors inline in casese where they were called in only
one place.

typo: casese


> Index: WebCore/html/HTMLAudioElement.cpp  
> ===================================================================
> +PassRefPtr<HTMLAudioElement> HTMLAudioElement::create(const QualifiedName&
tagName, Document* document)
> +{
> +    return new HTMLAudioElement(tagName, document);

Every time I see this pattern it bothers me. I want it to be "return
adoptRef(new HTMLAudioElement(tagName, document));"


> Index: WebCore/html/HTMLButtonElement.h
> ===================================================================
> -    HTMLButtonElement(const QualifiedName&, Document*, HTMLFormElement* =
0);
> +    static PassRefPtr<HTMLButtonElement> create(const QualifiedName&,
Document*, HTMLFormElement*);

I guess the default value for HTMLFormElement* was never used.


> Index: WebCore/html/HTMLDataGridCellElement.cpp
> ===================================================================
> +HTMLDataGridCellElement::HTMLDataGridCellElement(const QualifiedName& name,
Document* document)

Consider adding "inline" here, since the constructor is only called from one
place.


> Index: WebCore/html/HTMLDataGridColElement.cpp
> ===================================================================
> +HTMLDataGridColElement::HTMLDataGridColElement(const QualifiedName& name,
Document* document)

Add "inline"?


> Index: WebCore/html/HTMLDataGridElement.cpp
> ===================================================================
> @@ -45,6 +45,11 @@ HTMLDataGridElement::HTMLDataGridElement

Add inline to constructor?



> Index: WebCore/html/HTMLDataGridRowElement.cpp
> ===================================================================
> +HTMLDataGridRowElement::HTMLDataGridRowElement(const QualifiedName& name,
Document* document)
> +    : HTMLElement(name, document)

Add inline to constructor?


> Index: WebCore/html/HTMLDataListElement.cpp
> ===================================================================
> +HTMLDataListElement::HTMLDataListElement(const QualifiedName& tagName,
Document* document)
> +    : HTMLElement(tagName, document)

Add inline to constructor?

> Index: WebCore/html/HTMLDataListElement.h
> ===================================================================
>  /*
>   * Copyright (c) 2009, Google Inc. All rights reserved.

That small c and comma after 2009 are non-uniform. If you wish, it is fine with
me if you change that to "(C) 2009 Google..." in any of these files.


> Index: WebCore/html/HTMLTableColElement.cpp
> ===================================================================
> +HTMLTableColElement::HTMLTableColElement(const QualifiedName& tagName,
Document* document)

Add inline?