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.
Created attachment 355331 [details] Patch
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
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
Created attachment 355353 [details] Patch
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
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).
(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 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
Created attachment 355389 [details] Patch
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 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.)
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()".
Created attachment 355874 [details] Patch
(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 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
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
(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 on attachment 355874 [details] Patch Clearing flags on attachment: 355874 Committed r238628: <https://trac.webkit.org/changeset/238628>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46318140>