Bug 42943

Summary:

WebKit fails to compile when building with WML support enabled

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, charles.wei, eric, gustavo, joone, peter.rybin, ryuan.choi, staikos, webkit.review.bot, xan.lopez, yichao.yin, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Proposed Patch	none
Proposed Patch2	none
extending handleError signature -- not a regular patch	none

Daniel Bates

Reported 2010-07-24 22:19:04 PDT

I get the following compile-time errors when building WebKit with WML support enabled: /Users/dbates/Desktop/WebKit/WebCore/wml/WMLCardElement.cpp: In static member function 'static WebCore::WMLCardElement* WebCore::WMLCardElement::findNamedCardInDocument(WebCore::Document*, const WebCore::String&)': /Users/dbates/Desktop/WebKit/WebCore/wml/WMLCardElement.cpp:290: error: 'class WebCore::WMLCardElement' has no member named 'getIDAttribute' /Users/dbates/Desktop/WebKit/WebCore/wml/WMLDocument.cpp: In constructor 'WebCore::WMLDocument::WMLDocument(WebCore::Frame*)': /Users/dbates/Desktop/WebKit/WebCore/wml/WMLDocument.cpp:39: error: no matching function for call to 'WebCore::Document::Document(WebCore::Frame*&, bool, bool)' /Users/dbates/Desktop/WebKit/WebCore/dom/Document.h:1009: note: candidates are: WebCore::Document::Document(WebCore::Frame*, const WebCore::KURL&, bool, bool) /Users/dbates/Desktop/WebKit/WebCore/dom/Document.h:189: note: WebCore::Document::Document(const WebCore::Document&) /Users/dbates/Desktop/WebKit/WebCore/wml/WMLDocument.cpp: In member function 'virtual void WebCore::WMLDocument::finishedParsing()': /Users/dbates/Desktop/WebKit/WebCore/wml/WMLDocument.cpp:51: error: 'class WebCore::DocumentParser' has no member named 'wellFormed' The following errors are related to Bug #41186: /Users/dbates/Desktop/WebKit/WebCore/dom/XMLDocumentParser.h: In function 'void WebCore::reportWMLError(WebCore::Document*, WebCore::WMLErrorCode)': /Users/dbates/Desktop/WebKit/WebCore/dom/XMLDocumentParser.h:103: error: 'virtual bool WebCore::XMLDocumentParser::wellFormed() const' is private /Users/dbates/Desktop/WebKit/WebCore/wml/WMLErrorHandling.cpp:48: error: within this context /Users/dbates/Desktop/WebKit/WebCore/dom/XMLDocumentParser.h:104: error: 'virtual int WebCore::XMLDocumentParser::lineNumber() const' is private /Users/dbates/Desktop/WebKit/WebCore/wml/WMLErrorHandling.cpp:51: error: within this context /Users/dbates/Desktop/WebKit/WebCore/dom/XMLDocumentParser.h:105: error: 'virtual int WebCore::XMLDocumentParser::columnNumber() const' is private /Users/dbates/Desktop/WebKit/WebCore/wml/WMLErrorHandling.cpp:51: error: within this context /Users/dbates/Desktop/WebKit/WebCore/wml/WMLGoElement.cpp: In member function 'void WebCore::WMLGoElement::preparePOSTRequest(WebCore::ResourceRequest&, bool, const WebCore::String&)': /Users/dbates/Desktop/WebKit/WebCore/wml/WMLGoElement.cpp:158: error: 'FormDataBuilder' has not been declared /Users/dbates/Desktop/WebKit/WebCore/wml/WMLGoElement.cpp: In member function 'WTF::PassRefPtr<WebCore::FormData> WebCore::WMLGoElement::createFormData(const WTF::CString&)': /Users/dbates/Desktop/WebKit/WebCore/wml/WMLGoElement.cpp:191: error: 'FormDataBuilder' has not been declared /Users/dbates/Desktop/WebKit/WebCore/wml/WMLGoElement.cpp:202: error: 'FormDataBuilder' has not been declared /Users/dbates/Desktop/WebKit/WebCore/wml/WMLGoElement.cpp:203: error: 'FormDataBuilder' has not been declared /Users/dbates/Desktop/WebKit/WebCore/wml/WMLGoElement.cpp:211: error: 'FormDataBuilder' has not been declared /Users/dbates/Desktop/WebKit/WebCore/wml/WMLGoElement.cpp:215: error: 'FormDataBuilder' has not been declared /Users/dbates/Desktop/WebKit/WebCore/wml/WMLInputElement.cpp:140: warning: unused parameter 'sendChangeEvent' /Users/dbates/Desktop/WebKit/WebCore/wml/WMLInputElement.cpp:159: warning: unused parameter 'value'

Attachments
Proposed Patch (1.42 KB, patch) 2010-12-08 09:37 PST, Joone Hur	no flags	Details Formatted Diff Diff
Proposed Patch2 (2.55 KB, patch) 2010-12-12 14:43 PST, Joone Hur	no flags	Details Formatted Diff Diff
extending handleError signature -- not a regular patch (6.17 KB, patch) 2010-12-13 09:36 PST, Peter Rybin	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Joone Hur

Comment 1 2010-12-08 09:34:41 PST

Now, it still failed to compile with WML as follows, CXX WebCore/dom/libwebkitgtk_1_0_la-Document.lo CXX WebCore/dom/libwebkitgtk_1_0_la-DocumentFragment.lo CXX WebCore/dom/libwebkitgtk_1_0_la-ProcessingInstruction.lo CXX WebCore/dom/libwebkitgtk_1_0_la-XMLDocumentParser.lo CXX WebCore/dom/libwebkitgtk_1_0_la-XMLDocumentParserLibxml2.lo CXX WebCore/loader/libwebkitgtk_1_0_la-FrameLoader.lo CXX WebCore/svg/libwebkitgtk_1_0_la-SVGUseElement.lo CXX WebCore/wml/libwebkitgtk_1_0_la-WMLErrorHandling.lo ../../WebCore/dom/XMLDocumentParser.h: In function ‘void WebCore::reportWMLError(WebCore::Document*, WebCore::WMLErrorCode)’: ../../WebCore/dom/XMLDocumentParser.h:132: error: ‘int WebCore::XMLDocumentParser::columnNumber() const’ is private ../../WebCore/wml/WMLErrorHandling.cpp:51: error: within this context make[1]: *** [WebCore/wml/libwebkitgtk_1_0_la-WMLErrorHandling.lo] Error 1

Joone Hur

Comment 2 2010-12-08 09:37:37 PST

Created attachment 75915 [details] Proposed Patch It fixes this build error.

Eric Seidel (no email)

Comment 3 2010-12-12 02:08:44 PST

Comment on attachment 75915 [details] Proposed Patch OK.

Eric Seidel (no email)

Comment 4 2010-12-12 02:10:23 PST

Comment on attachment 75915 [details] Proposed Patch Actually... Why is this needed?

Eric Seidel (no email)

Comment 5 2010-12-12 02:12:13 PST

TextPosition is the new way to do this: http://trac.webkit.org/browser/trunk/WebCore/dom/ScriptableDocumentParser.h

Joone Hur

Comment 6 2010-12-12 04:28:21 PST

(In reply to comment #5) > TextPosition is the new way to do this: > http://trac.webkit.org/browser/trunk/WebCore/dom/ScriptableDocumentParser.h It doesn't fix this problem.

Eric Seidel (no email)

Comment 7 2010-12-12 10:28:04 PST

Can you elaborate?

Joone Hur

Comment 8 2010-12-12 14:43:07 PST

Created attachment 76335 [details] Proposed Patch2 I'm not sure if I use TextPosition in a right way.

Eric Seidel (no email)

Comment 9 2010-12-13 01:34:32 PST

Comment on attachment 76335 [details] Proposed Patch2 I'm confused why you're moving lineNumber and columnNumber? Neither of these need to be public anymore. textPosition is the new way to do this all I believe.

Eric Seidel (no email)

Comment 10 2010-12-13 01:35:14 PST

Peter added the TextPosition stuff.

Peter Rybin

Comment 11 2010-12-13 07:52:26 PST

(In reply to comment #10) > Peter added the TextPosition stuff. The idea behind TextPosition is to keep line/column pair together and what's more important to take care of 0-based/1-based numbers. Incidentally, such issue seems to be hidden in "Patch2": actual arguments of handleError are explicitly 0-based, while this method treats them as 1-based numbers (in user messages all numbers are expected to be 1-based I believe). I propose we adding TextPosition to handleError signature. Let me prepare a patch about this real soon.

Peter Rybin

Comment 12 2010-12-13 09:36:12 PST

Created attachment 76395 [details] extending handleError signature -- not a regular patch I propose this additional patch -- switches handleError method to TextPosition1. This patch does not fix the bug itself, only helps. Joone, could you please try if it works for you? If it works, you can merge it with your change. I guess you will also need to convert your TextPosition0 (zero-based value) to TextPosition1 (one-based value expected in handleError), WTF::toOneBasedTextPosition can be used for this. Peter

Joone Hur

Comment 13 2010-12-13 14:37:48 PST

(In reply to comment #9) > (From update of attachment 76335 [details]) > I'm confused why you're moving lineNumber and columnNumber? Neither of these need to be public anymore. textPosition is the new way to do this all I believe. Yes, I know. I made them private as follows, private: XMLDocumentParser(Document*, FrameView* = 0); XMLDocumentParser(DocumentFragment*, Element*, FragmentScriptingPermission); // From DocumentParser virtual void insert(const SegmentedString&); virtual void append(const SegmentedString&); virtual void finish(); virtual bool finishWasCalled(); virtual bool isWaitingForScripts() const; virtual void stopParsing(); virtual void detach(); virtual int lineNumber() const; int columnNumber() const;

Eric Seidel (no email)

Comment 14 2010-12-13 14:41:14 PST

Comment on attachment 76335 [details] Proposed Patch2 This seems OK. Peter's fix is definitely better, and it would be good to follow up with a patch which does peter's proposed refactoring.

WebKit Review Bot

Comment 15 2010-12-13 15:19:14 PST

The commit-queue encountered the following flaky tests while processing attachment 76335 [details]: inspector/elements-panel-styles.html bug 50987 (author: pfeldman@chromium.org) The commit-queue is continuing to process your patch.

WebKit Review Bot

Comment 16 2010-12-13 15:21:15 PST

Comment on attachment 76335 [details] Proposed Patch2 Clearing flags on attachment: 76335 Committed r73975: <http://trac.webkit.org/changeset/73975>

WebKit Review Bot

Comment 17 2010-12-13 15:21:22 PST

All reviewed patches have been landed. Closing bug.

Joone Hur

Comment 18 2010-12-16 15:10:09 PST

(In reply to comment #12) > Created an attachment (id=76395) [details] > extending handleError signature -- not a regular patch > > I propose this additional patch -- switches handleError method to TextPosition1. This patch does not fix the bug itself, only helps. > > Joone, could you please try if it works for you? If it works, you can merge it with your change. I guess you will also need to convert your TextPosition0 (zero-based value) to TextPosition1 (one-based value expected in handleError), WTF::toOneBasedTextPosition can be used for this. > > Peter I tried to use TextPosition1 as follows, parser->handleError(XMLDocumentParser::fatal, errorMessage.latin1().data(), toOneBasedTextPosition(parser->textPosition()).m_line.oneBasedInt(), toOneBasedTextPosition(parser->textPosition()).m_column.oneBasedInt()); It gives us wrong error report. For example, 16 <card id="card2" title="Answer"> *17 <p>You selected: $($$name)</p> 18 </card> 19 </wml 17 line has an error, so if we use TextPosition1, it reports the following error, "This page contains the following errors: error on line 18 at column 36: A variable reference uses invalid syntax. Below is a rendering of the page up to the first error." I guess that the error should be on line 17 at column 26. If we use TextPosition0, it reports, "This page contains the following errors: error on line 17 at column 35: A variable reference uses invalid syntax. Below is a rendering of the page up to the first error." It seems right except the column number, but actually, is line 16 right if we use TextPosition0?

Peter Rybin

Comment 19 2010-12-16 15:43:56 PST

Oh, I see. I gets a little more complicated that I thought. I remember I didn't quite complete with refactoring in XML parser because I noticed an inconsistency around lineNumber/columnNumber methods: in some cases their result was used as 0-based, in other cases it was clearly used as 1-based. The problem was I didn't know which is correct. So I put TextPosition there, but I retained inconsistency by adding an explicit casting code. What I did was the following: XMLDocumentParser::textPosition method expects lineNumber/columnNumber works as a wrapper for lineNumber/columnNumber methods. Since most of the code expected lineNumber/columnNumber to return 0-based values, textPosition method takes them as 0-based. Now I remember I had to add a helper method textPositionOneBased(). It returns same numbers, but treats them as 1-based; probably it should work for you. Also it would be of a great help if you could tell what XMLDocumentParser::lineNumber/XMLDocumentParser::columnNumber result actually is: 0-based or 1-based. Please add a comment in this case. However it's a bit complicated by the fact there are 2 different implementations of the parser: "XMLDocumentParserQT.cpp" and "XMLDocumentParserLibxml2.cpp" Adding annotation to at least one of these files will help fixing it in the future. > I tried to use TextPosition1 as follows, > > parser->handleError(XMLDocumentParser::fatal, errorMessage.latin1().data(), toOneBasedTextPosition(parser->textPosition()).m_line.oneBasedInt(), toOneBasedTextPosition(parser->textPosition()).m_column.oneBasedInt()); > > It gives us wrong error report. > > For example, > > 16 <card id="card2" title="Answer"> > *17 <p>You selected: $($$name)</p> > 18 </card> > 19 </wml > > 17 line has an error, so if we use TextPosition1, it reports the following error, > > "This page contains the following errors: > error on line 18 at column 36: A variable reference uses invalid syntax. > Below is a rendering of the page up to the first error." > > I guess that the error should be on line 17 at column 26. > > If we use TextPosition0, it reports, > > "This page contains the following errors: > error on line 17 at column 35: A variable reference uses invalid syntax. > Below is a rendering of the page up to the first error." > > It seems right except the column number, but actually, is line 16 right if we use TextPosition0? Column number looks quite weird, I have no idea about it. You are right, TextPosition0 should contain 16. This is because of inconsistency that I retained to keep refactoring accurate.

Peter Rybin

Comment 20 2010-12-16 15:46:18 PST

> What I did was the following: > XMLDocumentParser::textPosition method expects lineNumber/columnNumber works as a wrapper for lineNumber/columnNumber methods. Since most of the code expected lineNumber/columnNumber to return 0-based values, textPosition method takes them as 0-based. Sorry, I meant: "XMLDocumentParser::textPosition method works as a wrapper for lineNumber/columnNumber methods." And thank you for your investigation. Peter

Joone Hur

Comment 21 2010-12-24 17:45:52 PST

(In reply to comment #19) > Also it would be of a great help if you could tell what XMLDocumentParser::lineNumber/XMLDocumentParser::columnNumber result actually is: 0-based or 1-based. Please add a comment in this case. However it's a bit complicated by the fact there are 2 different implementations of the parser: "XMLDocumentParserQT.cpp" and "XMLDocumentParserLibxml2.cpp" > > Adding annotation to at least one of these files will help fixing it in the future. XMLDocumentParser uses 1-based values, so I filed a new bug https://bugs.webkit.org/show_bug.cgi?id=5160

Joone Hur

Comment 22 2010-12-24 17:46:29 PST

(In reply to comment #21) > (In reply to comment #19) > > > Also it would be of a great help if you could tell what XMLDocumentParser::lineNumber/XMLDocumentParser::columnNumber result actually is: 0-based or 1-based. Please add a comment in this case. However it's a bit complicated by the fact there are 2 different implementations of the parser: "XMLDocumentParserQT.cpp" and "XMLDocumentParserLibxml2.cpp" > > > > Adding annotation to at least one of these files will help fixing it in the future. > > XMLDocumentParser uses 1-based values, so I filed a new bug https://bugs.webkit.org/show_bug.cgi?id=5160 https://bugs.webkit.org/show_bug.cgi?id=51601

Peter Rybin

Comment 23 2010-12-25 13:57:17 PST

>> complicated by the fact there are 2 different implementations of the parser: >> "XMLDocumentParserQT.cpp" and "XMLDocumentParserLibxml2.cpp" > XMLDocumentParser uses 1-based values, so I filed a new bug https://bugs.webkit.org/show_bug.cgi?id=5160 To me your patch is fine. Thank you for checking with XML parser; however, to be accurate I need to know, whether it was "Libxml2" or "QT" version. (The brutal way to check would be to spoil both "XMLDocumentParserQT.cpp" and "XMLDocumentParserLibxml2.cpp" and see which one emits compile error in your build configuration).

Note You need to log in before you can comment on or make changes to this bug.