Closed
Bug 179990
Opened 22 years ago
Closed 22 years ago
nsWebBrowserPersist::SetDocumentBase() weirdness
Categories
(Core Graveyard :: File Handling, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: hjtoi-bugzilla, Assigned: adamlock)
Details
Attachments
(2 files, 2 obsolete files)
299 bytes,
text/html
|
Details | |
4.10 KB,
patch
|
Brade
:
review+
hjtoi-bugzilla
:
superreview+
|
Details | Diff | Splinter Review |
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.
Comment 2•22 years ago
|
||
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?
Attachment #106142 -
Attachment is obsolete: true
Reporter | ||
Comment 8•22 years ago
|
||
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" -->
Attachment #107074 -
Attachment is obsolete: true
Assignee | ||
Comment 10•22 years ago
|
||
Comment on attachment 107088 [details] [diff] [review]
Patch v3
Can I have an r/sr on this please?
Attachment #107088 -
Flags: superreview?(brade)
Attachment #107088 -
Flags: review?(heikki)
Comment 11•22 years ago
|
||
Comment on attachment 107088 [details] [diff] [review]
Patch v3
I'm not a super-reviewer; swap with heikki
Attachment #107088 -
Flags: superreview?(heikki)
Attachment #107088 -
Flags: superreview?(brade)
Attachment #107088 -
Flags: review?(heikki)
Attachment #107088 -
Flags: review?(brade)
Comment 12•22 years ago
|
||
Comment on attachment 107088 [details] [diff] [review]
Patch v3
r=brade
Attachment #107088 -
Flags: review?(brade) → review+
Reporter | ||
Comment 13•22 years ago
|
||
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("\" ");
Attachment #107088 -
Flags: superreview?(heikki) → superreview+
Assignee | ||
Comment 14•22 years ago
|
||
Fix is checked in with Heikki's change
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 15•22 years ago
|
||
Verified in 2003-01-15-08 trunk builds under Win XP and OS X 10.2.3.
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•