WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
39697
Make more HTML DOM members private, especially constructors
https://bugs.webkit.org/show_bug.cgi?id=39697
Summary
Make more HTML DOM members private, especially constructors
Darin Adler
Reported
2010-05-25 16:03:57 PDT
Make more HTML DOM members private, especially constructors
Attachments
Patch
(58.33 KB, patch)
2010-05-25 16:21 PDT
,
Darin Adler
levin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
2010-05-25 16:21:02 PDT
Created
attachment 57053
[details]
Patch
David Levin
Comment 2
2010-05-26 11:51:53 PDT
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
Comment 3
2010-05-26 13:54:10 PDT
(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
Comment 4
2010-05-26 21:10:12 PDT
(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
Comment 5
2010-05-27 21:22:21 PDT
Committed
r60342
: <
http://trac.webkit.org/changeset/60342
>
WebKit Review Bot
Comment 6
2010-05-27 22:43:36 PDT
http://trac.webkit.org/changeset/60342
might have broken Tiger Intel Release
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug