[webkit-reviews] review denied: [Bug 43746] Port view-source to new parser : [Attachment 63970] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 43746: Port view-source to new parser
https://bugs.webkit.org/show_bug.cgi?id=43746

Attachment 63970: Patch
https://bugs.webkit.org/attachment.cgi?id=63970&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.