[DevTools] Pass the frame payload

[DevTools] Pass the frame payload

Some frame information, like URL and security origin was lost because
frame payload object was not passed in the ctor. This fixes empty origin
on the "Clear storage" section on the Application tab.

BUG=678832
Review-Url: https://codereview.chromium.org/2610963005
Cr-Commit-Position: refs/heads/master@{#442426}
Committed: https://chromium.googlesource.com/chromium/src/+/ed73f79d6abbe1f7f09b0ba735c5aba03072197c

[eostroukhov, 2017-01-05 23:56:07]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 23:56:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov, 2017-01-05 23:58:19]

eostroukhov@chromium.org changed reviewers:
+ dgozman%chromium.org@gtempaccount.com, pfeldman@chromium.org

[eostroukhov, 2017-01-05 23:58:20]

Please take a look, I don't have attached bug report - just an issue I hit
looking at something else.

[pfeldman, 2017-01-06 00:03:44]

Please file a bug and associate this change with it. We might want to merge the
fix if that is regression.

[eostroukhov, 2017-01-06 00:11:08]

Description was changed from

==========
[DevTools] Pass the frame payload

Some frame information, like URL and security origin was lost because
frame payload object was not passed in the ctor. This fixes empty origin
on the "Clear storage" section on the Application tab.

BUG=NONE
==========

to

==========
[DevTools] Pass the frame payload

Some frame information, like URL and security origin was lost because
frame payload object was not passed in the ctor. This fixes empty origin
on the "Clear storage" section on the Application tab.

BUG=678832
==========

[eostroukhov, 2017-01-06 00:11:47]

Done.

[eostroukhov, 2017-01-06 00:43:48]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 00:44:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov, 2017-01-06 00:45:04]

I updated the patch, please take another look. I added commit hash to the bug
report.

[commit-bot: I haz the power, 2017-01-06 02:36:19]

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

[commit-bot: I haz the power, 2017-01-06 02:36:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pfeldman, 2017-01-06 18:45:19]

https://codereview.chromium.org/2610963005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2610963005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:206:
this._securityOriginManager.setMainSecurityOrigin(frame.url);
You are setting url in place of security origin.

[eostroukhov, 2017-01-06 19:14:11]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 19:14:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov, 2017-01-06 19:28:24]

Thank you for the review. I've updated the CL, please take another look.

https://codereview.chromium.org/2610963005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2610963005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:206:
this._securityOriginManager.setMainSecurityOrigin(frame.url);
On 2017/01/06 18:45:19, pfeldman wrote:
> You are setting url in place of security origin.

Done.

[pfeldman, 2017-01-06 19:49:08]

https://codereview.chromium.org/2610963005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2610963005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:206:
this._securityOriginManager.setMainSecurityOrigin(frame.securityOrigin);
You already have "addedOrigin" and you should do that from under if
(addedOrigin) I guess.

[commit-bot: I haz the power, 2017-01-06 20:59:19]

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

[commit-bot: I haz the power, 2017-01-06 20:59:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[eostroukhov, 2017-01-06 22:19:13]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 22:19:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov, 2017-01-06 22:22:34]

Thanks. Change was updated, please take another look.

https://codereview.chromium.org/2610963005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2610963005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:206:
this._securityOriginManager.setMainSecurityOrigin(frame.securityOrigin);
On 2017/01/06 19:49:08, pfeldman wrote:
> You already have "addedOrigin" and you should do that from under if
> (addedOrigin) I guess.

I joined the addSecurityOrigin and setMainSecurityOrigin

[commit-bot: I haz the power, 2017-01-06 23:52:37]

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

[commit-bot: I haz the power, 2017-01-06 23:52:38]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-01-07 00:33:18]

https://codereview.chromium.org/2610963005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js
(right):

https://codereview.chromium.org/2610963005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js:47:
removeSecurityOrigin(securityOrigin) {
I don't think this counter makes sense - _navigate would clean up childFrames,
but not update the securityorigin counters. What you should do instead is
traverse frames every time they change and compare sets of origins.

[eostroukhov, 2017-01-09 19:31:37]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 19:32:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov, 2017-01-09 19:34:52]

I changed the logic so now it recomputes entire list of origins. This resulted
in two cases when events were reshuffled:
1. Origin removed event is now fired after the navigation event, not before.
2. Origin removed event is now fired before each detached, they are no longer
batched.

Please take a look.

[pfeldman, 2017-01-09 20:06:14]

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:136:
this._updateSecurityOrigins();
You should unconditionally update origins.

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:211:
frame._remove();
You should update origins here as well.

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:431: var
securityOrigins = [];
You should use Set here, otherwise you report same origin as added multiple
times.

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:651:
this._model._updateSecurityOrigins();
You are calling this in a loop, I'd rather intercept higher level calls.

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js
(right):

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js:14:
this._securityOrigins = new Set();
Please annotate type (Set of strings)

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js:29:
updateSecurityOrigins(securityOrigins) {
Please annotate securityOrigins type, should be Set of strings.

[eostroukhov, 2017-01-09 20:31:09]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 20:31:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov, 2017-01-09 20:32:01]

Updated, please consider.

These changes resulted in further changes in the notifications order.

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:136:
this._updateSecurityOrigins();
On 2017/01/09 20:06:13, pfeldman wrote:
> You should unconditionally update origins.

Done.

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:431: var
securityOrigins = [];
On 2017/01/09 20:06:13, pfeldman wrote:
> You should use Set here, otherwise you report same origin as added multiple
> times.

Done.

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:651:
this._model._updateSecurityOrigins();
On 2017/01/09 20:06:13, pfeldman wrote:
> You are calling this in a loop, I'd rather intercept higher level calls.

Done.

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js
(right):

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js:14:
this._securityOrigins = new Set();
On 2017/01/09 20:06:13, pfeldman wrote:
> Please annotate type (Set of strings)

Done.

https://codereview.chromium.org/2610963005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js:29:
updateSecurityOrigins(securityOrigins) {
On 2017/01/09 20:06:13, pfeldman wrote:
> Please annotate securityOrigins type, should be Set of strings.

Done.

[pfeldman, 2017-01-09 20:34:47]

lgtm % nit

https://codereview.chromium.org/2610963005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js
(right):

https://codereview.chromium.org/2610963005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js:35:
this._securityOrigins = new Set(securityOrigins);
No need to wrap in a new set, it is already a set.

[eostroukhov, 2017-01-09 22:08:30]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 22:09:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov, 2017-01-09 22:09:49]

Updated, please take another look.

https://codereview.chromium.org/2610963005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js
(right):

https://codereview.chromium.org/2610963005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/SecurityOriginManager.js:35:
this._securityOrigins = new Set(securityOrigins);
On 2017/01/09 20:34:47, pfeldman wrote:
> No need to wrap in a new set, it is already a set.

I don't trust API users... But I changed the code.

[eostroukhov, 2017-01-09 22:10:06]

The CQ bit was unchecked by eostroukhov@chromium.org

[eostroukhov, 2017-01-09 22:10:08]

The CQ bit was checked by eostroukhov@chromium.org

[eostroukhov, 2017-01-09 22:10:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2610963005/#ps120001
(title: "[DevTools] Pass the frame payload")

[commit-bot: I haz the power, 2017-01-09 22:10:40]

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-01-10 00:39:20]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483999808043390,
"parent_rev": "c95c89bebf9003e535e609060cd19849ca22a19f", "commit_rev":
"ed73f79d6abbe1f7f09b0ba735c5aba03072197c"}

[commit-bot: I haz the power, 2017-01-10 00:39:50]

Description was changed from

==========
[DevTools] Pass the frame payload

Some frame information, like URL and security origin was lost because
frame payload object was not passed in the ctor. This fixes empty origin
on the "Clear storage" section on the Application tab.

BUG=678832
==========

to

==========
[DevTools] Pass the frame payload

Some frame information, like URL and security origin was lost because
frame payload object was not passed in the ctor. This fixes empty origin
on the "Clear storage" section on the Application tab.

BUG=678832

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

[commit-bot: I haz the power, 2017-01-10 00:39:52]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ed73f79d6abbe1f7f09b0ba735c5...