nsWebBrowserPersist::SetDocumentBase() seems to be called twice, which is somewhat surprising. Also, if there is no head and no base tags in a document, a head element is created during the first call. During the second call a head and base element is created. When looking at the serizlied content, there is only the empty head element. I am not totally sure what the method should be doing, but what it is doing now seems also wrong. If the base tag is not supposed to be seriazlied, then we shouldn't create an empty head tag either, right? Or if we are concerned about correctness, then we should create head AND title element inside of it. I became aware of this while working on bug 146062.

I'll investigate to see if the base unsetting/setting can be replaced by adding code to CloneNodeWithFixedUpURIAttributes to replace base elements with an empty comment as the document is persisted.

responding to comment 8 from bug 146062 >------- Additional Comment #8 From Adam Lock 2002-11-13 13:52 ------- > >As I commented when I realised before, the removal of base tags is actually >deliberate during persistence which is why I think this patch needs more >consideration. The base tags are removed because it hoses relative links in the >file. For example, if the HTML had a base tag pointing to >http://www.mozilla.org then an img tag fixed up to point to >"mozilla.org_files/image.gif" would resolve to >"http://www.mozilla.org/mozilla.org_files/image.gif" which is wrong. Maybe instead we need a check to see if the base tag is valid for the destination? If I am viewing and saving local files, it's not clear to me that the base tag should be altered (or viewing and publishing remove files). Perhaps I need to think about this some more.

There is a flag to prevent the base tag being changed, so the caller is able to override this behaviour.

This patch removes the need to call SetDocumentBase to remove and then reinstate the base tag. Instead CloneNodeWithFixedUpURIAttributes tests for base elements and replaces them with an empty comment. There is still one place in the code where SetDocumentBase is justified so the function remains to deal with it - where nsIWebBrowserPersist::SaveDocument is called to save a document but not its data. This ensures that relative links in the document still function properly. I've updated SetDocumentBase to remove the base removing code, it only adds it now. I haven't considered XHTML at all since the other bug deals with that.

BTW, would it be better to use a text node? Can that represent whitespace in the document or is it likely to do something wacky such as insert a CDATA block?

Patch updated after XML changes. It replaces the BASE element (if there is one) with an empty comment. I'm not sure if QI'ing for nsIDOMHTMLBaseElement will work with the XML case. Is there something else I should use?

Yes, QI works for XHTML elements because they use the same HTML implementation. But why empty comments, wouldn't it be more informative to put the base tag into a comment, maybe just cut the < and > out?

Patch turns base element into a comment like this, <!-- base href="foo" -->

Comment on attachment 107088 [details] [diff] [review] Patch v3 Can I have an r/sr on this please?

Comment on attachment 107088 [details] [diff] [review] Patch v3 I'm not a super-reviewer; swap with heikki

Comment on attachment 107088 [details] [diff] [review] Patch v3 r=brade

Comment on attachment 107088 [details] [diff] [review] Patch v3 >Index: mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp >=================================================================== >+ commentText += NS_LITERAL_STRING("href=\""); >+ commentText += href; >+ commentText += NS_LITERAL_STRING("\" "); The above would be more effective like this: >+ commentText += NS_LITERAL_STRING("href=\"") >+ + href >+ + NS_LITERAL_STRING("\" ");

Fix is checked in with Heikki's change

Verified in 2003-01-15-08 trunk builds under Win XP and OS X 10.2.3.