Bug 42937 - splitTextAt*IfNeed and splitTextElementAt*IfNeed need to be cleaned up
Summary: splitTextAt*IfNeed and splitTextElementAt*IfNeed need to be cleaned up
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Blocker
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 39989
  Show dependency treegraph
 
Reported: 2010-07-24 17:15 PDT by Ryosuke Niwa
Modified: 2010-07-25 21:11 PDT (History)
6 users (show)

See Also:


Attachments
clean up (10.01 KB, patch)
2010-07-24 17:50 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed needS (10.00 KB, patch)
2010-07-24 17:52 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed references and pointers (space before &/*) (10.00 KB, patch)
2010-07-24 17:56 PDT, Ryosuke Niwa
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.