Bug 51601 - WML Parser should treat line/column number in a consistent way
Summary: WML Parser should treat line/column number in a consistent way
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-24 14:15 PST by Joone Hur
Modified: 2011-01-06 12:53 PST (History)
3 users (show)

See Also:


Attachments
Proposed Patch (9.98 KB, patch)
2010-12-24 17:43 PST, Joone Hur
eric: review-
Details | Formatted Diff | Diff
Proposed Patch2 (10.02 KB, patch)
2010-12-27 12:59 PST, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch3 (9.77 KB, patch)
2010-12-28 15:29 PST, Joone Hur
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch4 (9.87 KB, patch)
2011-01-02 17:16 PST, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch5 (9.91 KB, patch)
2011-01-02 23:52 PST, Joone Hur
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch6 (10.38 KB, patch)
2011-01-06 07:40 PST, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2010-12-24 14:15:40 PST
Currently, XML Parser treats line/column number as 1-based values, but WML ErrorHandler treat them as 0-based.
Therefore, we need to treat them in a consistent way, but I'm not sure which way is best.
When considering the current implementation, it seems to be easier to allow WML ErrorHandler to use 1-based values.
Comment 1 Joone Hur 2010-12-24 17:43:39 PST
Created attachment 77432 [details]
Proposed Patch

This patch includes https://bugs.webkit.org/attachment.cgi?id=76395
Comment 2 Peter Rybin 2010-12-25 13:47:41 PST
(In reply to comment #1)
> Created an attachment (id=77432) [details]
> Proposed Patch
> 
> This patch includes https://bugs.webkit.org/attachment.cgi?id=76395

LGTM (I'm not a reviewer)
Comment 3 Eric Seidel (no email) 2010-12-25 13:59:50 PST
Comment on attachment 77432 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77432&action=review

In general looks good.  It hink this needs one more round though, given teh nits.

And thanks peter!  Unofficial reviews are *always* welcome (and are infact "required" if you ever plan to be a reviewer)

> WebCore/dom/XMLDocumentParser.cpp:151
> +    handleError(type, m, TextPosition1(WTF::OneBasedNumber::fromOneBasedInt(lineNumber), WTF::OneBasedNumber::fromOneBasedInt(columnNumber)));

This is really the only way to construct a TextPosition1?  Seems cumbersome.

> WebCore/dom/XMLDocumentParser.h:89
> +        void handleError(ErrorType, const char* message, TextPosition1 position);

The arg name for "position" isn't needed.

> WebCore/dom/XMLDocumentParser.h:108
> +        // This method is introduced to temporary legalize existing line/column

legalize?  What law? :)

All sentences should start with a capital and end with a period. :)

> WebCore/dom/XMLDocumentParser.h:214
> +        TextPosition1 m_lastErrorPosition;

Much nicer!

> WebCore/dom/XMLDocumentParserLibxml2.cpp:560
> +    , m_lastErrorPosition(TextPosition1::belowRangePosition())

Seems we might want the default TextPosition constructor to do this, no?  Then we wouldndt' even need these lines.
Comment 4 Peter Rybin 2010-12-25 14:40:54 PST
> > WebCore/dom/XMLDocumentParser.cpp:151
> > +    handleError(type, m, TextPosition1(WTF::OneBasedNumber::fromOneBasedInt(lineNumber), WTF::OneBasedNumber::fromOneBasedInt(columnNumber)));
> 
> This is really the only way to construct a TextPosition1?  Seems cumbersome.

This isn't really polished for everyday use so far. E.g. ZeroBasedNumber/OneBasedNumber types needs namespace qualifier. 

I think the API should be extended as needed.
The main limitation I had in mind was we should be able to merge corresponding types in the future (TextPosition0/TextPosition1 -> TextPosition), so we can't have a constructor that accepts ints and all int-dealing methods should have names saying "zero based" or "one based".
As for now TextPosition0/TextPosition1 accurately denote what kind of ints was used in the place before refactoring (to help prove that we didn't break anything).

> > WebCore/dom/XMLDocumentParser.h:108
> > +        // This method is introduced to temporary legalize existing line/column
> legalize?  What law? :)

A law that strictly separates 0-based and 1-based integers from each other -- we have to break it in this one place.
Anyway I'm fine with changing wording :)

> > WebCore/dom/XMLDocumentParserLibxml2.cpp:560
> > +    , m_lastErrorPosition(TextPosition1::belowRangePosition())
> Seems we might want the default TextPosition constructor to do this, no?  Then we wouldndt' even need these lines.

Unfortunately there are 2 defaults: "below range" and "base" (-1 and 0 for 0-based). It's fine to choose in favor of one of them, historically I just wanted to have my patch as transparent as possible.
Comment 5 Joone Hur 2010-12-27 12:59:59 PST
Created attachment 77516 [details]
Proposed Patch2

How about using "allow" instead of "legalize"?
Comment 6 Eric Seidel (no email) 2010-12-28 14:17:53 PST
Comment on attachment 77516 [details]
Proposed Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=77516&action=review

> WebCore/dom/XMLDocumentParser.h:109
> +        // This method is introduced to temporary allow existing line/column
> +        // coordinate bug: it is believed that numbers that originally were zero-based
> +        // eventually becomes one-based.

This comment isn't really english and doesn't make any sense to me.  I think we could just remove it.
Comment 7 Joone Hur 2010-12-28 15:29:09 PST
Created attachment 77584 [details]
Proposed Patch3

The comment has been removed. Thanks!
Comment 8 WebKit Commit Bot 2010-12-28 17:34:55 PST
Comment on attachment 77584 [details]
Proposed Patch3

Rejecting attachment 77584 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
........................
fast/multicol/span ............
fast/overflow ...............................................
fast/parser ..................................................................................................................................
fast/parser/xhtml-alternate-entities.xml -> failed

Exiting early after 1 failures. 15360 tests run.
288.85s total testing time

15359 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
6 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7206236
Comment 9 WebKit Commit Bot 2010-12-28 18:01:30 PST
Comment on attachment 77584 [details]
Proposed Patch3

Rejecting attachment 77584 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
........................
fast/multicol/span ............
fast/overflow ...............................................
fast/parser ..................................................................................................................................
fast/parser/xhtml-alternate-entities.xml -> failed

Exiting early after 1 failures. 15360 tests run.
282.30s total testing time

15359 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
6 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7272234
Comment 10 Joone Hur 2011-01-02 17:16:05 PST
Created attachment 77785 [details]
Proposed Patch4

Fixed the DRT test fail.

XML Parser reports only the first parsing error even if there are many errors on the same line.
However, the latest patch reports all errors on the same line, so I fixed the patch as follows,

bool operator==(const TextPosition& other) { return m_line == other.m_line && m_column == other.m_column; }
bool operator!=(const TextPosition& other) { return !((*this) == other); }

=> 

bool operator==(const TextPosition& other) { return m_line == other.m_line && m_column == other.m_column; }
bool operator!=(const TextPosition& other) { return m_line != other.m_line && m_column != other.m_column; }
Comment 11 Peter Rybin 2011-01-02 19:13:23 PST
(In reply to comment #10)
> Created an attachment (id=77785) [details]
> Proposed Patch4
> 
> Fixed the DRT test fail.
> 
> XML Parser reports only the first parsing error even if there are many errors on the same line.
> However, the latest patch reports all errors on the same line, so I fixed the patch as follows,
> 
> bool operator==(const TextPosition& other) { return m_line == other.m_line && m_column == other.m_column; }
> bool operator!=(const TextPosition& other) { return !((*this) == other); }
> 
> => 
> 
> bool operator==(const TextPosition& other) { return m_line == other.m_line && m_column == other.m_column; }
> bool operator!=(const TextPosition& other) { return m_line != other.m_line && m_column != other.m_column; }

This change looks extremely suspicious. It alters semantics of operator "!=" quite unexpectedly.

My guess is that the original code contains wrong boolean operator by mistake (ignore the error if it is at the same COLUMN as the previous one 100 lines above?). Anyway it was committed in r7094 in year 2004 so it's hard to find out for sure.

I would propose we either:
- consider the current code as a mistake and fix test expectations (do show all errors) or
- keep the original logic in new terms:
   m_lastErrorPosition.m_line != position.m_line && m_lastErrorPosition.m_column != position.m_column
   (TextPosition may or may not retain its now-unused operator)
Comment 12 Joone Hur 2011-01-02 23:49:50 PST
(In reply to comment #11)

> 
> I would propose we either:
> - consider the current code as a mistake and fix test expectations (do show all errors) or
> - keep the original logic in new terms:
>    m_lastErrorPosition.m_line != position.m_line && m_lastErrorPosition.m_column != position.m_column
>    (TextPosition may or may not retain its now-unused operator)

Firefox also just shows the first error, so I think the second solution would be better.
Comment 13 Joone Hur 2011-01-02 23:52:42 PST
Created attachment 77792 [details]
Proposed Patch5
Comment 14 Joone Hur 2011-01-05 00:47:41 PST
Is this patch okay to land?
Comment 15 Eric Seidel (no email) 2011-01-05 10:17:43 PST
Comment on attachment 77792 [details]
Proposed Patch5

Looks OK to me.
Comment 16 Peter Rybin 2011-01-05 10:56:22 PST
(In reply to comment #13)
> Created an attachment (id=77792) [details]
> Proposed Patch5

Looks good to me.
Comment 17 WebKit Commit Bot 2011-01-05 17:54:26 PST
Comment on attachment 77792 [details]
Proposed Patch5

Rejecting attachment 77792 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
ml .............................
fast/images .........................................................
fast/inline-block ..............
fast/inline ..............................
fast/innerHTML ................
fast/inspector-support ......
fast/invalid .......................
fast/invalid/junk-data.xml -> failed

Exiting early after 1 failures. 8935 tests run.
205.19s total testing time

8934 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
6 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7248412
Comment 18 Joone Hur 2011-01-06 07:40:01 PST
Created attachment 78119 [details]
Proposed Patch6

fast/invalid/junk-data.xml -> failed 
The above fail has been fixed by using belowBase() instead of base() to initialize to 0.
Comment 19 Eric Seidel (no email) 2011-01-06 11:09:17 PST
Comment on attachment 78119 [details]
Proposed Patch6

I'm not sure I see any change between this and the last diff, but OK.
Comment 20 WebKit Commit Bot 2011-01-06 12:53:38 PST
Comment on attachment 78119 [details]
Proposed Patch6

Clearing flags on attachment: 78119

Committed r75188: <http://trac.webkit.org/changeset/75188>
Comment 21 WebKit Commit Bot 2011-01-06 12:53:45 PST
All reviewed patches have been landed.  Closing bug.