Bug 40356 - Fix handling of bytes received from the network while in document.write
Summary: Fix handling of bytes received from the network while in document.write
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: 39259
  Show dependency treegraph
 
Reported: 2010-06-09 00:48 PDT by Adam Barth
Modified: 2010-06-09 17:49 PDT (History)
1 user (show)

See Also:


Attachments
Patch (8.37 KB, patch)
2010-06-09 00:58 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (9.13 KB, patch)
2010-06-09 01:37 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (9.37 KB, patch)
2010-06-09 09:13 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (9.15 KB, patch)
2010-06-09 16:29 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (9.65 KB, patch)
2010-06-09 17:06 PDT, Adam Barth
no flags 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-06-09 00:48:25 PDT
Fix handling of bytes received from the network while in document.write
Comment 1 Adam Barth 2010-06-09 00:58:22 PDT
Created attachment 58220 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-06-09 01:07:39 PDT
Comment on attachment 58220 [details]
Patch

WebCore/html/HTML5Tokenizer.cpp:126
 +              // We don't want to consume any more of the input stream now.  Do
This is confusing.  I think you mean:
"We were just passed data off the network in a nested call to write().  We append this data to the end, but we don't process it until we unwind to a less nested write()." or similar.

WebCore/ChangeLog:18
 +          tokenizer.
You should explain here how you also fixed the close() stuff to be more sane.

WebCore/html/HTML5Tokenizer.h:111
 +                  m_inputStream->m_last = &m_afterInsertionPoint;
This line is not clear to me.

WebCore/html/HTML5Tokenizer.h:117
 +              if (m_inputStream->m_last == &m_afterInsertionPoint)
This is also not clear.

I think InsertionPointRecord needs more explanation.  And the ChangeLog needs to explain the close() stuff.  I assume you ran all the tests?
Comment 3 Adam Barth 2010-06-09 01:37:15 PDT
Created attachment 58223 [details]
Patch
Comment 4 Eric Seidel (no email) 2010-06-09 01:41:40 PDT
Attachment 58223 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3198078
Comment 5 David Levin 2010-06-09 09:07:57 PDT
Comment on attachment 58223 [details]
Patch

r- due to mac build break.

Tricky, it appears the problem is the \ at the end of the line here "//            /\"
Comment 6 Adam Barth 2010-06-09 09:13:47 PDT
Created attachment 58251 [details]
Patch
Comment 7 Eric Seidel (no email) 2010-06-09 13:41:10 PDT
Comment on attachment 58251 [details]
Patch

WebCore/html/HTML5Tokenizer.h:126
 +              if (m_inputStream->m_last == &m_inputStream->m_current)
You're right that this would be cleaner if this code was moved onto InputStream itself.  I don't really have good naming suggestions for you. :(

I think  my biggest gripe is that this m_last == check doesn't have a comment to explain what it's doing.  I'm still confused by both of the save and restore logic, even after some sleep. :)  Yes, I know what it does, but the code isn't easy to read.
Comment 8 Adam Barth 2010-06-09 16:29:35 PDT
Created attachment 58307 [details]
Patch
Comment 9 Adam Barth 2010-06-09 17:06:51 PDT
Created attachment 58310 [details]
Patch
Comment 10 Eric Seidel (no email) 2010-06-09 17:32:19 PDT
Comment on attachment 58310 [details]
Patch

The names "splitInto" and "mergeFrom" aren't very clear to me.  BUt the code looks good.  Thanks.
Comment 11 Adam Barth 2010-06-09 17:49:36 PDT
Comment on attachment 58310 [details]
Patch

Clearing flags on attachment: 58310

Committed r60926: <http://trac.webkit.org/changeset/60926>
Comment 12 Adam Barth 2010-06-09 17:49:44 PDT
All reviewed patches have been landed.  Closing bug.