Bug 113364 - Selection code spends a lot of time in InlineTextBox::localSelectionRect
Summary: Selection code spends a lot of time in InlineTextBox::localSelectionRect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 113358
Blocks: 113478
  Show dependency treegraph
 
Reported: 2013-03-26 21:01 PDT by Ryosuke Niwa
Modified: 2013-03-27 22:37 PDT (History)
11 users (show)

See Also:


Attachments
Work in progress 1 (888 bytes, patch)
2013-03-26 21:01 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
A/B test result with WIP1 (24.77 KB, text/html)
2013-03-26 21:02 PDT, Ryosuke Niwa
no flags Details
Ready for review (1.70 KB, patch)
2013-03-27 11:00 PDT, Ryosuke Niwa
enrica: 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 2013-03-26 21:01:11 PDT
As far as I've sampled in the bug 113358, we're spending 5+% of time in InlineTextBox::localSelectionRect computing font width.
But there is no need to do this when we're selecting the entire line box.
Comment 1 Ryosuke Niwa 2013-03-26 21:01:51 PDT
Created attachment 195214 [details]
Work in progress 1
Comment 2 Ryosuke Niwa 2013-03-26 21:02:56 PDT
Created attachment 195215 [details]
A/B test result with WIP1
Comment 3 Radar WebKit Bug Importer 2013-03-26 21:03:56 PDT
<rdar://problem/13513153>
Comment 4 WebKit Review Bot 2013-03-26 21:11:31 PDT
Attachment 195214 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/InlineTextBox.cpp']" exit_code: 1
Source/WebCore/rendering/InlineTextBox.cpp:211:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Enrica Casucci 2013-03-27 10:36:54 PDT
Comment on attachment 195214 [details]
Work in progress 1

View in context: https://bugs.webkit.org/attachment.cgi?id=195214&action=review

Missing ChangeLog file

>> Source/WebCore/rendering/InlineTextBox.cpp:211
>> +    if (sPos != 0 || ePos != static_cast<int>(m_len))
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

Please fix the style issue and add a comment.
Comment 6 Ryosuke Niwa 2013-03-27 10:40:16 PDT
(In reply to comment #5)
> (From update of attachment 195214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195214&action=review
> 
> Missing ChangeLog file
> 
> >> Source/WebCore/rendering/InlineTextBox.cpp:211
> >> +    if (sPos != 0 || ePos != static_cast<int>(m_len))
> > 
> > Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> 
> Please fix the style issue and add a comment.

Oh, oops. This wasn't up for review.  The one on https://bugs.webkit.org/show_bug.cgi?id=113358 is. I'm trying to add the performance test first so that people can verify my assertion that my patch improves the performance.
Comment 7 Ryosuke Niwa 2013-03-27 11:00:47 PDT
Created attachment 195358 [details]
Ready for review
Comment 8 Enrica Casucci 2013-03-27 11:05:45 PDT
Comment on attachment 195358 [details]
Ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=195358&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:209
> +    FloatPoint startingPoint = FloatPoint(logicalLeft(), selTop);

I would add a comment here about the optimization.
Comment 9 Ryosuke Niwa 2013-03-27 13:49:17 PDT
Committed r147008: <http://trac.webkit.org/changeset/147008>