[Devtools][Regression] Fixed websocket frame selection loss on frame received

[Devtools][Regression] Fixed websocket frame selection loss on frame received

Fixed bug where if a websocket frame was received it would sometimes
move the selected node in network panel to a new request.

Offending patch: https://codereview.chromium.org/2694103007

R=dgozman,caseq
BUG=714229
Review-Url: https://codereview.chromium.org/2838673003
Cr-Commit-Position: refs/heads/master@{#468185}
Committed: https://chromium.googlesource.com/chromium/src/+/fc4b4d8fbe2657a7d51cef8939e68acc99fae05a

[allada, 2017-04-25 03:18:55]

Patchset #1 (id:1) has been deleted

[allada, 2017-04-25 03:19:12]

PTL

[allada, 2017-04-25 03:19:12]

PTL

[dgozman, 2017-04-25 21:20:46]

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js
(right):

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:80:
InspectorTest.sdkRequestStartedFromUrlPromise = function(url)
Doesn't InspectorTest.findRequestsByURLPattern work in your case?

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:95:
InspectorTest.makeWebsocketPromise = function(url)
Only add "promise" at the end if there is non-promise version already.

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:120: var
promises = InspectorTest._sdkResponseReceivedPromises.get(request);
It's easier to add/remove listener right here, instead of having a global
listener and a map.

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/websocket/network-preserve-selection-on-frame-receive.html
(right):

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/websocket/network-preserve-selection-on-frame-receive.html:21:
InspectorTest.addResult("Selected Request: " + (dataGrid.selectedNode &&
dataGrid.selectedNode.request().url()) || '');
Extract dumpSelectedRequest function.

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/websocket/network-preserve-selection-on-frame-receive.html:24:
await InspectorTest.callFunctionInPageOnRemoteObjectAsync(wsRemoteObject,
"this.send('test')");
Why do we need remote object? Just store it on window in the page.

[allada, 2017-04-27 17:03:19]

PTaL

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js
(right):

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:80:
InspectorTest.sdkRequestStartedFromUrlPromise = function(url)
On 2017/04/25 21:20:45, dgozman wrote:
> Doesn't InspectorTest.findRequestsByURLPattern work in your case?

Partially, My goal was to have a function that would resolve when the request is
found.

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:95:
InspectorTest.makeWebsocketPromise = function(url)
On 2017/04/25 21:20:45, dgozman wrote:
> Only add "promise" at the end if there is non-promise version already.

Done.

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:120: var
promises = InspectorTest._sdkResponseReceivedPromises.get(request);
On 2017/04/25 21:20:45, dgozman wrote:
> It's easier to add/remove listener right here, instead of having a global
> listener and a map.

Done.

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/websocket/network-preserve-selection-on-frame-receive.html
(right):

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/websocket/network-preserve-selection-on-frame-receive.html:21:
InspectorTest.addResult("Selected Request: " + (dataGrid.selectedNode &&
dataGrid.selectedNode.request().url()) || '');
On 2017/04/25 21:20:46, dgozman wrote:
> Extract dumpSelectedRequest function.

Done.

https://codereview.chromium.org/2838673003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/websocket/network-preserve-selection-on-frame-receive.html:24:
await InspectorTest.callFunctionInPageOnRemoteObjectAsync(wsRemoteObject,
"this.send('test')");
On 2017/04/25 21:20:46, dgozman wrote:
> Why do we need remote object? Just store it on window in the page.

I was trying to make the code verbose in what it is doing. If I store it on the
main page and work directly with it, than we end up with black magic and
concurrency hell. I feel it is more natural to say, "create this object in page"
... "run this code on this object that should exist in page", rather than
calling methods in the page and forcing global state. (keep in mind there are
few cases where RemoteObjects make sense for this, but I believe this is one of
them.

[allada, 2017-04-28 20:23:59]

PTaL

[dgozman, 2017-04-28 20:49:31]

Beautiful! lgtm

https://codereview.chromium.org/2838673003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js
(right):

https://codereview.chromium.org/2838673003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:84: *
@return {?Network.NetworkRequestNode}
Promise<...>

https://codereview.chromium.org/2838673003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:86:
InspectorTest.awaitNetworkLogViewNodeForRequest = function(request)
waitForNetworkLogViewNode(request)

[allada, 2017-04-28 20:54:41]

https://codereview.chromium.org/2838673003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js
(right):

https://codereview.chromium.org/2838673003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:84: *
@return {?Network.NetworkRequestNode}
On 2017/04/28 20:49:31, dgozman wrote:
> Promise<...>

Done.

https://codereview.chromium.org/2838673003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:86:
InspectorTest.awaitNetworkLogViewNodeForRequest = function(request)
On 2017/04/28 20:49:31, dgozman wrote:
> waitForNetworkLogViewNode(request)

Done.

[allada, 2017-04-28 20:54:45]

The CQ bit was checked by allada@chromium.org

[allada, 2017-04-28 20:54:46]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2838673003/#ps80001
(title: "changes")

[commit-bot: I haz the power, 2017-04-28 20:55:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 23:40:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 23:40:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[allada, 2017-04-28 23:42:17]

The CQ bit was checked by allada@chromium.org

[commit-bot: I haz the power, 2017-04-28 23:42:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 00:10:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493422937397260,
"parent_rev": "0a54e8804a182388f0aedc8a18524eaa10e76b85", "commit_rev":
"fc4b4d8fbe2657a7d51cef8939e68acc99fae05a"}

[commit-bot: I haz the power, 2017-04-29 00:11:26]

Description was changed from

==========
[Devtools][Regression] Fixed websocket frame selection loss on frame received

Fixed bug where if a websocket frame was received it would sometimes
move the selected node in network panel to a new request.

Offending patch: https://codereview.chromium.org/2694103007

R=dgozman,caseq
BUG=714229
==========

to

==========
[Devtools][Regression] Fixed websocket frame selection loss on frame received

Fixed bug where if a websocket frame was received it would sometimes
move the selected node in network panel to a new request.

Offending patch: https://codereview.chromium.org/2694103007

R=dgozman,caseq
BUG=714229

Review-Url: https://codereview.chromium.org/2838673003
Cr-Commit-Position: refs/heads/master@{#468185}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc4b4d8fbe2657a7d51cef8939e6...
==========

[commit-bot: I haz the power, 2017-04-29 00:11:28]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/fc4b4d8fbe2657a7d51cef8939e6...