Bug 42943 - WebKit fails to compile when building with WML support enabled
Summary: WebKit fails to compile when building with WML support enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-24 22:19 PDT by Daniel Bates
Modified: 2010-12-25 13:57 PST (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 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'
Comment 1 Joone Hur 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
Comment 2 Joone Hur 2010-12-08 09:37:37 PST
Created attachment 75915 [details]
Proposed Patch

It fixes this build error.
Comment 3 Eric Seidel (no email) 2010-12-12 02:08:44 PST
Comment on attachment 75915 [details]
Proposed Patch

OK.
Comment 4 Eric Seidel (no email) 2010-12-12 02:10:23 PST
Comment on attachment 75915 [details]
Proposed Patch

Actually... Why is this needed?
Comment 5 Eric Seidel (no email) 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
Comment 6 Joone Hur 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.
Comment 7 Eric Seidel (no email) 2010-12-12 10:28:04 PST
Can you elaborate?
Comment 8 Joone Hur 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.
Comment 9 Eric Seidel (no email) 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.
Comment 10 Eric Seidel (no email) 2010-12-13 01:35:14 PST
Peter added the TextPosition stuff.
Comment 11 Peter Rybin 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.
Comment 12 Peter Rybin 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
Comment 13 Joone Hur 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;
Comment 14 Eric Seidel (no email) 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.
Comment 15 WebKit Review Bot 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.
Comment 16 WebKit Review Bot 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>
Comment 17 WebKit Review Bot 2010-12-13 15:21:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Joone Hur 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?
Comment 19 Peter Rybin 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.
Comment 20 Peter Rybin 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
Comment 21 Joone Hur 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
Comment 22 Joone Hur 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
Comment 23 Peter Rybin 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).