Bug 36487

Summary: Refactoring: HTMLFormControlElement should not have redundant null check
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
a small change for cleanup
none
v1; removed the chagne on HTMLCanvasElemet none

Hajime Morrita
Reported 2010-03-23 04:39:04 PDT
HTMLFormControlElement::dispatchBlurEvent() and HTMLFormControlElement::dispatchFocusEvent() has redundant NULL-check. We can remove it to simplify the code.
Attachments
a small change for cleanup (3.01 KB, patch)
2010-03-23 04:55 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
v1; removed the chagne on HTMLCanvasElemet (1.67 KB, patch)
2010-03-23 22:18 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hajime Morrita
Comment 1 2010-03-23 04:55:10 PDT
Created attachment 51413 [details] a small change for cleanup
Alexey Proskuryakov
Comment 2 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; }
Darin Adler
Comment 3 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-
Hajime Morrita
Comment 4 2010-03-23 22:18:27 PDT
Created attachment 51477 [details] v1; removed the chagne on HTMLCanvasElemet
Hajime Morrita
Comment 5 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.
WebKit Commit Bot
Comment 6 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>
WebKit Commit Bot
Comment 7 2010-03-24 15:45:14 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.