553121 - Use resource urls to refer to dtds instead of file urls

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Michael Wu [:mwu]]

Attachment: Use resource URLs instead of file URLs

Split from bug 552121.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 433216 [details] [diff] [review]
Use resource URLs instead of file URLs

(switching sr to jst)

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 433216 [details] [diff] [review]
Use resource URLs instead of file URLs

Seems like this looses the purpose of what this function does since we no longer check whether or not the file exists, and if it doesn't exist it sure isn't loadable. Changing review request to peterv here, as he probably knows this stuff the best. It could very well be that the DTD load fails later on in a graceful way and this change would be fine, but at the very least we should change the method name and the comment explaining what this function does now, if checking for file existence isn't necessary.

Peter, what's your thoughts here?

Comment 3

[Michael Wu [:mwu]]

(In reply to comment #2)
> (From update of attachment 433216 [details] [diff] [review])
> Seems like this looses the purpose of what this function does since we no
> longer check whether or not the file exists, and if it doesn't exist it sure
> isn't loadable.
Well, we just move the check for whether the file exists to open time. If the URI can't be opened OpenInputStreamFromExternalDTD fails. Basically the same behavior should be maintained.

> Changing review request to peterv here, as he probably knows
> this stuff the best. It could very well be that the DTD load fails later on in
> a graceful way and this change would be fine, but at the very least we should
> change the method name and the comment explaining what this function does now,
> if checking for file existence isn't necessary.
> 
Agreed, a name change + comment would make sense.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 433216 [details] [diff] [review]
Use resource URLs instead of file URLs

Ok, fair enough. And I'm fine with taking this change as long as we rename the method and update the comment.

Comment 5

[Michael Wu [:mwu]]

Attachment: Use resource URLs instead of file URLs, v2

How's this?

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 436603 [details] [diff] [review]
Use resource URLs instead of file URLs, v2

Looks good.

Comment 7

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/fa6d38d214ee