Bug 43180 - Change permission to access methods in XMLDocumentParser.h
Summary: Change permission to access methods in XMLDocumentParser.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-29 01:51 PDT by Gyuyoung Kim
Modified: 2019-02-06 09:02 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.86 KB, patch)
2010-07-29 01:54 PDT, Gyuyoung Kim
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Patch (1.85 KB, patch)
2010-08-04 17:53 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch for commit (1.85 KB, patch)
2010-08-05 18:36 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2010-07-29 01:51:27 PDT
When WML is enabled, there are build breaks by 3 methods of XMLDocumentParser.h.
The methods are wellFormed(), lineNumber() and columnNumber().

These methods is private accessibility. However, WMLErrorHandling.cpp file is using the methods.
I am not sure if this accessibility change is correct or not. If we can't change the accessibility, 
We need to modify the WMLErrorHandling.cpp file.
Comment 1 Gyuyoung Kim 2010-07-29 01:54:58 PDT
Created attachment 62928 [details]
Patch
Comment 2 Darin Adler 2010-07-29 11:02:37 PDT
Comment on attachment 62928 [details]
Patch

This is not the right fix. If we have to make these public we will, but we should not use conditionals.

Where is the code in WML that uses these? Why is it using an XMLDocumentParser* instead of a DocumentParser*?
Comment 3 Gyuyoung Kim 2010-07-30 03:22:58 PDT
(In reply to comment #2)
> (From update of attachment 62928 [details])
> This is not the right fix. If we have to make these public we will, but we should not use conditionals.
> 
> Where is the code in WML that uses these? Why is it using an XMLDocumentParser* instead of a DocumentParser*?

The reportWMLError() of WMLErrorHandling.cpp is using these functions. It seems that the accessibility was changed by Bug 41114 - Start to clean up DocumentParser interface. But, there are no the reasons in the bug.

Frankly, I don't know why this WMLErrorHandling.cpp uses XMLDocumentParser.cpp yet. I am trying to trace the reason via log.

I suspects that WML guys uses XMLDocumentParser to handle error via these functions.

In my opinion, there are two solutions. One is we change these functions's accessibility with public. As you said, other is that we don't use XMLDocumentParser in WMLErrorHandling.cpp.
Comment 4 Darin Adler 2010-07-30 12:24:14 PDT
(In reply to comment #3)
> In my opinion, there are two solutions. One is we change these functions's accessibility with public. As you said, other is that we don't use XMLDocumentParser in WMLErrorHandling.cpp.

I think making them public is fine.
Comment 5 Gyuyoung Kim 2010-08-04 17:53:49 PDT
Created attachment 63523 [details]
Patch

Sorry for my late response. I was summer vacation.
Ok, I change the accessibility with public. Please review this patch. :)
Comment 6 Darin Adler 2010-08-05 07:27:10 PDT
Comment on attachment 63523 [details]
Patch

> +        // WMLErrorHandling uses these methods.

In future, please use the C++ term, functions, rather than calling these methods.
Comment 7 Gyuyoung Kim 2010-08-05 18:21:59 PDT
(In reply to comment #6)
> (From update of attachment 63523 [details])
> > +        // WMLErrorHandling uses these methods.
> 
> In future, please use the C++ term, functions, rather than calling these methods.

Ok, I keep in mind. Thank you.
Comment 8 Gyuyoung Kim 2010-08-05 18:26:16 PDT
Hello darin,

It seems that reviewed patch doesn't pass style error for gtk. However, I don't know why this patch cannot pass the gtk style error. If there are no style errors, could you commit this patch manually ?

But, if I need to modify this patch, please let me know. Thank you.
Comment 9 Gyuyoung Kim 2010-08-05 18:36:01 PDT
Created attachment 63676 [details]
Patch for commit

Hello Darin,

I change "methods' with "functions" for commit. :)
Comment 10 Darin Adler 2010-08-05 19:35:35 PDT
(In reply to comment #8)
> It seems that reviewed patch doesn't pass style error for gtk.

That’s incorrect. I just reviewed the patch before the GTK EWS bot got a chance to compile it, and once reviewed the bots stop processing a path. There is nothing wrong with the patch.
Comment 11 Darin Adler 2010-08-05 19:36:07 PDT
Comment on attachment 63523 [details]
Patch

Clear the flags on the older patch.
Comment 12 WebKit Commit Bot 2010-08-06 09:43:57 PDT
Comment on attachment 63676 [details]
Patch for commit

Clearing flags on attachment: 63676

Committed r64855: <http://trac.webkit.org/changeset/64855>
Comment 13 WebKit Commit Bot 2010-08-06 09:44:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Lucas Forschler 2019-02-06 09:02:51 PST
Mass moving XML DOM bugs to the "DOM" Component.