Bug 39916 – Make more HTML DOM members private, especially constructors, third and final batch

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Make more HTML DOM members private, especially constructors, third and final batch

[Darin Adler]

Created attachment 57396 [details]
Patch

[WebKit Review Bot]

Attachment 57396 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2592059

[WebKit Review Bot]

Attachment 57396 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2657012

[Sam Weinig]

Comment on attachment 57396 [details]
Patch

JavaScriptCore/wtf/OwnArrayPtr.h:2
 +   *  Copyright (C) 2006, 2010 Apple Inc. All righs reserved.
Typo: All rights reserved.

WebCore/editing/DeleteButton.cpp:57
 +              return;
Is this a functional change, or just an optimization?  Since it seems quite unrelated, it might make sense to do it in another patch. (We don't have any DeleteButton tests do we?

WebCore/html/HTMLFrameSetElement.h:91
 +      OwnArrayPtr<Length> m_colLengths;
Where is OwnArrayPtr.h getting pulled in from?

WebCore/rendering/TextControlInnerElements.cpp:183
 +          return;
Same question here about behavior change as the other defaultEventHandler change.  This is in a few more places as well.

r=me. It seems like some of the EWS are not happy, so if you can divine their complaints, that might be helpful before landing.

[Darin Adler]

(In reply to comment #4)
> JavaScriptCore/wtf/OwnArrayPtr.h:2
>  +   *  Copyright (C) 2006, 2010 Apple Inc. All righs reserved.
> Typo: All rights reserved.

Thanks. I fixed that and landed just this change as <http://trac.webkit.org/changeset/60417>.

> WebCore/editing/DeleteButton.cpp:57
>  +              return;
> Is this a functional change, or just an optimization?  Since it seems quite unrelated, it might make sense to do it in another patch. (We don't have any DeleteButton tests do we?

I’ll roll that out and replace it with a FIXME. Generally speaking default event handlers should return and not call through to the base class when they handle the default, but clearly I should not mix any changes of that type in with this change.

> WebCore/html/HTMLFrameSetElement.h:91
>  +      OwnArrayPtr<Length> m_colLengths;
> Where is OwnArrayPtr.h getting pulled in from?

Good point. No idea. I’ll add the include.

> WebCore/rendering/TextControlInnerElements.cpp:183
>  +          return;
> Same question here about behavior change as the other defaultEventHandler change.  This is in a few more places as well.

I’ll roll all this out.

> r=me. It seems like some of the EWS are not happy, so if you can divine their complaints, that might be helpful before landing.

Thanks.

The Chromium-Linux failure seems to just be the lack of OwnArrayPtr.h include. The Qt and Windows failures are mysterious.

[Darin Adler]

(In reply to comment #5)
> > WebCore/rendering/TextControlInnerElements.cpp:183
> >  +          return;
> > Same question here about behavior change as the other defaultEventHandler change.  This is in a few more places as well.
> 
> I’ll roll all this out.

These other changes were not behavior changes. I just made the logic more idiomatic as opposed to the old code which checked defaultHandled explicitly later in the function instead of the usual style.

I'll keep them.

[Darin Adler]

(In reply to comment #6)
> I'll keep them.

Changed my mind again. I'll roll them back and do this change later if needed.

[Darin Adler]

Committed r60418: <http://trac.webkit.org/changeset/60418>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60418 might have broken Chromium Linux Release

[Daniel Bates]

(In reply to comment #9)
> http://trac.webkit.org/changeset/60418 might have broken Chromium Linux Release

This change broke the Apple Windows bots, all the Qt bots, and all the Chromium bots.

[Daniel Bates]

Prepared rollout <https://bugs.webkit.org/show_bug.cgi?id=39937> while I look to see if this is straight forward to repair online.

[Daniel Bates]

Committed attempt to fix the build in change set 60419 <http://trac.webkit.org/changeset/60419>.

[Darin Adler]

(In reply to comment #12)
> Committed attempt to fix the build in change set 60419 <http://trac.webkit.org/changeset/60419>.

Thanks very much, Daniel, for fixing this instead of rolling it out!

[Daniel Bates]

(In reply to comment #13)
> (In reply to comment #12)
> > Committed attempt to fix the build in change set 60419 <http://trac.webkit.org/changeset/60419>.
> 
> Thanks very much, Daniel, for fixing this instead of rolling it out!

No problem.

[Daniel Bates]

For historical bookkeeping, I also committed change-set 60420 <http://trac.webkit.org/changeset/60420> to fix the build.

And, committed change-set 60444 <http://trac.webkit.org/changeset/60444> to add my commit messages for r60419, and r60420 to the change logs.