Bug 135212 - Web Inspector: ReplayManager shouldn't assume replay status when the inspector is opened
Summary: Web Inspector: ReplayManager shouldn't assume replay status when the inspecto...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 135225
  Show dependency treegraph
 
Reported: 2014-07-23 14:33 PDT by Brian Burg
Modified: 2014-08-05 14:59 PDT (History)
5 users (show)

See Also:


Attachments
Patch (21.43 KB, patch)
2014-07-23 15:06 PDT, Brian Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-07-23 14:33:54 PDT
The current implementation blindly assumes that no capturing or replaying has happened yet when the ReplayManager is being constructed. This breaks replay when the inspector is closed and then later reopened.
Comment 1 Radar WebKit Bug Importer 2014-07-23 14:34:28 PDT
<rdar://problem/17785105>
Comment 2 Brian Burg 2014-07-23 15:06:36 PDT
Created attachment 235384 [details]
Patch
Comment 3 WebKit Commit Bot 2014-07-23 15:09:27 PDT
Attachment 235384 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorReplayAgent.h:107:  The parameter name "sessionState" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/inspector/InspectorReplayAgent.h:107:  The parameter name "segmentState" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Timothy Hatcher 2014-08-05 11:44:37 PDT
Comment on attachment 235384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235384&action=review

> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:55
> +    this._initializationPromise = ReplayAgent.currentReplayState.promise()

No need for .promise() now.

> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:68
> +        })
> +        .then(function() {

Same line?

> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:69
> +            return ReplayAgent.getAvailableSessions.promise();

No .promise().

> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:210
> +        if (!this._initialized)
> +            return this.waitUntilInitialized().then(this.captureStarted.bind(this));

Very cool! Too bad this can't be more automagic of all of these.
Comment 5 Brian Burg 2014-08-05 14:57:13 PDT
Committed r172087: <http://trac.webkit.org/changeset/172087>
Comment 6 Brian Burg 2014-08-05 14:59:02 PDT
Comment on attachment 235384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235384&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:55
>> +    this._initializationPromise = ReplayAgent.currentReplayState.promise()
> 
> No need for .promise() now.

That's not landed yet.

>> Source/WebInspectorUI/UserInterface/Controllers/ReplayManager.js:210
>> +            return this.waitUntilInitialized().then(this.captureStarted.bind(this));
> 
> Very cool! Too bad this can't be more automagic of all of these.

It would require some sort of property proxying thing, which will be just as slow and a bunch of work.. :(