1344976 - Further remove xml:base usage in FeedWriter

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, enhancement, P3)

Description

[Xidorn Quan [:xidorn] UTC+11]

It seems to me there isn't anything wrong happens with this removal as well.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1344976 - Remove xml:base usage in FeedWriter.

Review commit: https://reviewboard.mozilla.org/r/117796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117796/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8844277 [details]
Bug 1344976 - Remove xml:base usage in FeedWriter.

https://reviewboard.mozilla.org/r/117796/#review119576

I'm not convinced this is righteous on its own. Some of the relevant code here lives in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/feeds/FeedProcessor.js . There we're still processing xml:base, we just don't seem to use Node/Entry.base in a lot of cases, 'privately' resolving a lot of the URIs. Your code now ignores that property, but it's still being set in some cases.

There is also a test that this works correctly, and background in bug 436801 suggests that there were changes in xml:base behavior implemented there, too. Though, to be honest, looking at the test I doubt it actually verifies that the xml:base behavior is correct.

I don't know if we can remove support for xml:base from the feedprocessor code. If we can, we should do so. If we can't, we should presumably still remove the `.base` references in the toolkit code, taking care not to break the actual resolution of URLs in the process (which from a brief look doesn't seem to rely on those properties, but we should check).

Comment 3

[Jonathan Kingston [:jkt] he/him]

I'm going to add this to a patch in Bug 903372. I don't think there are any blockers in removing this now right?

Comment 4

[Jonathan Kingston [:jkt] he/him]

Actually it has been fully removed... closing