[webkit-reviews] review denied: [Bug 30116] WebCore::InsertLineBreakCommand::shouldUseBreakElement ReadAV at NULL : [Attachment 58320] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Tony Chang (Google)
<tony at chromium.org>'s request for review:
Bug 30116: WebCore::InsertLineBreakCommand::shouldUseBreakElement ReadAV at NULL
https://bugs.webkit.org/show_bug.cgi?id=30116

Attachment 58320: Patch
https://bugs.webkit.org/attachment.cgi?id=58320&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
http://wkrietveld.appspot.com/30116/diff/2001/3003
File LayoutTests/editing/inserting/return-key-in-hidden-field.html (right):

http://wkrietveld.appspot.com/30116/diff/2001/3003#newcode13
LayoutTests/editing/inserting/return-key-in-hidden-field.html:13: }
Nit: technically by webkit style this (one-line if) shouldn't have curly
braces.

http://wkrietveld.appspot.com/30116/diff/2001/3003#newcode30
LayoutTests/editing/inserting/return-key-in-hidden-field.html:30:
layoutTestController.dumpAsText();
28-30 duplicates 20-22, was that intentional? I think you can just remove this.


http://wkrietveld.appspot.com/30116/diff/2001/3004
File WebCore/ChangeLog (right):

http://wkrietveld.appspot.com/30116/diff/2001/3004#newcode7
WebCore/ChangeLog:7: 
Maybe add a comment saying that this causes text insertions on
visibility:hidden elements to get ignored and add a link to bug 40342?

http://wkrietveld.appspot.com/30116/diff/2001/3005
File WebCore/editing/InsertLineBreakCommand.cpp (right):

http://wkrietveld.appspot.com/30116/diff/2001/3005#newcode97
WebCore/editing/InsertLineBreakCommand.cpp:97: VisiblePosition caret(pos);
I thought the conclusion was to make this a null-check with a comment pointing
to bug 40342 instead?