Bug 41186 - Make DocumentParser API private on subclasses to catch misuse bugs
Summary: Make DocumentParser API private on subclasses to catch misuse bugs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-24 17:37 PDT by Eric Seidel (no email)
Modified: 2010-07-27 22:30 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.02 KB, patch)
2010-06-24 18:00 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Got rid of reference hackery (12.82 KB, patch)
2010-06-24 21:53 PDT, Eric Seidel (no email)
abarth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-06-24 17:37:19 PDT
Make DocumentParser API private on subclasses to catch misuse bugs
Comment 1 Eric Seidel (no email) 2010-06-24 18:00:46 PDT
Created attachment 59715 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-06-24 21:53:54 PDT
Created attachment 59731 [details]
Got rid of reference hackery
Comment 3 Adam Barth 2010-06-24 21:56:17 PDT
Comment on attachment 59731 [details]
Got rid of reference hackery

Much better.  Thank you.  :)
Comment 4 Eric Seidel (no email) 2010-06-25 00:10:23 PDT
Committed r61835: <http://trac.webkit.org/changeset/61835>
Comment 5 Daniel Bates 2010-07-24 21:55:45 PDT
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.
Comment 6 Daniel Bates 2010-07-24 22:04:19 PDT
(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.
Comment 7 WebKit Commit Bot 2010-07-24 22:11:29 PDT
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
Comment 8 Eric Seidel (no email) 2010-07-25 08:17:56 PDT
(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.
Comment 9 Nikolas Zimmermann 2010-07-26 22:44:38 PDT
(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.