Bug 42937

Summary: splitTextAt*IfNeed and splitTextElementAt*IfNeed need to be cleaned up
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Blocker CC: darin, eric, levin, ojan, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 39989    
Attachments:
Description Flags
clean up
none
fixed needS
none
fixed references and pointers (space before &/*) tkent: review+

Description Ryosuke Niwa 2010-07-24 17:15:25 PDT
splitTextAtStartIfNeed, splitTextAtEndIfNeed, splitTextElementAtStartIfNeed and splitTextElementAtEndIfNeed need to be cleaned up to fix the bug 39989.
Comment 1 Ryosuke Niwa 2010-07-24 17:50:37 PDT
Created attachment 62512 [details]
clean up

Split the cleanup from the patch for the bug 39989.
Comment 2 WebKit Review Bot 2010-07-24 17:52:56 PDT
Attachment 62512 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/ApplyStyleCommand.cpp:1538:  Declaration has space between type name and * in Text *text  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1545:  Declaration has space between type name and * in Text *text  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1548:  Declaration has space between type name and * in Node *prevNode  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1550:  Declaration has space between type name and * in Node *startNode  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1558:  Declaration has space between type name and * in Text *text  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1565:  Declaration has space between type name and * in Text *text  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1568:  Declaration has space between type name and * in Node *prevNode  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1570:  Declaration has space between type name and * in Node *startNode  [whitespace/declaration] [3]
Total errors found: 8 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Ryosuke Niwa 2010-07-24 17:52:58 PDT
Created attachment 62513 [details]
fixed needS
Comment 4 WebKit Review Bot 2010-07-24 17:53:40 PDT
Attachment 62513 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/ApplyStyleCommand.cpp:1538:  Declaration has space between type name and * in Text *text  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1545:  Declaration has space between type name and * in Text *text  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1548:  Declaration has space between type name and * in Node *prevNode  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1550:  Declaration has space between type name and * in Node *startNode  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1558:  Declaration has space between type name and * in Text *text  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1565:  Declaration has space between type name and * in Text *text  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1568:  Declaration has space between type name and * in Node *prevNode  [whitespace/declaration] [3]
WebCore/editing/ApplyStyleCommand.cpp:1570:  Declaration has space between type name and * in Node *startNode  [whitespace/declaration] [3]
Total errors found: 8 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Ryosuke Niwa 2010-07-24 17:56:41 PDT
Created attachment 62515 [details]
fixed references and pointers (space before &/*)
Comment 6 Kent Tamura 2010-07-25 19:35:46 PDT
Comment on attachment 62515 [details]
fixed references and pointers (space before &/*)


> -
> -bool ApplyStyleCommand::splitTextElementAtEndIfNeeded(const Position &start, const Position &end)
> +    

nit: Do not add whitespace at the end of line.

>  }
> -
> +    
>  static bool areIdenticalElements(Node *first, Node *second)

ditto.
Comment 7 David Levin 2010-07-25 20:02:58 PDT
In the ChangeLog "No new tests added."

This should explain why no new tests were added. Was it because no user visible functionality should have changed?
Comment 8 Ryosuke Niwa 2010-07-25 20:29:48 PDT
(In reply to comment #6)
> (From update of attachment 62515 [details])
> 
> > -
> > -bool ApplyStyleCommand::splitTextElementAtEndIfNeeded(const Position &start, const Position &end)
> > +    
> 
> nit: Do not add whitespace at the end of line.
> 
> >  }
> > -
> > +    
> >  static bool areIdenticalElements(Node *first, Node *second)
> 
> ditto.

Thanks for the review & style check.  I didn't catch that.

(In reply to comment #7)
> In the ChangeLog "No new tests added."
> 
> This should explain why no new tests were added. Was it because no user visible functionality should have changed?

I thought it's quite normal to omit the reason or mentioning of test at all when the bug itself says it's a clean up. But I'll change it to "No new tests added since this is a clean up" per your request.
Comment 9 Ryosuke Niwa 2010-07-25 21:11:52 PDT
Landed as http://trac.webkit.org/changeset/64028.