[webkit-reviews] review denied: [Bug 45271] HTML parser should provide script column position within HTML document to JavaScript engine : [Attachment 68715] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Peter Rybin <peter.rybin at gmail.com>'s
request for review:
Bug 45271: HTML parser should provide script column position within HTML
document to JavaScript engine
https://bugs.webkit.org/show_bug.cgi?id=45271

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68715&action=review

This patch looks great.  I'd like to see one more iteration to address the
comments below.  Can we drop generation from this patch?  It doesn't seem to be
used.  We can add it (or something like it) in the next patch when we use it. 
Thanks for sticking with it.

> JavaScriptCore/GNUmakefile.am:517
> +	JavaScriptCore/wtf/TextPosition.cpp \
> +	JavaScriptCore/wtf/TextPosition.h \

These should probably be in JavaScriptCore/wtf/text

> JavaScriptCore/wtf/TextPosition.h:39
> + * for saving script source position. The third field 'generation' reflects
the fact
> + * that in HTML document its any part can be generated on the fly, by a
'document.write'
> + * call. While elements of the generated part may still have coordinates
(probably),

I'm not sure generation is appropriate for WTF.  WTF doesn't know anything
about document.write.

> JavaScriptCore/wtf/TextPosition.h:86
> +    int m_generation;

This doesn't seem right here.

> WebCore/bindings/js/ScriptSourceCode.h:45
> +    ScriptSourceCode(const String& source, const KURL& url = KURL(), const
TextPosition1& startPosition = TextPosition1::minimumPosition())

TextPosition1 => Does this build?

> WebCore/bindings/v8/ScriptEventListener.cpp:54
> +    // FIXME: very strange: we initialize zero-based number with '1'.
> +    TextPosition0 position(WTF::ZeroBasedNumber::fromZeroBasedInt(1),
WTF::ZeroBasedNumber::s_base, 0);

Yes, I think this code is wrong.  :)

Let's fix it in another patch.

> WebCore/dom/XMLDocumentParserLibxml2.cpp:1373
> +    // FIXME: the implementation probably returns 1-based int, but method
should return 0-based.
> +    //   New "- 1" fixes this. Is it correct?

I'm confused.  Are there tests that break with or without the -1?  We need
tests to cover this.  If there are tests, we can skip the comment.  If there
are no tests, I'd leave the code as-is, can keep the first line of the comment
(with a capital "T").  We can then write the test and fix the bug in a followup
patch.

> WebCore/dom/XMLDocumentParserLibxml2.cpp:1381
> +    // FIXME: the implementation probably returns 1-based int, but method
should return 0-based.
> +    //   New "- 1" fixes this. Is it correct?
> +    return  (context() ? context()->input->col : 1) - 1;

Same here.