Bug 41186 – Make DocumentParser API private on subclasses to catch misuse bugs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Make DocumentParser API private on subclasses to catch misuse bugs

[Eric Seidel (no email)]

Created attachment 59715 [details]
Patch

[Eric Seidel (no email)]

Created attachment 59731 [details]
Got rid of reference hackery

[Adam Barth]

Comment on attachment 59731 [details]
Got rid of reference hackery

Much better.  Thank you.  :)

[Eric Seidel (no email)]

Committed r61835: <http://trac.webkit.org/changeset/61835>

[Daniel Bates]

This change broke WML support by changing the visibility of the functions wellFormed(), lineNumber(), and columnNumber() from public to private. These functions are used in WMLErrorHandling::reportWMLError(). For instance, wellFormed() is called on line 48 <http://trac.webkit.org/browser/trunk/WebCore/wml/WMLErrorHandling.cpp?rev=61104#L48>.

I'm not familiar with this area of the code. Is there an alternative way to get this information? Should we use the NVI idiom here? Or should we re-expose these as public?

CC'ing Nikolas Zimmermann since he worked on the WML code.

[Daniel Bates]

(In reply to comment #5)
> This change broke WML support by changing the visibility of the functions wellFormed(), lineNumber(), and columnNumber() from public to private. These functions are used in WMLErrorHandling::reportWMLError(). For instance, wellFormed() is called on line 48 <http://trac.webkit.org/browser/trunk/WebCore/wml/WMLErrorHandling.cpp?rev=61104#L48>.
> 
> I'm not familiar with this area of the code. Is there an alternative way to get this information? Should we use the NVI idiom here? Or should we re-expose these as public?
> 
> CC'ing Nikolas Zimmermann since he worked on the WML code.

I should be more precise. This change contributed to breaking WML support. WML support is broken in other places as well.

[WebKit Commit Bot]

Comment on attachment 59731 [details]
Got rid of reference hackery

Rejecting patch 59731 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1
Last 500 characters of output:
LegacyHTMLTreeBuilder.cpp
Hunk #1 FAILED at 1648.
Hunk #2 FAILED at 1672.
2 out of 2 hunks FAILED -- saving rejects to file WebCore/html/LegacyHTMLTreeBuilder.cpp.rej
patching file WebCore/loader/DocumentWriter.cpp
Hunk #1 FAILED at 72.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/loader/DocumentWriter.cpp.rej
patching file WebCore/loader/TextDocument.cpp
Hunk #1 FAILED at 44.
Hunk #2 FAILED at 61.
2 out of 2 hunks FAILED -- saving rejects to file WebCore/loader/TextDocument.cpp.rej

Full output: http://queues.webkit.org/results/3617103

[Eric Seidel (no email)]

(In reply to comment #5)
> This change broke WML support by changing the visibility of the functions wellFormed(), lineNumber(), and columnNumber() from public to private. These functions are used in WMLErrorHandling::reportWMLError(). For instance, wellFormed() is called on line 48 <http://trac.webkit.org/browser/trunk/WebCore/wml/WMLErrorHandling.cpp?rev=61104#L48>.

That's incorrect.  wellFormed, lineNumber and columnNumber no longer exist on DocumentParser, only on the subclass ScriptableDocumentParser.

What does WML use for parsing?  Its own DocumentParser subclass?  If so, that should be a subclass of ScriptableDocumentParser.  In any case, WML code should be dealing with more specific pointer types when possible.

Please open a new bug.

Also, WML is broken and really should be ripped out from the tree in its broken, unmaintained, poorly-written state.  If WML ever wants to be a real boy, its going to need a build-bot, as well as some cleanup.

[Nikolas Zimmermann]

(In reply to comment #8)
> (In reply to comment #5)
> > This change broke WML support by changing the visibility of the functions wellFormed(), lineNumber(), and columnNumber() from public to private. These functions are used in WMLErrorHandling::reportWMLError(). For instance, wellFormed() is called on line 48 <http://trac.webkit.org/browser/trunk/WebCore/wml/WMLErrorHandling.cpp?rev=61104#L48>.
> 
> That's incorrect.  wellFormed, lineNumber and columnNumber no longer exist on DocumentParser, only on the subclass ScriptableDocumentParser.
> 
> What does WML use for parsing?  Its own DocumentParser subclass?  If so, that should be a subclass of ScriptableDocumentParser.  In any case, WML code should be dealing with more specific pointer types when possible.
WML is not scriptable, that should be remembered.
> 
> Please open a new bug.
> 
> Also, WML is broken and really should be ripped out from the tree in its broken, unmaintained, poorly-written state.  If WML ever wants to be a real boy, its going to need a build-bot, as well as some cleanup.
WML is unmaintained at the moment, that's true, but it's _not_ poorly written. I rewrote the WML support from scratch, and don't think it contains any poor code, except the parsing bits.