Bug 51601

Created attachment 77432 [details] Proposed Patch This patch includes https://bugs.webkit.org/attachment.cgi?id=76395

(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 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.

> > 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.

Created attachment 77516 [details] Proposed Patch2 How about using "allow" instead of "legalize"?

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.

Created attachment 77584 [details] Proposed Patch3 The comment has been removed. Thanks!

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 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

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; }

(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)

(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.

Created attachment 77792 [details] Proposed Patch5

Is this patch okay to land?

Comment on attachment 77792 [details] Proposed Patch5 Looks OK to me.

(In reply to comment #13) > Created an attachment (id=77792) [details] > Proposed Patch5 Looks good to me.

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

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 on attachment 78119 [details] Proposed Patch6 I'm not sure I see any change between this and the last diff, but OK.

Comment on attachment 78119 [details] Proposed Patch6 Clearing flags on attachment: 78119 Committed r75188: <http://trac.webkit.org/changeset/75188>

All reviewed patches have been landed. Closing bug.