Ability to replay existing XHR.
Created attachment 160952 [details] Patch
Comment on attachment 160952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160952&action=review Please add tests. > Source/WebCore/inspector/Inspector.json:878 > + "description": "Replays XHR.", Please make this description more verbose. It should mention that XHR resent is identical to the original one and specify the concrete parameters that are identical in them. > Source/WebCore/inspector/InspectorInstrumentation.cpp:333 > +void InspectorInstrumentation::DidCreateXhrRequestImpl(InstrumentingAgents* instrumentingAgents, String method, bool async, PassRefPtr<FormData> formData, const HTTPHeaderMap& headers, bool includeCredentials, String login, String password) didCreateXhrRequestImpl > Source/WebCore/inspector/InspectorResourceAgent.h:71 > +struct XhrData; XHRReplayData > Source/WebCore/inspector/InspectorResourceAgent.h:-108 > - Please revert unneeded changes. > Source/WebCore/inspector/front-end/NetworkPanel.js:965 > + if (request && request.type.name() === "xhr") { request.type === WebInspector.resourceTypes.XHR > Source/WebCore/inspector/front-end/NetworkRequest.js:801 > + Please revert > Source/WebCore/loader/DocumentThreadableLoader.cpp:403 > + if (m_preflightRequestIdentifier) Let's make it look like if (m_actualRequest) ... else if (m_preflightRequestIdentifier) ... for consistency. > Source/WebCore/xml/XMLHttpRequest.cpp:690 > + createRequest(ec); As discussed we should set Content-Type header here.
Comment on attachment 160952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160952&action=review Please add a screenshot > Source/WebCore/ChangeLog:8 > + Added XHR replay in network tab. Please add a description of implementation details.
Created attachment 160961 [details] Patch
Comment on attachment 160961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160961&action=review > Source/WebCore/inspector/InspectorInstrumentation.h:165 > + static void didCreateXHRRequestImpl(InstrumentingAgents*, String, bool, PassRefPtr<FormData>, const HTTPHeaderMap&, bool, String, String); ...Impl methods should be below (together with the others). > Source/WebCore/inspector/InspectorInstrumentation.h:167 > + static void preflightRequestSuccededImpl(InstrumentingAgents*, unsigned long); Ditto > Source/WebCore/inspector/InspectorResourceAgent.cpp:226 > + m_preflightIdToRequestId.add(requestId, IdentifiersFactory::requestId(m_preflightRequestId)); m_resourcesData->setXHRReplayData(requestId, m_resourcesData->XHRData(m_preflightRequestId)); This way you don't need this map at all. > Source/WebCore/inspector/InspectorResourceAgent.cpp:650 > + , m_XHRData(0) m_XHRReplayData > Source/WebCore/inspector/InspectorResourceAgent.h:164 > + unsigned long m_preflightRequestId; I eould call it m_xhrReplayDataRequestId and reuse in replayXHR to avoid duplicating xhrReplayData for replayed xhrs. > Source/WebCore/inspector/InspectorResourceAgent.h:165 > + HashMap<String, String> m_preflightIdToRequestId; This map is not really needed, see above.
Comment on attachment 160961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160961&action=review > Source/WebCore/inspector/InspectorResourceAgent.h:163 > + XHRReplayData* m_XHRData; RefPtr > Source/WebCore/inspector/NetworkResourcesData.h:58 > + XHRReplayData(String method, bool async, PassRefPtr<FormData> formData, const HTTPHeaderMap& headers, bool includeCredentials, String login, String password): m_method(method), m_async(async), m_formData(formData), m_headers(headers), m_includeCredentials(includeCredentials), m_login(login), m_password(password) { } Please move implementation to .cpp > Source/WebCore/inspector/NetworkResourcesData.h:118 > + XHRReplayData* m_XHRData; RefPtr
Created attachment 161220 [details] Patch
Comment on attachment 161220 [details] Patch Attachment 161220 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13686209
Comment on attachment 161220 [details] Patch Attachment 161220 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13683283
Comment on attachment 161220 [details] Patch Clearing r? as some comments were not addressed yet.
Created attachment 161232 [details] Patch
Comment on attachment 161232 [details] Patch Attachment 161232 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13685292
Created attachment 161256 [details] Patch
Comment on attachment 161256 [details] Patch Attachment 161256 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13680301
Comment on attachment 161256 [details] Patch Attachment 161256 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13682283
Comment on attachment 161256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161256&action=review > Source/WebCore/inspector/InspectorResourceAgent.cpp:595 > +void InspectorResourceAgent::replayXHR(ErrorString* errorString, const String& requestId) Add ASSERT_UNUSED(errorString); below to make mac bot happy. > Source/WebCore/inspector/InspectorResourceAgent.cpp:602 > + RefPtr<XHRReplayData> XHRData = m_resourcesData->XHRData(actualRequestId); xhrData > Source/WebCore/inspector/InspectorResourceAgent.cpp:654 > + , m_XHRData(0) m_xhrData > Source/WebCore/inspector/NetworkResourcesData.h:120 > + RefPtr<XHRReplayData> m_XHRData; ditto > Source/WebCore/inspector/NetworkResourcesData.h:153 > + PassRefPtr<XHRReplayData> XHRData(const String& requestId); ditto > Source/WebCore/loader/DocumentThreadableLoader.cpp:386 > + InspectorInstrumentation::preflightRequestSucceded(m_document, m_preflightRequestIdentifier); No brackets for one line conditionals.
Comment on attachment 161256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161256&action=review > Source/WebCore/inspector/Inspector.json:882 > + "returns": [ Empty returns is redundant. > Source/WebCore/inspector/InspectorInstrumentation.cpp:333 > +void InspectorInstrumentation::didCreateXHRRequestImpl(InstrumentingAgents* instrumentingAgents, String method, bool async, PassRefPtr<FormData> formData, const HTTPHeaderMap& headers, bool includeCredentials, String login, String password) Here and in other places: s/String/const String&/ > Source/WebCore/xml/XMLHttpRequest.cpp:689 > + m_requestEntityBody = formData; You are not really passing the ownership of formData here. You should pass it by pointer and deepCopy back here. > Source/WebCore/xml/XMLHttpRequest.cpp:728 > + InspectorInstrumentation::didCreateXHRRequest(scriptExecutionContext(), m_method, m_async, m_requestEntityBody ? m_requestEntityBody->deepCopy() : 0, m_requestHeaders, m_includeCredentials, m_url.user(), m_url.pass()); We'd better use m_requestHeaders.copyData() here instead of m_requestHeaders (and adopt() the result on the other side)
Created attachment 161470 [details] Patch
Comment on attachment 161470 [details] Patch This patch looks identical to the previous one.
Created attachment 161475 [details] Patch
Comment on attachment 161475 [details] Patch Attachment 161475 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13694695
Comment on attachment 161475 [details] Patch Attachment 161475 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13682716
Comment on attachment 161475 [details] Patch Attachment 161475 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13681720
Comment on attachment 161475 [details] Patch Attachment 161475 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13690727
Comment on attachment 161475 [details] Patch Attachment 161475 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13686692
Comment on attachment 161475 [details] Patch Attachment 161475 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13695710
Comment on attachment 161475 [details] Patch Attachment 161475 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13686693
Comment on attachment 161475 [details] Patch Attachment 161475 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13690732
Created attachment 161487 [details] Patch
Comment on attachment 161487 [details] Patch Attachment 161487 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13682752
Comment on attachment 161487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161487&action=review > Source/WebCore/inspector/InspectorResourceAgent.cpp:212 > + OwnPtr<CrossThreadHTTPHeaderMapData> headerMapData = headers.copyData(); Please remove > Source/WebCore/inspector/InspectorResourceAgent.cpp:614 > + xhr->setRequestHeader(it->first, it->second, code); xhr->sendFromInspector(xhrData->m_formData, code); New line > Source/WebCore/inspector/NetworkResourcesData.cpp:276 > +void NetworkResourcesData::setXHRReplayData(const String& requestId, PassRefPtr<XHRReplayData> XHRData) zhrData > Source/WebCore/inspector/NetworkResourcesData.h:51 > + String m_method; Please make fields private an add getters. > Source/WebCore/inspector/NetworkResourcesData.h:108 > + void setXHRReplayData(PassRefPtr<XHRReplayData> XHRData) { m_xhrReplayData = XHRData;}; space between ; and } > Source/WebCore/loader/DocumentThreadableLoader.cpp:404 > + if (m_actualRequest) Please surround with #if ENABLE(INSPECTOR)
Created attachment 161641 [details] Patch
Comment on attachment 161641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161641&action=review > Source/WebCore/inspector/Inspector.json:878 > + "description": "Replays XHR. The new XHR is identical to original (parameters method, async, body, headers, withCredentials, login, password are copied)", to original -> to the original one > Source/WebCore/inspector/InspectorInstrumentation.h:1185 > + extra line? > Source/WebCore/inspector/InspectorResourceAgent.cpp:600 > +void InspectorResourceAgent::replayXHR(ErrorString* errorString, const String& requestId) "ErrorString* errorString," -> "ErrorString*," and remove UNUSED_PARAM macro below > Source/WebCore/inspector/InspectorResourceAgent.cpp:615 > + m_originalRequestId = String(); I guess String should have a method like clear() or reset() > Source/WebCore/inspector/InspectorResourceAgent.h:71 > +struct XHRReplayData; struct -> class > Source/WebCore/inspector/NetworkResourcesData.cpp:49 > +PassRefPtr<XHRReplayData> XHRReplayData::create(String method, bool async, PassRefPtr<FormData> formData, const HTTPHeaderMap& headers, bool includeCredentials, String login, String password) String method -> const String& method? for all other places also. > Source/WebCore/inspector/NetworkResourcesData.h:52 > + String method() { return m_method; } const String& method() const; and for other methods also. > Source/WebCore/inspector/NetworkResourcesData.h:53 > + bool async() { return m_async; } bool async() const;
Comment on attachment 161641 [details] Patch Attachment 161641 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13717093
Created attachment 161651 [details] Patch
Created attachment 161707 [details] Patch
Comment on attachment 161707 [details] Patch Attachment 161707 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13723250 New failing tests: animations/suspend-resume-animation-events.html
Created attachment 163904 [details] Patch
ap, japhet: Could you please review XMLHttpRequest / DocumentThreadableLoader / CachedResource changes?
Comment on attachment 163904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163904&action=review > Source/WebCore/loader/DocumentThreadableLoader.cpp:46 > +#include "SubresourceLoader.h" I don't think this include is necessary. > Source/WebCore/loader/cache/CachedRawResource.h:-44 > - // FIXME: This is exposed for the InpsectorInstrumentation for preflights in DocumentThreadableLoader. It's also really lame. > - unsigned long identifier() const { return m_identifier; } > - Does this actually need to move down to CachedResource? I don't immediately see a call that requires it.
(In reply to comment #40) > (From update of attachment 163904 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163904&action=review > > > Source/WebCore/loader/DocumentThreadableLoader.cpp:46 > > +#include "SubresourceLoader.h" > > I don't think this include is necessary. > It is not, thanks > > Source/WebCore/loader/cache/CachedRawResource.h:-44 > > - // FIXME: This is exposed for the InpsectorInstrumentation for preflights in DocumentThreadableLoader. It's also really lame. > > - unsigned long identifier() const { return m_identifier; } > > - > > Does this actually need to move down to CachedResource? I don't immediately see a call that requires it. It does not need it, I will move it back.
Comment on attachment 163904 [details] Patch Attachment 163904 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13849390 New failing tests: http/tests/inspector/network/network-xhr-replay.html
Comment on attachment 163904 [details] Patch Attachment 163904 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13858019 New failing tests: http/tests/inspector/network/network-xhr-replay.html
Created attachment 164061 [details] Patch
Created attachment 164070 [details] Patch
Comment on attachment 164070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164070&action=review > Source/WebCore/inspector/InspectorResourceAgent.cpp:-364 > -void InspectorResourceAgent::willLoadXHRSynchronously() Let's keep will/didLoadXHRSynchronously as before for clarity as discussed off the bug. > Source/WebCore/inspector/InspectorResourceAgent.cpp:618 > + xhr->sendFromInspector(xhrReplayData->formData(), code);return; Remove 'return;' > Source/WebCore/loader/DocumentThreadableLoader.cpp:410 > +#if ENABLE(INSPECTOR) No need in this guard as we already have one in the InspectorInstrumentation
Committed r128580: <http://trac.webkit.org/changeset/128580>
(In reply to comment #48) > Committed r128580: <http://trac.webkit.org/changeset/128580> It broke the !ENABLE(INSPECTOR) builds: ---------------------------------------- /ramdisk/qt-linux-release-minimal/build/Source/WebCore/loader/DocumentThreadableLoader.cpp: In member function 'void WebCore::DocumentThreadableLoader::loadRequest(const WebCore::ResourceRequest&, WebCore::SecurityCheckPolicy)': /ramdisk/qt-linux-release-minimal/build/Source/WebCore/loader/DocumentThreadableLoader.cpp:410: error: 'InspectorInstrumentation' has not been declared Could you fix it, please? It seems only an ifdef guard is missing.
(In reply to comment #49) > (In reply to comment #48) > > Committed r128580: <http://trac.webkit.org/changeset/128580> > > It broke the !ENABLE(INSPECTOR) builds: > ---------------------------------------- > /ramdisk/qt-linux-release-minimal/build/Source/WebCore/loader/DocumentThreadableLoader.cpp: In member function 'void WebCore::DocumentThreadableLoader::loadRequest(const WebCore::ResourceRequest&, WebCore::SecurityCheckPolicy)': > /ramdisk/qt-linux-release-minimal/build/Source/WebCore/loader/DocumentThreadableLoader.cpp:410: error: 'InspectorInstrumentation' has not been declared > > Could you fix it, please? It seems only an ifdef guard is missing. Should be fixed, thanks!
http/tests/inspector/network/network-xhr-replay.html crashes on EFL, Lion, MountenLion, Qt. Can you check it, please?
(In reply to comment #51) > http/tests/inspector/network/network-xhr-replay.html crashes on EFL, Lion, MountenLion, Qt. Can you check it, please? On Qt http/tests/inspector/network/network-xhr-same-url-as-main-resource.html is crashing, sorry.