192366 - document.lastModified has no properties for XML documents.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Garrett Smith]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2.1) Gecko/20021130

document.lastModified is undefined for documents with application/xhtml+xml or
application/xml. 

Additionally, the lastModified field in info is blank.

Comparison:
http://dhtmlkitchen.com/html/xhtml/sample.html
http://dhtmlkitchen.com/html/xhtml/sample.xml

Reproducible: Always

Steps to Reproduce:
1. Load an XML doc
2. View > Page Info
3. Last Modified is unspecified


Actual Results:  
lastModified is undefined

Expected Results:  
lastModified should apply to any kind of document.

There's no DOM spec for document.lastModified. This property, unlike others
(such as compatMode) can apply to any document, and not only certain types (like
html) of documents.

Why is lastModified only supported for documents implementing HTMLDocument
interface?

Comment 1

[Boris Zbarsky [:bzbarsky]]

We _could_ move this off nsIDOMNSHTMLDocument and onto nsIDOMNSDocument, I
suppose... jst?  sicking?  Thoughts?

Comment 2

[Johnny Stenback (:jst)]

Fine with me.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So what needs to be done here is that the attr needs to move to nsIDOMNSDocument
and the implementation needs to move from nsHTMLDocument (see
SetLastModified/GetLastModified/StartDocumentLoad) into nsDocument.

XUL documents could probably leave this as the empty string for now....

Comment 4

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, now that XULDocument inherits from XMLDocument, all this stuff can just move
into nsDocument and be happy...

Comment 6

[Pratik]

Well my first attempt to move everything to nsDocument didn't work. I did what
comment 3 said and I still don't get a lastModified field for the URL posted.
I'll investigate some more. Meanwhile if anyone has some more thoughts please
let me know.

Comment 7

[Boris Zbarsky [:bzbarsky]]

You moved the StartDocumentLoad code too?

Comment 8

[Pratik]

cvs-mirror doesn't seem to be working so I can't post a patch.

by moving StartDocumentLoad, I hope you're referring to the relevant code in
RetrieveRelevantHeaders() which does the LastModified date stuff. I moved that
to StartDocumentLoad(0 in nsDocument and it fixes the bug. XML files now show
the Last Modified field. However, XUL files do not show it even though the
Response Header for the page has a Last Modified field.

Is this correct or should I be moving more stuff from nsHTMLDocument to nsDocument?

I'll post a rough patch tomorrow once cvs-mirror starts working.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I hope you're referring to the relevant code in
> RetrieveRelevantHeaders() which does the LastModified date stuff.

Yep.  Not all of StartDocumentLoad, of course.  ;)

> However, XUL files do not show it

nsXULDocument::StartDocumentLoad does not call up to the parent classes...
Perhaps it should be calling nsDocument::StartDocumentLoad?

Alternately, we could put RetrieveRelevantHeaders on nsDocument and just make
sure all the ::StartDocumentLoad funcs call it; the HTML document could override
it to fetch extra HTML-specific info.

Comment 10

[Johnny Stenback (:jst)]

Sounds reasonable to me.

Comment 11

[Pratik]

If I move last-modified code RetrieveRelevantHeaders from nsHTMLDocument to
nsDocument, then the only thing left in the nsHTMLDocument version is setting
the referer part.

Here's my proposed solution after reading bz's comments.

1. Add a virtual function (NSIMETHOD?) RetrieveRelevantHeaders in 
   nsDocument.
2. nsDocument::RetrieveRelevantHeaders will have the set-last-modified 
   code.
3. Have the nsHTMLDocument version contains a call to
   nsDocument.RetrieveRelevantHeaders and the set-referer code.
4. Have nsDocument::StartDocumentLoad call RetrieveRelevantHeaders
   (virtual call).
5. Remove call to RetrieveRelevantHeaders from 
   nsHTMLDocument::StartDocumentLoad
6. Make all subclass versions of StartDocumentLoad call 
   RetrieveRelevantHeaders if they don't call nsDocument::StartDocumentLoad.

Does that sound good?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Sounds good; just do a virtual nsresult or virtual void, not NS_IMETHOD (it's
not an interface method).

Comment 13

[Pratik]

What about the referer field? The way I see it now, it seems XML and XUL
documents don't have a referer field. Page Info for XML documents always shows
no referring URL even though LiveHTTPHeaders shows a referring URL. To fix that
I'd have to move the code from nsHTMLDocument to nsDocument. This effectively
means that we don't need RetrieveRelevantHeaders in nsHTMLDocument anymore. I
can just move the entire implementation it to nsDocument. Also, I'd have to move
the setReferrer() functions plus related attributes to nsDocument. Does this
sound like the right thing to do?

Comment 14

[Boris Zbarsky [:bzbarsky]]

"referrer" is a property on the HTMLDocument object in the W3C DOM.  It is _NOT_
present on the Document object in the W3C DOM.  I don't think we want to move
it, particularly.... (if we did, we'd need to put it on nsIDOMNSDocument and do
some sort of magic to the frozen nsIDOMHTMLDocument interface....)

Comment 15

[Pratik]

Attachment: First stab at the bug

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 120840 [details] [diff] [review]
First stab at the bug

+    aLastModified.Assign(NS_LITERAL_STRING("January 1, 1970 GMT"));

maybe "unknown" would be a better idea?

Comment 17

[Pratik]

That part of the code was picked up from nsHTMLDocument.cpp. Since that was the
way it implemented for the very first time (See
<http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsHTMLDocument.cpp&root=/cvsroot&subdir=mozilla/content/html/document/src&command=DIFF_FRAMESET&rev1=3.139&rev2=3.140>),
and it hasn't been touched in more than 2 years, I don't think I'd want to
change it. But I don't know enough about nsDocument/nsHTMLDocument to make an
authoritative statement.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120840 [details] [diff] [review]
First stab at the bug

>Index: content/base/src/nsDocument.cpp
>+#include "nsIFileChannel.h"

Can you take this #include out of nsHTMLDocument?

>@@ -784,38 +785,8 @@
>     CopyASCIItoUCS2(Substring(start, semicolon), mContentType);
>   }
> 
>-  PRBool have_contentLanguage = PR_FALSE;

Is this in StartDocumentLoad? (A hint -- doing diff -u9 is a lot more
reviewable...)

>+  rv = RetrieveRelevantHeaders(aChannel);
>+  NS_ASSERTION(NS_SUCCEEDED(rv), "RetrieveRelevantHeaders failed");

Why is this assertion-worthy?

>+      // if we failed to get a last modification date, then we don't
>+      // want to necessarily fail to create a document for this
>+      // file. Just don't set the last modified date on it...

Is this comment still relevant? In particular, why would we be failing to
create the document here?
>+        SetLastModified(lastModified);

Is there a reason to do this instead of assigning to mLastModified directly?

>Index: content/base/src/nsDocument.h
>+   * Set the last modified time of this document.
>+   */
>+  NS_IMETHOD SetLastModified(const nsAString& aLastModified);

Does anyone other than RetrieveRelevantHeaders actually call this? 

>Index: content/html/document/src/nsHTMLDocument.cpp
>+  // Since nsDocument::RetrieveRelevantHeaders always returns success.
>+  rv = nsDocument::RetrieveRelevantHeaders(aChannel);

Why is it returning nsresult if it always returns success?  It should return
void, then,

>     // The misspelled key 'referer' is as per the HTTP spec
>     rv = mHttpChannel->GetRequestHeader(NS_LITERAL_CSTRING("referer"),

Wouldn't it make sense to move all the header stuff left here into
nsHTMLDocument::RetrieveRelevantHeaders, have StartDocumentLoad call
RetrieveRelevantHeaders, and call up to the base class in
nsHTMLDocument::RetrieveRelevantHeaders?

Comment 19

[Pratik]

>>Index: content/base/src/nsDocument.cpp
>>+#include "nsIFileChannel.h"
> 
> Can you take this #include out of nsHTMLDocument?

Done.
 
>>@@ -784,38 +785,8 @@
>>    CopyASCIItoUCS2(Substring(start, semicolon), mContentType);
>>  }
>>
>>-  PRBool have_contentLanguage = PR_FALSE;
> 
> Is this in StartDocumentLoad? (A hint -- doing diff -u9 is a lot more
> reviewable...)

Yes its in nsDocument::StartDocumentLoad. New patch coming up is diff -u9
 
>>+  rv = RetrieveRelevantHeaders(aChannel);
>>+  NS_ASSERTION(NS_SUCCEEDED(rv), "RetrieveRelevantHeaders failed");
> 
> Why is this assertion-worthy?

Just because the function always returns true and if it changes, we'd want to
know how the failure should be handled. I've made teh function return void now
based on other comments.
 
>>+      // if we failed to get a last modification date, then we don't
>>+      // want to necessarily fail to create a document for this
>>+      // file. Just don't set the last modified date on it...
> 
> Is this comment still relevant? In particular, why would we be failing to
> create the document here?

We shouldn't fail. Which is what the comment says. I could get rid of it if you
want but I know too little about the code to make a judgement.
 
>>+        SetLastModified(lastModified);
> 
> Is there a reason to do this instead of assigning to mLastModified directly?

uumm.. If there is a Set function then I'd rather call that instead of
duplicating its functinoality at other places...
 
>>Index: content/base/src/nsDocument.h
>>+   * Set the last modified time of this document.
>>+   */
>>+  NS_IMETHOD SetLastModified(const nsAString& aLastModified);
> 
> Does anyone other than RetrieveRelevantHeaders actually call this? 

Not that I can find. That seems to be the only place from where its called.
 
>>Index: content/html/document/src/nsHTMLDocument.cpp
>>+  // Since nsDocument::RetrieveRelevantHeaders always returns success.
>>+  rv = nsDocument::RetrieveRelevantHeaders(aChannel);
> 
> Why is it returning nsresult if it always returns success?  It should return
> void, then,

Changed it to return void. 
 
>>    // The misspelled key 'referer' is as per the HTTP spec
>>    rv = mHttpChannel->GetRequestHeader(NS_LITERAL_CSTRING("referer"),
> 
> Wouldn't it make sense to move all the header stuff left here into
> nsHTMLDocument::RetrieveRelevantHeaders, have StartDocumentLoad call
> RetrieveRelevantHeaders, and call up to the base class in
> nsHTMLDocument::RetrieveRelevantHeaders?

I'm not sure I understand this. I can't move the referrer code in nsDocument
because nsDocument doesn't know about referrer. I'd have to do some more surgery
to get referrer in nsDocument (and this should be spawned into a new bug IMO).

Comment 20

[Pratik]

Attachment: 2nd try (diff -u9)

Comment 21

[Boris Zbarsky [:bzbarsky]]

> We shouldn't fail. Which is what the comment says

The comment was written when the code was inline in StartDocumentLoad so that an
early return would cause document creation to fail.  That's hardly relevant now
that the code is in a separate function.

> If there is a Set function then I'd rather call that
>> Does anyone other than RetrieveRelevantHeaders actually call this? 
> Not that I can find. That seems to be the only place from where its called.

In that case, why not just remove it and set mLastModified directly?  Less
confusing, imo....

What I mean with the last part is this.  You would have the following functions.

1) nsDocument::RetrieveRelevantHeaders -- gets last-modified and such
2) nsHTMLDocument::RetrieveRelevantHeaders -- gets referer, then calls
nsDocument::RetrieveRelevantHeaders

both being virtual.  OK?  Now nsDocument::StartDocumentLoad will call
RetrieveRelevantHeaders with no classname on it.  This is called by XML and HTML
documents, so they will automatically get the right RetrieveRelevantHeaders
called (and nsHTMLDocument::StartDocumentLoad does not need to call
RetrieveRelevantHeaders anymore).  nsXULDocument::StartDocumentLoad does not
call nsDocument::StartDocumentLoad, so it would need to call
RetrieveRelevantHeaders explicitly.

Make sense?

Comment 22

[Pratik]

Attachment: 3rd try

Addresses bz's comments in the bug report and on irc. bz, could you take a
closer look at the replacement code for SetLastModified()? Thanks.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120979 [details] [diff] [review]
3rd try

>Index: content/base/src/nsDocument.cpp
>-    if (NS_SUCCEEDED(rv)) {
>-      // XXX what's wrong w/ ASCII?
>-      CopyASCIItoUCS2(contentLanguage, mContentLanguage);
>-
>-      have_contentLanguage = PR_TRUE;
>-    }
>-  }
>-
>-  if (!have_contentLanguage) {
>-    nsCOMPtr<nsIPrefBranch> prefBranch =
>-      do_GetService(NS_PREFSERVICE_CONTRACTID);

Gets replaced by:

>+    rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("Content-Language"),
>+                                        header);
>+
>+    if (NS_SUCCEEDED(rv)) {
>+      // XXX what's wrong w/ ASCII?
>+      CopyASCIItoUCS2(header, mContentLanguage);
>+
>+      nsCOMPtr<nsIPrefBranch> prefBranch =
>+        do_GetService(NS_PREFSERVICE_CONTRACTID);
>+
>+      if (prefBranch) {

which means you're always clobbering the HTTP header with the pref.  You need
an "else" right after that CopyASCIItoUCS2, no?

>+    rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("last-modified"),
>+                                         header);
>+ 
>+    if (NS_SUCCEEDED(rv)) {
>+      mLastModified.Assign(NS_ConvertASCIItoUCS2(header));

CopyASCIItoUCS2(header, mLastModified);

>+        if (NS_SUCCEEDED(rv)) {
>+          mContentLanguage.AssignWithConversion(prefLanguage);

CopyASCIItoUCS2(preflanguage, mContentLanguage);

The rest looks good.  Fix the logic error with the content-language, though. 
;)

Comment 24

[Pratik]

Attachment: 4th try

ooops.. I didn't see the ! in the check for have_contentLanguage. I've reverted
back to the original code.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 120997 [details] [diff] [review]
4th try

- In nsDocument::RetrieveRelevantHeaders():

+  return;
 }

Pointless return at the end of a void function. Remove it.

Other than that, this looks good to me. sr=jst

What about document.referrer? Want to do that as a separate change, or roll it
in here?

Comment 26

[Hixie (not reading bugmail)]

jst: If you could get the WG to move document.referrer, document.cookie,
document.URL and document.domain from the HTML DOM to Core DOM, I think that
would be very useful for authors. There isn't anything HTML-specific about them,
really, and they are commonly used. It seems silly to require authors to get the
HTML document object to get at this data (especially since there might not be
any HTML present in their document).

Comment 27

[Johnny Stenback (:jst)]

Hixie, feel free to post that as an issue on [e-mail], but to be honest, I
doubt they'll make it into the spec. document.URL exists in DOM Core Level 3,
but it's called document.documentURI, but non-browser vendors (which is all but
one of the members of the DOM WG) don't care about those properties. While
document.referrer, document.domain, and document.cookies are not HTML specific,
they are very much browser centric. Therefore, it'll be hard to convince the WG
that there's a strong need for those in DOM Core. But if you make your case on
[e-mail], I'll push for them, but as I said, I'm guessing they won't make
it into the spec.

Comment 28

[Hixie (not reading bugmail)]

jst: Well I don't care that much; the comments above about it making
document.referrer easier to implement are why I suggested it.

Comment 29

[Pratik]

bz, jst: thanks for the review

bz: Could you check this in for me with the minor change that jst requested? Thanks!

jst: I've filed bug 202636 for the document.referrer issue. I'll look into
fixing it sometime next week. I've also CC'd you and Hixie.

Comment 30

[Boris Zbarsky [:bzbarsky]]

I had to also patch embedding/browser/activex/src/control/IEHtmlDocument.cpp,
which used GetLastModified...

Fixed.

Comment 31

[Pratik]

Hey bz, thanks for fixing the bustage. Guess I should have searched lxr for
nsDocument along with my local checked out sources to make sure I didn't break
anything. Will be more careful next time. Thanks.

Comment 32

[Boris Zbarsky [:bzbarsky]]

All good.  These things happen.  ;)