Bug 43746 – Port view-source to new parser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Port view-source to new parser

[Adam Barth]

Created attachment 63928 [details]
Patch

[Adam Barth]

I'm going to try another approach of creating the view-source view directly from the input stream.

[Adam Barth]

Created attachment 63931 [details]
now with HTMLViewSourceParser

[Adam Barth]

Comment on attachment 63931 [details]
now with HTMLViewSourceParser

Ok.  I've got this working.  I'm going to upload the changes in three dependent bugs for ease of reviewing.

[Adam Barth]

Created attachment 63970 [details]
Patch

[Adam Barth]

I should say that most (all?) of the code in HTMLViewSourceDocument should really be in a HTMLViewSourceTreeBuilder, but I didn't have the heart to do that in this patch.

[Early Warning System Bot]

Attachment 63970 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3750009

[Eric Seidel (no email)]

Attachment 63970 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3745016

[WebKit Review Bot]

Attachment 63970 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3731006

[Eric Seidel (no email)]

Comment on attachment 63970 [details]
Patch

WebCore/html/HTMLToken.h:293
 +      int m_startIndex;
I'm surprised you didn't use your little Range object here.

WebCore/html/HTMLViewSourceDocument.cpp:105
 +      if (!m_current)
This seems like an old treebuilder concept.  Strange to see it in this Document class?

WebCore/html/HTMLViewSourceDocument.cpp:144
 +      String tagName = String(token.name().data(), token.name().size());
I'm slightly surprised Token doesn't have a method to do this for you, but I guess that's what AtomicToken is all about?

WebCore/html/HTMLViewSourceDocument.cpp:162
 +          if (tagName == "base" && name == "href") {
We have AtomicStrings for these.

WebCore/html/HTMLViewSourceDocument.cpp:156
 +          String name = String(iter->m_name.data(), iter->m_name.size());
Don't we have a String-From-Vector conversion function?  Or shouldn't we use an inline for these?  Seems silly that this is the 3rd time in this patch so far... :)

WebCore/html/HTMLViewSourceDocument.cpp:168
 +          index = addRange(source, index, iter->m_valueRange.m_start - token.startIndex(), "");
We keep doing this same code too.  Maybe this needs a wrapper around addRange?  Or something to do this subtraction?  Seems like a lot of copy/paste code here.

WebCore/html/HTMLViewSourceDocument.cpp:170
 +          bool isLink = name == "src" || name == "href";
We have AtomicStrings for these.

WebCore/html/HTMLViewSourceDocument.cpp:171
 +          index = addRange(source, index, iter->m_valueRange.m_end - token.startIndex(), "webkit-html-attribute-value", isLink, tagName == "a");
And these...

WebCore/html/HTMLViewSourceDocument.cpp:276
 +      if (start < end) {
Early return instead?

WebCore/html/HTMLViewSourceDocument.cpp:277
 +          String text = source.substring(start, end - start);
I might have put this whole block in a function "spanForText".  Not sure I understand welle enough yet.  The setting m_current feels like something wanting to call a function to return whatever your'e about to set m_current too.

Overall I think this is great.  I'd like to see a refreshed patch though.

[Adam Barth]

(In reply to comment #10)
> (From update of attachment 63970 [details])
> WebCore/html/HTMLToken.h:293
>  +      int m_startIndex;
> I'm surprised you didn't use your little Range object here.

Fixed.

> WebCore/html/HTMLViewSourceDocument.cpp:105
>  +      if (!m_current)
> This seems like an old treebuilder concept.  Strange to see it in this Document class?

Yes.  This is wrong code inherited from the old HTMLViewSourceDocument.  I'd like to move it to an HTMLViewSourceTreeBuilder at some point.

> I'm slightly surprised Token doesn't have a method to do this for you, but I guess that's what AtomicToken is all about?

Yes.  Token had a bunch of string caching infrastructure at some point, but we moved all that to atomic token.

> WebCore/html/HTMLViewSourceDocument.cpp:162
>  +          if (tagName == "base" && name == "href") {
> We have AtomicStrings for these.

True!  Now that we don't need to do case-insensitive compares we can use the atomics.

> WebCore/html/HTMLViewSourceDocument.cpp:156
>  +          String name = String(iter->m_name.data(), iter->m_name.size());
> Don't we have a String-From-Vector conversion function?  Or shouldn't we use an inline for these?  Seems silly that this is the 3rd time in this patch so far... :)

We have one, but not for vectors with inline capacity, I believe.

> WebCore/html/HTMLViewSourceDocument.cpp:168
>  +          index = addRange(source, index, iter->m_valueRange.m_start - token.startIndex(), "");
> We keep doing this same code too.  Maybe this needs a wrapper around addRange?  Or something to do this subtraction?  Seems like a lot of copy/paste code here.

Hum...  Ok.  I look at that.

> WebCore/html/HTMLViewSourceDocument.cpp:170
>  +          bool isLink = name == "src" || name == "href";
> We have AtomicStrings for these.

Yep.

> WebCore/html/HTMLViewSourceDocument.cpp:171
>  +          index = addRange(source, index, iter->m_valueRange.m_end - token.startIndex(), "webkit-html-attribute-value", isLink, tagName == "a");
> And these...

Yep.

> WebCore/html/HTMLViewSourceDocument.cpp:276
>  +      if (start < end) {
> Early return instead?

Sure.

> WebCore/html/HTMLViewSourceDocument.cpp:277
>  +          String text = source.substring(start, end - start);
> I might have put this whole block in a function "spanForText".  Not sure I understand welle enough yet.  The setting m_current feels like something wanting to call a function to return whatever your'e about to set m_current too.

Hum...  I'm not sure I quite understand.

> Overall I think this is great.  I'd like to see a refreshed patch though.

Okiedokes.

[Adam Barth]

> > WebCore/html/HTMLViewSourceDocument.cpp:156
> >  +          String name = String(iter->m_name.data(), iter->m_name.size());
> > Don't we have a String-From-Vector conversion function?  Or shouldn't we use an inline for these?  Seems silly that this is the 3rd time in this patch so far... :)
> 
> We have one, but not for vectors with inline capacity, I believe.

I tried adding this, but I failed.  I'm not sure why, but I couldn't get it to compile.  :(

> > WebCore/html/HTMLViewSourceDocument.cpp:168
> >  +          index = addRange(source, index, iter->m_valueRange.m_start - token.startIndex(), "");
> > We keep doing this same code too.  Maybe this needs a wrapper around addRange?  Or something to do this subtraction?  Seems like a lot of copy/paste code here.
> 
> Hum...  Ok.  I look at that.

The problem is that these are all independent pieces of data.  I could turn the "-" into a "," but I'm not sure that would buy us much.

[Adam Barth]

Created attachment 64065 [details]
Patch

[Early Warning System Bot]

Attachment 64065 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3752029

[Eric Seidel (no email)]

Attachment 64065 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3775036

[Adam Barth]

Created attachment 64066 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 64066 [details]
Patch

Slightly sad you weren't able to make the String constructor work.  It probably had to be a template itself or something.

WebCore/html/HTMLToken.h:75
 +      int startIndex() const { return m_range.m_start; }
Feels slightly odd to not just expose the Range object, but seems fine.

WebCore/html/HTMLViewSourceParser.cpp:41
 +      ASSERT_NOT_REACHED();
I'm surprised you have to implement insert()

WebCore/html/HTMLViewSourceParser.cpp:79
 +      // FIXME: The tokenizer should do this work for us.
I don't understand what you mean here.  Could you file a bug?

Looks great!

[Adam Barth]

Committed r65132: <http://trac.webkit.org/changeset/65132>