Bug 39697 – Make more HTML DOM members private, especially constructors

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

Make more HTML DOM members private, especially constructors

[Darin Adler]

Created attachment 57053 [details]
Patch

[David Levin]

Comment on attachment 57053 [details]
Patch

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

[Darin Adler]

(In reply to comment #2)
> (From update of attachment 57053 [details])
> > 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

Thanks, I'll fix that.

> > 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));"

That's the underlying purpose of this sequence of patches. Once every caller is using create functions and the calls to new are all in a smaller number of call sites, then we can change the reference counts to start at 1 and we will then use adoptRef. It's safer for me to separate the "make a create" function step from the "change reference counts to start at 1" case.

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

Yes, true.

> > 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/HTMLTableColElement.cpp
> > ===================================================================
> > +HTMLTableColElement::HTMLTableColElement(const QualifiedName& tagName, Document* document)
> 
> Add inline?

OK, I'll think about it for those.

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

OK, I might do that.

[Darin Adler]

(In reply to comment #3)
> It's safer for me to separate the "make a create" function step from the "change reference counts to start at 1" case.

I meant:

It's safer for me to separate the "make a create function" step from the "change reference count to start at 1 and use adoptRef" step.

[Darin Adler]

Committed r60342: <http://trac.webkit.org/changeset/60342>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60342 might have broken Tiger Intel Release