Align XMLHttpRequest's open() / send() / abort() with the latest specification: - https://xhr.spec.whatwg.org
Created attachment 336697 [details] WIP Patch
Created attachment 336698 [details] WIP Patch
Created attachment 336702 [details] WIP Patch
Created attachment 336711 [details] WIP Patch
Created attachment 336717 [details] WIP Patch
Created attachment 336721 [details] Patch
Created attachment 336725 [details] Patch
Comment on attachment 336725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336725&action=review > Source/WebCore/xml/XMLHttpRequest.cpp:338 > + m_method = normalizeHTTPMethod(method); We can probably normalize the method after the early returns, at the moment the spec says to set m_method. > Source/WebCore/xml/XMLHttpRequest.cpp:381 > changeState(OPENED); changeState is checking for m_state so we can probably call changeState directly, name is not great though. > Source/WebCore/xml/XMLHttpRequest.cpp:591 > + // Also, only async requests support upload progress events. Do we need this last comment? > Source/WebCore/xml/XMLHttpRequest.cpp:622 > + return { }; As per spec, we should check for m_state and m_sendFlag, but I am not clear why we are checking for m_loader? > Source/WebCore/xml/XMLHttpRequest.cpp:-679 > - m_state = UNSENT; Above it is stated "// Clear headers as required by the spec", which is no longer the case. Maybe we can get rid of this comment or the spec is missing something. If we add a header, abort and add the same header, would the values be combined? > LayoutTests/ChangeLog:14 > + per specification and those tests were also failing in Firefox. I wonder whether these 3 tests bring actually anything compared to their WPT equivalent.
Comment on attachment 336725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336725&action=review >> Source/WebCore/xml/XMLHttpRequest.cpp:338 >> + m_method = normalizeHTTPMethod(method); > > We can probably normalize the method after the early returns, at the moment the spec says to set m_method. Good idea. >> Source/WebCore/xml/XMLHttpRequest.cpp:381 >> changeState(OPENED); > > changeState is checking for m_state so we can probably call changeState directly, name is not great though. Ok. >> Source/WebCore/xml/XMLHttpRequest.cpp:591 >> + // Also, only async requests support upload progress events. > > Do we need this last comment? I'll drop it. >> Source/WebCore/xml/XMLHttpRequest.cpp:622 >> + return { }; > > As per spec, we should check for m_state and m_sendFlag, but I am not clear why we are checking for m_loader? I had to add it or some WPT tests would still fail when: 1. We dispatch the loadstart event (above) 2. In the loadstart event handler, the JS calls both open() and send() again Because the JS has called open() and send() again, m_state is OPENED and m_sendFlag is true. open() normally cancels any pending load but we have no started the load yet (it happens a few lines below). Adding the m_loader check fixes the issue as we return early when we've re-entered and started a new load. >> Source/WebCore/xml/XMLHttpRequest.cpp:-679 >> - m_state = UNSENT; > > Above it is stated "// Clear headers as required by the spec", which is no longer the case. > Maybe we can get rid of this comment or the spec is missing something. > If we add a header, abort and add the same header, would the values be combined? I'll check.
Comment on attachment 336725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336725&action=review >> LayoutTests/ChangeLog:14 >> + per specification and those tests were also failing in Firefox. > > I wonder whether these 3 tests bring actually anything compared to their WPT equivalent. My understanding is that we are not actively trying to avoid duplication between layout tests and WPT?
Comment on attachment 336725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336725&action=review >>> Source/WebCore/xml/XMLHttpRequest.cpp:622 >>> + return { }; >> >> As per spec, we should check for m_state and m_sendFlag, but I am not clear why we are checking for m_loader? > > I had to add it or some WPT tests would still fail when: > 1. We dispatch the loadstart event (above) > 2. In the loadstart event handler, the JS calls both open() and send() again > > Because the JS has called open() and send() again, m_state is OPENED and m_sendFlag is true. open() normally cancels any pending load but we have no started the load yet (it happens a few lines below). Adding the m_loader check fixes the issue as we return early when we've re-entered and started a new load. This is the test that would fail: imported/w3c/web-platform-tests/XMLHttpRequest/loadstart-and-state.html Diff: -PASS open() during loadstart +FAIL open() during loadstart assert_equals: expected "BB-8" but got "R2-D2" This test is passing in both Firefox and Chrome.
> > I wonder whether these 3 tests bring actually anything compared to their WPT equivalent. > > My understanding is that we are not actively trying to avoid duplication > between layout tests and WPT? That makes sense for tests that continue passing. When a test is obsolete, we should either think of modifying it or removing it if superseded by another test. I looked at http/tests/xmlhttprequest/onloadend-event-after-abort.html and it is not exactly a copy so they might still be useful. > >> Source/WebCore/xml/XMLHttpRequest.cpp:622 > >> + return { }; > > > > As per spec, we should check for m_state and m_sendFlag, but I am not clear why we are checking for m_loader? > > I had to add it or some WPT tests would still fail when: > 1. We dispatch the loadstart event (above) > 2. In the loadstart event handler, the JS calls both open() and send() again > > Because the JS has called open() and send() again, m_state is OPENED and > m_sendFlag is true. open() normally cancels any pending load but we have no > started the load yet (it happens a few lines below). Adding the m_loader > check fixes the issue as we return early when we've re-entered and started a > new load. That makes sense, maybe that is worth a comment.
Created attachment 336750 [details] Patch
Comment on attachment 336750 [details] Patch Clearing flags on attachment: 336750 Committed r230066: <https://trac.webkit.org/changeset/230066>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38985134>