Bug 40274 - Fix script-after-frameset test in HTML5 parser
Summary: Fix script-after-frameset test in HTML5 parser
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: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 39259
  Show dependency treegraph
 
Reported: 2010-06-07 18:55 PDT by Tony Gentilcore
Modified: 2010-06-09 10:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.86 KB, patch)
2010-06-07 19:00 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Fix tabs in ChangeLog (3.89 KB, patch)
2010-06-07 20:59 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (3.86 KB, patch)
2010-06-08 09:14 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2010-06-07 18:55:30 PDT
Fix script-after-frameset test in HTML5 parser
Comment 1 Tony Gentilcore 2010-06-07 19:00:05 PDT
Created attachment 58099 [details]
Patch
Comment 2 Tony Gentilcore 2010-06-07 19:01:36 PDT
Let me know if you like this approach. If you don't think it makes sense to add InsertionMode here, another alternative would be to just add an afterFrameset boolean (similar to what HTMLTokenizer::scriptHandler did).
Comment 3 WebKit Review Bot 2010-06-07 19:03:42 PDT
Attachment 58099 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Tony Gentilcore 2010-06-07 20:59:23 PDT
Created attachment 58108 [details]
Fix tabs in ChangeLog
Comment 5 Eric Seidel (no email) 2010-06-07 22:12:52 PDT
Comment on attachment 58108 [details]
Fix tabs in ChangeLog

WebCore/ChangeLog:13
 +          No new tests because covered by existing test.
You should give some example test.

WebCore/html/HTML5TreeBuilder.h:65
 +      // FIXME: Move this to HTML5Parser once it is created.
There is no HTML5Parser. :)  This is the right place for this.

This patch looks perfect, except for the comment.
Comment 6 Eric Seidel (no email) 2010-06-07 22:44:28 PDT
I think I confused you earlier with my talk of HTML5Parser vs. HTML5Tokenizer, etc.

CurrentName (PlannedName)

New design:

HTML5Tokenizer (HTMLParser)
  HTML5Lexer (probably will stay HTMLLexer, might be HTMLTokenizer)
    InputStreamPreprocesser (will keep its name)
    SegmentedString (will keep its name)
  HTML5TreeBuilder (HTMLTreeBuilder -- spec also calls this "parser" and "tree builder" interchangeably)
  HTML5ScriptRunner (HTMLScriptRunner, maybe HTMLScriptScheduler?)

Old design:

HTMLTokenizer (LegacyHTMLParser or LegacyHTMLTokenizer)
  SegmentedString (will keep)
  HTMLParser (LegacyHTMLTreeBuilder)

So it's confusing to talk about "HTMLParser" since it could mean lots of things.  "Tokenizer" is a horrible name for the current class hierarchy.  We have prefixed all our new classes with HTML5 to separate them from the old system.
Comment 7 Tony Gentilcore 2010-06-08 09:14:50 PDT
Created attachment 58143 [details]
Patch
Comment 8 Tony Gentilcore 2010-06-08 09:15:51 PDT
(In reply to comment #5)
> (From update of attachment 58108 [details])
> WebCore/ChangeLog:13
>  +          No new tests because covered by existing test.
> You should give some example test.

Done.

> 
> WebCore/html/HTML5TreeBuilder.h:65
>  +      // FIXME: Move this to HTML5Parser once it is created.
> There is no HTML5Parser. :)  This is the right place for this.
> 
Done.

> This patch looks perfect, except for the comment.
Comment 9 Tony Gentilcore 2010-06-08 09:16:35 PDT
(In reply to comment #6)
> I think I confused you earlier with my talk of HTML5Parser vs. HTML5Tokenizer, etc.
> 
> CurrentName (PlannedName)
> 
> New design:
> 
> HTML5Tokenizer (HTMLParser)
>   HTML5Lexer (probably will stay HTMLLexer, might be HTMLTokenizer)
>     InputStreamPreprocesser (will keep its name)
>     SegmentedString (will keep its name)
>   HTML5TreeBuilder (HTMLTreeBuilder -- spec also calls this "parser" and "tree builder" interchangeably)
>   HTML5ScriptRunner (HTMLScriptRunner, maybe HTMLScriptScheduler?)
> 
> Old design:
> 
> HTMLTokenizer (LegacyHTMLParser or LegacyHTMLTokenizer)
>   SegmentedString (will keep)
>   HTMLParser (LegacyHTMLTreeBuilder)
> 
> So it's confusing to talk about "HTMLParser" since it could mean lots of things.  "Tokenizer" is a horrible name for the current class hierarchy.  We have prefixed all our new classes with HTML5 to separate them from the old system.

Got it now. Thank you.
Comment 10 Adam Barth 2010-06-08 10:07:02 PDT
Comment on attachment 58143 [details]
Patch

WebCore/html/HTML5TreeBuilder.h:65
 +      enum InsertionMode {
Sadly, this might eventually need to be public.  Let's keep it private for as long as we can.

WebCore/html/HTML5TreeBuilder.h:77
 +      InsertionMode insertionMode() const { return m_insertionMode; }
I'm not sure these private getters/setters are needed, but ok.
Comment 11 Adam Barth 2010-06-09 10:05:27 PDT
Comment on attachment 58143 [details]
Patch

Clearing flags on attachment: 58143

Committed r60897: <http://trac.webkit.org/changeset/60897>
Comment 12 Adam Barth 2010-06-09 10:05:35 PDT
All reviewed patches have been landed.  Closing bug.