[Webkit-unassigned] [Bug 45271] HTML parser should provide script column position within HTML document to JavaScript engine

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45271


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68715|review?                     |review-
               Flag|                            |




--- Comment #25 from Adam Barth <abarth at webkit.org>  2010-09-24 11:27:30 PST ---
(From update of attachment 68715)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.