Bug 36487

Summary:

Refactoring: HTMLFormControlElement should not have redundant null check

Product:

WebKit

Reporter:

Hajime Morrita <morrita>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, commit-queue

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
a small change for cleanup	none
v1; removed the chagne on HTMLCanvasElemet	none

Description Hajime Morrita 2010-03-23 04:39:04 PDT

HTMLFormControlElement::dispatchBlurEvent() and HTMLFormControlElement::dispatchFocusEvent() has redundant NULL-check.
We can remove it to simplify the code.

Comment 1 Hajime Morrita 2010-03-23 04:55:10 PDT

Created attachment 51413 [details]
a small change for cleanup

Comment 2 Alexey Proskuryakov 2010-03-23 11:14:47 PDT

It looks like this patch introduces a redundant null check. Or am I just confused?

Page* Document::page() const
{
    return m_frame ? m_frame->page() : 0;    
}

Comment 3 Darin Adler 2010-03-23 13:57:02 PDT

Comment on attachment 51413 [details]
a small change for cleanup

The changes to HTMLFormControlElement::dispatchFocusEvent and HTMLFormControlElement::dispatchBlurEvent are OK. Not a great improvement, but nice to use the Document::page helper function to save a little code.

The changes to HTMLCanvasElement::convertLogicalToDevice and HTMLCanvasElement::convertLogicalToDevice are different though. Those are adding a null check of Page*. Presumably, this is a bug fix. Before, there would be a crash if the page was zero. That means we need to construct a test case to demonstrate that crash, and that the crash is fixed.

review-

Comment 4 Hajime Morrita 2010-03-23 22:18:27 PDT

Created attachment 51477 [details]
v1; removed the chagne on HTMLCanvasElemet

Comment 5 Hajime Morrita 2010-03-23 22:21:11 PDT

Thank you for reviewing!

> The changes to HTMLCanvasElement::convertLogicalToDevice and
> HTMLCanvasElement::convertLogicalToDevice are different though. Those are
> adding a null check of Page*. Presumably, this is a bug fix. Before, there
> would be a crash if the page was zero. That means we need to construct a test
> case to demonstrate that crash, and that the crash is fixed.
Agreed. But I could not figure out the test case for crash,
so I remove the change on HTMLCanvasElement.

@ap
Thank you reviewing too.
You are right. I did attempt to simplify the null-check code, 
but It actually introduced a redundant null check at runtime.

Regards.

Comment 6 WebKit Commit Bot 2010-03-24 15:45:09 PDT

Comment on attachment 51477 [details]
v1; removed the chagne on HTMLCanvasElemet

Clearing flags on attachment: 51477

Committed r56463: <http://trac.webkit.org/changeset/56463>

Comment 7 WebKit Commit Bot 2010-03-24 15:45:14 PDT

All reviewed patches have been landed.  Closing bug.