Make more HTML DOM members private, especially constructors
Created attachment 57053 [details] Patch
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?
(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.
(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.
Committed r60342: <http://trac.webkit.org/changeset/60342>
http://trac.webkit.org/changeset/60342 might have broken Tiger Intel Release