Bug 128782

I will post two patches here that can be reviewed separately. 1) replay machinery 2) new test infrastructure to support replay tests, and said tests They can be combined for EWS after review and landed as one.

Tests for replay will be part of the inspector layout tests, and require an alternate bootstrapping process. The current protocol tests open up a page from the specific test using window.open(). This child page serves as the dummy inspector page, and the parent (test) page serves as the inspected page. We instantiate protocol handlers and inspector models into existence using eval(), and route messages from the backend to the child inspector page. This won't work for testing replay functionality with reference tests (i.e., comparing output of two runs), because the child inspector page will be disconnected when the main frame navigates during capture or replay. For such tests, we could use the "old" inspector test bootstrapping, which calls showWebInspector() to launch a Web Inspector page (including views, IIRC) that persists across navigatios. We can use evaluateInWebInspector() to feed it the test script (calls to backend, etc). In a replay context, the test.html page would include the various replay test commands inside a runTest() method, which would be evaluated in the inspector page on the first page load (but not subsequent loads). The runTest() method will start capturing by reloading the test page, and save its output into a capture output buffer. Then, runTest() will replay the captured replay session which causes another reload, and save its output into a replay output buffer. Then the test can diff the two output buffers.

(In reply to comment #2) > For such tests, we could use the "old" inspector test bootstrapping, which calls showWebInspector() to launch a Web Inspector page (including views, IIRC) that persists across navigatios. We can use evaluateInWebInspector() to feed it the test script (calls to backend, etc). I was never fond of evaluateInWebInspector(). I think it causes more UI testing than model/controller testing (even if not intentional).

(In reply to comment #3) > (In reply to comment #2) > > For such tests, we could use the "old" inspector test bootstrapping, which calls showWebInspector() to launch a Web Inspector page (including views, IIRC) that persists across navigatios. We can use evaluateInWebInspector() to feed it the test script (calls to backend, etc). > > I was never fond of evaluateInWebInspector(). I think it causes more UI testing than model/controller testing (even if not intentional). If there's some reasonable way to do so, I'd be happy to only load the controllers and models into the separate inspector page. I only care about the inspector persisting across navigations.

Created attachment 224819 [details] patch part 1 - v1

Attachment 224819 [details] did not pass style-queue: ERROR: Source/WebCore/replay/SerializationMethods.cpp:93: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] ERROR: Source/WebCore/replay/ReplayInputTypes.cpp:41: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] ERROR: Source/WebCore/replay/SerializationMethods.h:64: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 3 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

After some discussion, we think it will take a bit of refactoring to make the test harness do what we want without inadvertently loading 400+ inspector resources on every test. So, let's move forward with this patch and the test situation will catch up next week.

Comment on attachment 224819 [details] patch part 1 - v1 View in context: https://bugs.webkit.org/attachment.cgi?id=224819&action=review Oops, meant to dump this nitpickery yesterday! > Source/WebCore/inspector/InspectorReplayAgent.cpp:61 > + RefPtr<TypeBuilder::Replay::ReplayPosition> positionObject = TypeBuilder::Replay::ReplayPosition::create() > + .setSegmentOffset(position.segmentOffset) > + .setInputOffset(position.inputOffset); You've indented the lines starting with "." below, but not here. > Source/WebCore/inspector/InspectorReplayAgent.cpp:93 > + return sessionObject; Avoid churning the reference count with .release(): return sessionObject.release(); (Though I'd skip the temporary RefPtr entirely here.) > Source/WebCore/inspector/InspectorReplayAgent.cpp:114 > + RefPtr<TypeBuilder::Array<TypeBuilder::Replay::ReplayInput> > m_inputs; No need for "> >" in WebKit. > Source/WebCore/inspector/InspectorReplayAgent.cpp:145 > + , m_frontendDispatcher(nullptr) > + , m_backendDispatcher(nullptr) You don't need to null-intialize smart pointers; their default constructors will take care of that. > Source/WebCore/inspector/InspectorReplayAgent.cpp:149 > + , m_sessionsMap(SessionsMap()) > + , m_segmentsMap(SegmentsMap()) These lines are not needed. > Source/WebCore/inspector/InspectorReplayAgent.h:122 > + typedef HashMap<int, RefPtr<ReplaySession>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>> SessionsMap; > + SessionsMap m_sessionsMap; > + > + typedef HashMap<int, RefPtr<ReplaySessionSegment>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>> SegmentsMap; > + SegmentsMap m_segmentsMap; I would use "auto" instead of typedeffing these. > Source/WebCore/replay/CapturingInputCursor.cpp:41 > + : m_storage(storage) > +{ > + ASSERT(m_storage); m_storage should just be a reference > Source/WebCore/replay/CapturingInputCursor.cpp:78 > +#endif // ENABLE(TIMEALPSE) ;) > Source/WebCore/replay/CapturingInputCursor.h:55 > + CapturingInputCursor(SegmentedInputStorage*); explicit > Source/WebCore/replay/ReplaySession.cpp:46 > + : m_segments(Vector<RefPtr<ReplaySessionSegment>>()) This line is not needed. > Source/WebCore/replay/ReplaySession.cpp:68 > + m_segments.append(segment); Avoid ref count churn by doing: m_segments.append(segment.release()); > Source/WebCore/replay/ReplaySession.h:56 > + SegmentIterator begin() const { return m_segments.begin(); }; > + SegmentIterator end() const { return m_segments.end(); }; The semicolons at EOL are not needed. > Source/WebCore/replay/ReplaySessionSegment.cpp:45 > + return adoptRef(new ReplaySessionSegment()); Unofficial(?) WebKit style is to omit () for argument-less constructors: return adoptRef(new ReplaySessionSegment); > Source/WebCore/replay/ReplaySessionSegment.cpp:74 > + return std::make_unique<FunctorInputCursor>(m_storage.get()); Looks like m_storage will never be non-null, so we should pass it by reference and make the input cursor take/store it as such. > Source/WebCore/replay/ReplayingInputCursor.cpp:44 > + , m_positions(Vector<size_t>()) This line is not needed.

Comment on attachment 224819 [details] patch part 1 - v1 View in context: https://bugs.webkit.org/attachment.cgi?id=224819&action=review Looks good to me as well. Maybe one more set of eyes on an updated patch? > Source/WebCore/DerivedSources.make:1149 > + $(WebReplayScripts)/CodeGeneratorReplayInputs.py \ > + $(WebReplayScripts)/CodeGeneratorReplayInputsTemplates.py \ Also update Source/WebCore/make-generated-sources.sh to export WebReplayScripts. AFAIK that is a helper tool to just run DerivedSources.make outside of Xcode, and it should have all external variables, like WebReplayScripts, be defined. > Source/WebCore/inspector/InspectorInstrumentation.h:1830 > +inline void InspectorInstrumentation::sessionCreated(Page* page, PassRefPtr<ReplaySession> session) > +{ > +#if ENABLE(INSPECTOR) > + if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForPage(page)) > + sessionCreatedImpl(instrumentingAgents, session); I think these should all fast return if no frontends: FAST_RETURN_IF_NO_FRONTENDS(void()); Since the instrumentingAgentsForPage -> inspectorReplayAgent would only return the agent when a frontend is attached (it adds the agent in didCreateFrontendAndBackend). Until there is a frontend, these methods will do nothing. > Source/WebCore/inspector/InspectorReplayAgent.cpp:147 > + , m_page(nullptr) How about initializing m_page here to be pageAgent->page() and avoiding the possibility of it ever being null. > Source/WebCore/inspector/InspectorReplayAgent.cpp:161 > + ASSERT(m_page); You can remove this. > Source/WebCore/inspector/InspectorReplayAgent.cpp:171 > + m_page = m_pageAgent->page(); And this. > Source/WebCore/inspector/InspectorReplayAgent.cpp:189 > + m_page = nullptr; And this. > Source/WebCore/inspector/InspectorReplayAgent.cpp:217 > + if (m_frontendDispatcher) > + m_frontendDispatcher->sessionCreated(session->identifier()); Should not need to if-check m_frontendDispatcher here (or anywhere in this file). If m_frontendDispatcher is null, then this agent would not be in InstrumentingAgents, and all InspectorInstrumentation calls would have flubbed. > Source/WebCore/inspector/InspectorReplayAgent.cpp:313 > + *errorString = "Can't start capturing if the session is already capturing or replaying."; Nit: ASCIILiteral for all *errorString = ASCIILiteral("..."); > Source/WebCore/inspector/InspectorReplayAgent.h:60 > +class InspectorReplayAgent final > + : public InspectorAgentBase > + , public Inspector::InspectorReplayBackendDispatcherHandler { Nit: 1 line is normal, unless you think this is better, then we should change other inspector agents. > Source/WebCore/inspector/InspectorReplayAgent.h:107 > + void startCapturing(ErrorString*) override; > + void stopCapturing(ErrorString*) override; > + > + void replayToPosition(ErrorString*, const RefPtr<Inspector::InspectorObject>&, bool shouldFastForward) override; > + void replayToCompletion(ErrorString*, bool shouldFastForward) override; > + void pausePlayback(ErrorString*) override; > + void cancelPlayback(ErrorString*) override; > + > + void switchSession(ErrorString*, SessionIdentifier) override; > + void insertSessionSegment(ErrorString*, SessionIdentifier, SegmentIdentifier, int segmentIndex) override; > + void removeSessionSegment(ErrorString*, SessionIdentifier, int segmentIndex) override; > + > + void getAvailableSessions(ErrorString*, RefPtr<Inspector::TypeBuilder::Array<SessionIdentifier>>&) override; > + void getSerializedSession(ErrorString*, SessionIdentifier, RefPtr<Inspector::TypeBuilder::Replay::ReplaySession>&) override; > + void getSerializedSegment(ErrorString*, SegmentIdentifier, RefPtr<Inspector::TypeBuilder::Replay::SessionSegment>&) override; > + Nit: I'd prefer to see "virtual" on all of these. They are virtual, as evident by override. But it also makes it easier them easier to find in the header. > Source/WebCore/inspector/protocol/Replay.json:18 > + { "name": "segmentOffset", "type": "integer", "description": "Offset for an event loop input within the specified session segment." }, > + { "name": "inputOffset", "type": "integer", "description": "Offset for an event loop input within the specified session segment." } The description of these are the same. There should be some difference in the description if these truly are different! > Source/WebCore/inspector/protocol/Replay.json:35 > + { "name": "type", "type": "string", "description": "Queue type" }, Should this be an enum? > Source/WebCore/inspector/protocol/Replay.json:40 > + "id": "SessionSegment", "description": "A standalone segment of a replay session that corresponds to a single main frame navigation and execution.", Neat style. This is fine, or description as the second line. > Source/WebCore/inspector/protocol/Replay.json:85 > + }, Wrong indentation. > Source/WebCore/inspector/protocol/Replay.json:125 > + { "name": "sessionId", "$ref": "SessionIdentifier" } Most of the time this is "sessionIdentifier" but here it is "sessionId". The name in the protocol is important, because this will be in the generated code and we may need to be backwards compatible with it. Lets settle on sessionIdentifier. > Source/WebCore/inspector/protocol/Replay.json:128 > + { "name": "session", "$ref": "ReplaySession", "description": "The requested serialized replay session.", "optional": true } Nit: Move optional before description. > Source/WebCore/inspector/protocol/Replay.json:138 > + { "name": "segment", "$ref": "SessionSegment", "description": "The requested serialized session segment.", "optional": true } Nit: Move optional before description. > Source/WebCore/inspector/protocol/Replay.json:142 > + Nit: unnecessary blank space. > Source/WebCore/inspector/protocol/Replay.json:235 > + { "name": "segmentId", "$ref": "SegmentIdentifier", "description": "Id for the loaded segment." } Same here. Lets go with segmentIdentifier. > Source/WebCore/replay/CapturingInputCursor.cpp:43 > + LOG(WebReplay, "%-30sCreated capture cursor=%p.\n", "[ReplayController]", (void*)this); Is the void* cast needed? I think just doing "this" will be fine. If the cast is needed, then static_cast<void*>(...). > Source/WebCore/replay/CapturingInputCursor.cpp:48 > + LOG(WebReplay, "%-30sDestroyed capture cursor=%p.\n", "[ReplayController]", (void*)this); Ditto. > Source/WebCore/replay/CapturingInputCursor.h:38 > +class EventLoopInputBase; Nit: Unneeded? > Source/WebCore/replay/EventLoopInputDispatcher.cpp:100 > + // The goal is to reproduce the dispatch delay between inputs is it was Typo: "is it was was" => "as it was" > Source/WebCore/replay/EventLoopInputDispatcher.cpp:161 > + m_client->didDispatchFinalInput(); Should this set m_running to false here? > Source/WebCore/replay/EventLoopInputDispatcher.h:45 > + Realtime, RealTime? > Source/WebCore/replay/EventLoopInputDispatcher.h:56 > + virtual void willDispatchInput(const EventLoopInputBase&) =0; > + virtual void didDispatchInput(const EventLoopInputBase&) =0; > + virtual void didDispatchFinalInput() =0; Style: "= 0;" > Source/WebCore/replay/EventLoopInputDispatcher.h:59 > +class EventLoopInputDispatcher final { Nit: "final" not needed. This doesn't appear to subclass anything. > Source/WebCore/replay/FunctorInputCursor.h:42 > +class FunctorInputCursor : public InputCursor { Style: final on the class definition instead of each method individually. I don't see anyone extending this right now. You can leave as is if this will change in the future. > Source/WebCore/replay/FunctorInputCursor.h:66 > + for (size_t i = 0; i < m_storage->m_queues[static_cast<size_t>(queue)]->size(); i++) Instead of reaching into m_storage->m_queues directly, how about providing a friend method on SegmentedInputStorage that ASSERTs the validity of "queue": const QueuedInputs& queue(InputQueue queue) { ASSERT(queue < InputQueue::Count); return m_queues.at(offsetForInputQueue(queue)); } Here you could use it to get the queue, assert the input queue is valid, and prevent some duplication in the long lines. SegmentedInputStorage::load and SegmentedInputStorage::queueSize could use that as well and get the ASSERT. > Source/WebCore/replay/ReplayController.cpp:75 > + LOG(WebReplay, "%-20sSwitching sessions from %p to %p.\n", "ReplayController", (void*)m_loadedSession.get(), (void*)session.get()); Same comment regarding the (void*) cast. There are more places below as well. > Source/WebCore/replay/ReplayController.cpp:113 > + unloadSegment(true); Mysterious bool. You can assign to a local to give it a name: bool suppressNotifications = true; unloadSegment(suppressNotifications); > Source/WebCore/replay/ReplayController.cpp:298 > + loader->frame()->document()->setInputCursor(m_activeCursor.get()); No frame tree traversal is needed here? > Source/WebCore/replay/ReplayController.cpp:310 > +RefPtr<ReplaySession> ReplayController::loadedSession() const > +{ > + return m_loadedSession; > +} > + > +RefPtr<ReplaySessionSegment> ReplayController::loadedSegment() const > +{ > + return m_loadedSegment; > +} We don't normally return RefPtrs. I think we would return a raw pointer here, e.g. m_loadedSession.get(). These could be inlined in the header. > Source/WebCore/replay/ReplayController.h:53 > +class ReplaySession; > +class ReplaySessionSegment; > +class DOMWindow; > +class Document; > +class DocumentLoader; > +class Element; > +class Event; > +class EventLoopInputBase; > +class Frame; > +class Node; > +class Page; Style: sort > Source/WebCore/replay/ReplaySession.cpp:54 > +ReplaySession::~ReplaySession() > +{ > +} Empty, should be fine to inline in the header file. > Source/WebCore/replay/ReplaySession.h:49 > + unsigned identifier() const { return m_identifier; }; Nit: semicolon at end not needed > Source/WebCore/replay/SegmentedInputStorage.cpp:51 > + case InputQueue::EventLoopInput: return "(DSPTCH-LOAD)"; > + case InputQueue::LoaderMemoizedData: return "<LDMEMO-LOAD"; > + case InputQueue::ScriptMemoizedData: return "<---<---<---JSMEMO-LOAD"; > + case InputQueue::Count: return "ERROR!"; These seem a bit Cryptic. > Source/WebCore/replay/SegmentedInputStorage.cpp:79 > + m_queues.append(new QueuedInputs()); Style: constructor "()" not needed here.

Comment on attachment 224819 [details] patch part 1 - v1 View in context: https://bugs.webkit.org/attachment.cgi?id=224819&action=review >> Source/WebCore/DerivedSources.make:1149 >> + $(WebReplayScripts)/CodeGeneratorReplayInputsTemplates.py \ > > Also update Source/WebCore/make-generated-sources.sh to export WebReplayScripts. AFAIK that is a helper tool to just run DerivedSources.make outside of Xcode, and it should have all external variables, like WebReplayScripts, be defined. OK >> Source/WebCore/inspector/InspectorInstrumentation.h:1830 >> + sessionCreatedImpl(instrumentingAgents, session); > > I think these should all fast return if no frontends: > > FAST_RETURN_IF_NO_FRONTENDS(void()); > > Since the instrumentingAgentsForPage -> inspectorReplayAgent would only return the agent when a frontend is attached (it adds the agent in didCreateFrontendAndBackend). Until there is a frontend, these methods will do nothing. OK >> Source/WebCore/inspector/InspectorReplayAgent.cpp:93 >> + return sessionObject; > > Avoid churning the reference count with .release(): > return sessionObject.release(); > > (Though I'd skip the temporary RefPtr entirely here.) I tried removing it once; the temporary is needed for the static template checking stuff in Inspector::TypeBuilder. I think it sets some "completed" bit when assigning, but not returning. >> Source/WebCore/inspector/InspectorReplayAgent.cpp:147 >> + , m_page(nullptr) > > How about initializing m_page here to be pageAgent->page() and avoiding the possibility of it ever being null. OK >> Source/WebCore/inspector/InspectorReplayAgent.cpp:217 >> + m_frontendDispatcher->sessionCreated(session->identifier()); > > Should not need to if-check m_frontendDispatcher here (or anywhere in this file). If m_frontendDispatcher is null, then this agent would not be in InstrumentingAgents, and all InspectorInstrumentation calls would have flubbed. OK >> Source/WebCore/inspector/InspectorReplayAgent.h:60 >> + , public Inspector::InspectorReplayBackendDispatcherHandler { > > Nit: 1 line is normal, unless you think this is better, then we should change other inspector agents. I prefer multi-line, since there's a lot going on (especially now with final). Templates are also multi-line. >> Source/WebCore/inspector/InspectorReplayAgent.h:107 >> + > > Nit: I'd prefer to see "virtual" on all of these. They are virtual, as evident by override. But it also makes it easier them easier to find in the header. Oops >> Source/WebCore/inspector/protocol/Replay.json:18 >> + { "name": "inputOffset", "type": "integer", "description": "Offset for an event loop input within the specified session segment." } > > The description of these are the same. There should be some difference in the description if these truly are different! oops! >> Source/WebCore/inspector/protocol/Replay.json:35 >> + { "name": "type", "type": "string", "description": "Queue type" }, > > Should this be an enum? I tried that but IIRC it caused problems with the code generator. I'll double-check that. >> Source/WebCore/inspector/protocol/Replay.json:125 >> + { "name": "sessionId", "$ref": "SessionIdentifier" } > > Most of the time this is "sessionIdentifier" but here it is "sessionId". The name in the protocol is important, because this will be in the generated code and we may need to be backwards compatible with it. Lets settle on sessionIdentifier. Oops >> Source/WebCore/replay/CapturingInputCursor.cpp:78 >> +#endif // ENABLE(TIMEALPSE) > > ;) muscle memory gone bad, or something! >> Source/WebCore/replay/CapturingInputCursor.h:55 >> + CapturingInputCursor(SegmentedInputStorage*); > > explicit Nice catch, I recently changed the arity >> Source/WebCore/replay/EventLoopInputDispatcher.cpp:161 >> + m_client->didDispatchFinalInput(); > > Should this set m_running to false here? Yes, that would make sense. >> Source/WebCore/replay/FunctorInputCursor.h:66 >> + for (size_t i = 0; i < m_storage->m_queues[static_cast<size_t>(queue)]->size(); i++) > > Instead of reaching into m_storage->m_queues directly, how about providing a friend method on SegmentedInputStorage that ASSERTs the validity of "queue": > > const QueuedInputs& queue(InputQueue queue) > { > ASSERT(queue < InputQueue::Count); > return m_queues.at(offsetForInputQueue(queue)); > } > > Here you could use it to get the queue, assert the input queue is valid, and prevent some duplication in the long lines. > > SegmentedInputStorage::load and SegmentedInputStorage::queueSize could use that as well and get the ASSERT. OK. >> Source/WebCore/replay/ReplayController.cpp:113 >> + unloadSegment(true); > > Mysterious bool. You can assign to a local to give it a name: > > bool suppressNotifications = true; > unloadSegment(suppressNotifications); OK >> Source/WebCore/replay/ReplayController.cpp:298 >> + loader->frame()->document()->setInputCursor(m_activeCursor.get()); > > No frame tree traversal is needed here? This method gets called as each subframe is created. So there will never be a case when a descendant frame has navigated into the current replay session segment but its children have not. >> Source/WebCore/replay/ReplayController.cpp:310 >> +} > > We don't normally return RefPtrs. I think we would return a raw pointer here, e.g. m_loadedSession.get(). These could be inlined in the header. It should be PassRefPtr probably. The segment and session aren't owned by the controller, and callers might want to add a ref (say, if they decide to unload it). I'll look through code to see where it is used eventually. >> Source/WebCore/replay/ReplaySession.cpp:68 >> + m_segments.append(segment); > > Avoid ref count churn by doing: > m_segments.append(segment.release()); OK >> Source/WebCore/replay/SegmentedInputStorage.cpp:51 >> + case InputQueue::Count: return "ERROR!"; > > These seem a bit Cryptic. They are carefully formatted to make LOG(WebReplay, ...) spew very readable. But not apparent from looking here :) I'll add a commen.

Created attachment 225336 [details] revised patch

Attachment 225336 [details] did not pass style-queue: ERROR: Source/WebCore/replay/SerializationMethods.cpp:93: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] ERROR: Source/WebCore/replay/ReplayInputTypes.cpp:41: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] ERROR: Source/WebCore/replay/SerializationMethods.h:64: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 3 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 225336 [details] revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=225336&action=review Looks good to me too. > Source/WebCore/inspector/InspectorReplayAgent.cpp:456 > + for (auto it = m_sessionsMap.begin(); it != m_sessionsMap.end(); ++it) Could be modernized.

Committed r164986: <http://trac.webkit.org/changeset/164986>