Bug 179375 – [XHR] Document.lastModified doesn't work for non-rendered documents

• Component: DOM
• Status: RESOLVED FIXED

[Ms2ger (he/him; ⌚ UTC+1/+2)]

For example in XHR, as tested in <http://w3c-test.org/XMLHttpRequest/responsexml-document-properties.htm>. The code does

    if (m_frame && loader())
        dateTime = loader()->response().lastModified();

Possibly there should be a better way to get at a response; XHR certainly has one lying around.

[Rob Buis]

Created attachment 355331 [details]
Patch

[EWS Watchlist]

Comment on attachment 355331 [details]
Patch

Attachment 355331 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10088684

New failing tests:
media/no-fullscreen-when-hidden.html

[EWS Watchlist]

Created attachment 355347 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 355353 [details]
Patch

[Rob Buis]

Comment on attachment 355353 [details]
Patch

Equivalent chromium patch is here:
https://chromium-review.googlesource.com/c/chromium/src/+/1344137
Firefox already passes:
https://wpt.fyi/results/xhr/responsexml-document-properties.htm?label=stable&aligned

[Frédéric Wang (:fredw)]

Comment on attachment 355353 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355353&action=review

> Source/WebCore/dom/Document.cpp:4850
> +    else if (loader())

Can you please explain in the changelog why we have to remove m_frame here? (not sure why the m_frame check was actually needed but it looks like a behavior change).

[Rob Buis]

(In reply to Frédéric Wang (:fredw) from comment #6)
> Comment on attachment 355353 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355353&action=review
> 
> > Source/WebCore/dom/Document.cpp:4850
> > +    else if (loader())
> 
> Can you please explain in the changelog why we have to remove m_frame here?
> (not sure why the m_frame check was actually needed but it looks like a
> behavior change).

Look what the first line in Document::loader() checks :) I don't think we have to spell it out in the changelog.

[Frédéric Wang (:fredw)]

Comment on attachment 355353 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355353&action=review

>>> Source/WebCore/dom/Document.cpp:4850
>>> +    else if (loader())
>> 
>> Can you please explain in the changelog why we have to remove m_frame here? (not sure why the m_frame check was actually needed but it looks like a behavior change).
> 
> Look what the first line in Document::loader() checks :) I don't think we have to spell it out in the changelog.

OK :-) I think you can just say something like "no need to test m_frame since that's already done in loader()".

> LayoutTests/ChangeLog:8
> +        Unskip since the dynamic message is gone.

Unskip responsexml-document-properties.htm

[Rob Buis]

Created attachment 355389 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 355331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355331&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:203
> +            m_responseDocument->setOverrideLastModified(m_response.lastModified());

Seems OK, however how exactly does this behave when there is no Last-Modified header field? It doesn’t look like there is a test for that. 

I’m not in front of a large screen now to check, does this function get called for all XHR documents, including HTML ones?

Finally, I wonder if it would make sense to stash the whole response in Document.

[Rob Buis]

Comment on attachment 355331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355331&action=review

>> Source/WebCore/xml/XMLHttpRequest.cpp:203
>> +            m_responseDocument->setOverrideLastModified(m_response.lastModified());
> 
> Seems OK, however how exactly does this behave when there is no Last-Modified header field? It doesn’t look like there is a test for that. 
> 
> I’m not in front of a large screen now to check, does this function get called for all XHR documents, including HTML ones?
> 
> Finally, I wonder if it would make sense to stash the whole response in Document.

If there is no Last-Modified header field the behavior is as before, i.e. the current date and time is taken, this is tested by the 'lastModified set to time of response if no HTTP header provided' subtest in responsexml-document-properties.htm.

This new API is called for both HTML and XML standalone Documents (only use of this API is in XMLHttpRequest::responseXML()).

I think it is enough to store the optional<WallTime> here. Are you worried about the size? My thinking was that Documents are pretty rare compared to some other classes (Nodes/Render objects etc.)

[Alexey Proskuryakov]

Size can be an interesting consideration. I was concerned about code elegance, as it's already getting out of hand - see how the naming differs between "overrideMIMEType()" and "setOverrideLastModified()".

[Rob Buis]

Created attachment 355874 [details]
Patch

[Rob Buis]

(In reply to Alexey Proskuryakov from comment #12)
> Size can be an interesting consideration. I was concerned about code
> elegance, as it's already getting out of hand - see how the naming differs
> between "overrideMIMEType()" and "setOverrideLastModified()".

Understood, I fixed that by changing to overrideLastModified().
Note also that this just landed on chromium side:
https://chromium-review.googlesource.com/c/chromium/src/+/1344137

[EWS Watchlist]

Comment on attachment 355874 [details]
Patch

Attachment 355874 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10182997

New failing tests:
media/no-fullscreen-when-hidden.html

[EWS Watchlist]

Created attachment 355883 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Frédéric Wang (:fredw)]

(In reply to Build Bot from comment #15)
> Comment on attachment 355874 [details]
> Patch
> 
> Attachment 355874 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/10182997
> 
> New failing tests:
> media/no-fullscreen-when-hidden.html

This failure seems unrelated, see https://bugs.webkit.org/show_bug.cgi?id=192088

[WebKit Commit Bot]

Comment on attachment 355874 [details]
Patch

Clearing flags on attachment: 355874

Committed r238628: <https://trac.webkit.org/changeset/238628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46318140>