Bug 42943 – WebKit fails to compile when building with WML support enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

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'

[Joone Hur]

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]

Created attachment 75915 [details]
Proposed Patch

It fixes this build error.

[Eric Seidel (no email)]

Comment on attachment 75915 [details]
Proposed Patch

OK.

[Eric Seidel (no email)]

Comment on attachment 75915 [details]
Proposed Patch

Actually... Why is this needed?

[Eric Seidel (no email)]

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

[Joone Hur]

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

Can you elaborate?

[Joone Hur]

Created attachment 76335 [details]
Proposed Patch2

I'm not sure if I use TextPosition in a right way.

[Eric Seidel (no email)]

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

Peter added the TextPosition stuff.

[Peter Rybin]

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

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]

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

The commit-queue encountered the following flaky tests while processing attachment 76335 [details]:

inspector/elements-panel-styles.html bug 50987 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 76335 [details]
Proposed Patch2

Clearing flags on attachment: 76335

Committed r73975: <http://trac.webkit.org/changeset/73975>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joone Hur]

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

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]

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

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

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

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