Bug 179375 - [XHR] Document.lastModified doesn't work for non-rendered documents
Summary: [XHR] Document.lastModified doesn't work for non-rendered documents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-07 07:40 PST by Ms2ger (he/him; ⌚ UTC+1/+2)
Modified: 2018-11-28 12:29 PST (History)
12 users (show)

See Also:


Attachments
Patch (9.39 KB, patch)
2018-11-20 03:21 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.72 MB, application/zip)
2018-11-20 08:24 PST, EWS Watchlist
no flags Details
Patch (9.53 KB, patch)
2018-11-20 11:09 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.67 KB, patch)
2018-11-20 23:30 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2018-11-28 07:57 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.59 MB, application/zip)
2018-11-28 09:57 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ms2ger (he/him; ⌚ UTC+1/+2) 2017-11-07 07:40:46 PST
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.
Comment 1 Rob Buis 2018-11-20 03:21:02 PST
Created attachment 355331 [details]
Patch
Comment 2 EWS Watchlist 2018-11-20 08:24:06 PST
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
Comment 3 EWS Watchlist 2018-11-20 08:24:07 PST
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
Comment 4 Rob Buis 2018-11-20 11:09:37 PST
Created attachment 355353 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 2018-11-20 13:01:59 PST
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).
Comment 7 Rob Buis 2018-11-20 13:07:13 PST
(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.
Comment 8 Frédéric Wang (:fredw) 2018-11-20 13:13:05 PST
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
Comment 9 Rob Buis 2018-11-20 23:30:01 PST
Created attachment 355389 [details]
Patch
Comment 10 Alexey Proskuryakov 2018-11-21 10:13:00 PST
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.
Comment 11 Rob Buis 2018-11-21 11:06:26 PST
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.)
Comment 12 Alexey Proskuryakov 2018-11-21 19:37:05 PST
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()".
Comment 13 Rob Buis 2018-11-28 07:57:01 PST
Created attachment 355874 [details]
Patch
Comment 14 Rob Buis 2018-11-28 07:58:48 PST
(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
Comment 15 EWS Watchlist 2018-11-28 09:57:17 PST
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
Comment 16 EWS Watchlist 2018-11-28 09:57:19 PST
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
Comment 17 Frédéric Wang (:fredw) 2018-11-28 10:48:04 PST
(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
Comment 18 WebKit Commit Bot 2018-11-28 12:28:17 PST
Comment on attachment 355874 [details]
Patch

Clearing flags on attachment: 355874

Committed r238628: <https://trac.webkit.org/changeset/238628>
Comment 19 WebKit Commit Bot 2018-11-28 12:28:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2018-11-28 12:29:35 PST
<rdar://problem/46318140>