Bug 41907 - Handle whitespace correctly
Summary: Handle whitespace correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 41123
  Show dependency treegraph
 
Reported: 2010-07-08 14:41 PDT by Adam Barth
Modified: 2010-07-09 01:08 PDT (History)
1 user (show)

See Also:


Attachments
Patch (20.42 KB, patch)
2010-07-08 14:46 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (20.61 KB, patch)
2010-07-08 15:25 PDT, Adam Barth
abarth: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-07-08 14:41:33 PDT
Handle whitespace correctly
Comment 1 Adam Barth 2010-07-08 14:46:40 PDT
Created attachment 60963 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-07-08 15:17:32 PDT
Comment on attachment 60963 [details]
Patch

WebCore/html/HTMLTreeBuilder.cpp:104
 +      String takeAll()
Should this be takeRemaining()?

WebCore/html/HTMLTreeBuilder.cpp:112
 +      String takeAllWhitespace()
takeRemainingWhitespace?

WebCore/html/HTMLTreeBuilder.cpp:109
 +          return String(start, m_current - start);
Could this have an optimized branch which returned the original string if we never moved m_current?

I like this change.  I think it would be useful to know the performance impact.  Not that it would actually change us from landing it, but would be useful to know how much perf we're losing here.
Comment 3 Adam Barth 2010-07-08 15:22:13 PDT
> WebCore/html/HTMLTreeBuilder.cpp:104
>  +      String takeAll()
> Should this be takeRemaining()?

Sure.

> WebCore/html/HTMLTreeBuilder.cpp:112
>  +      String takeAllWhitespace()
> takeRemainingWhitespace?

Sure.

> WebCore/html/HTMLTreeBuilder.cpp:109
>  +          return String(start, m_current - start);
> Could this have an optimized branch which returned the original string if we never moved m_current?

Yeah, but I'm going to make the source of these characters be the original buffer in the HTMLTokenizer, which is an inline buffer, not a String.

> I like this change.  I think it would be useful to know the performance impact.  Not that it would actually change us from landing it, but would be useful to know how much perf we're losing here.

True.  I'm working from my laptop, so it's painful to build release from scratch.  :(
Comment 4 Adam Barth 2010-07-08 15:25:28 PDT
Created attachment 60970 [details]
Patch for landing
Comment 5 Adam Barth 2010-07-09 01:08:59 PDT
Committed r62912: <http://trac.webkit.org/changeset/62912>