DevTools: make CompilerScriptMapping / SASSSourceMapping manage UISourceCodes

DevTools: make CompilerScriptMapping / SASSSourceMapping manage UISourceCodes

This patch:
- introduce frame attribution methods in NetworkProject
- change CompilerScriptMapping / SASSSourceMapping to fully manage related
  UISourceCodes by themselves. They create a single UISourceCode per target
  instead of UISourceCode-per-frame approach.

R=dgozman
BUG=670180
Review-Url: https://codereview.chromium.org/2869293002
Cr-Commit-Position: refs/heads/master@{#471960}
Committed: https://chromium.googlesource.com/chromium/src/+/abb90a5c3d4a074c35fb3b60fdeddda93aaef865

[lushnikov, 2017-05-10 01:59:06]

please, take a look

[lushnikov, 2017-05-10 01:59:11]

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

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

Dry run: 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-05-10 04:04:07]

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

[commit-bot: I haz the power, 2017-05-10 04:04:08]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[dgozman, 2017-05-10 18:10:33]

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js
(right):

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:111:
return (isContentScript ? 'extensions:' : '') + 'jsSourceMaps:' + target.id();
inline it?

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:111:
return (isContentScript ? 'extensions:' : '') + 'jsSourceMaps:' + target.id();
Let's keep the equal amount of ':' in project ids.

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:177:
var script = scripts.length ? scripts[0] : null;
So, this is kind of wrong now?

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:230:
for (var sourceURL of sourceMap.sourceURLs()) {
Let's combine both for-loops.

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/bindings/SASSSourceMapping.js
(right):

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/SASSSourceMapping.js:39:
static projectId(target) {
inline it

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/SASSSourceMapping.js:106:
if (!headers.length) {
Let's rewrite below for clarity:

for (var sassURL of sourceMap.sourceURLS()) {
  if (headers.length) {
    var uiSourceCode = ...;
    changeAttribution
  } else {
    this._project.removeFile(sassURL);
  }
}
updateLocations(header);

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/SASSSourceMapping.js:113:
var attribution = new Set(headers.map(header => header.frameId));
Doing this here looks ugly. Should we instead introduce two methods:
NetworkProject.{add,remove}FrameAttribution ?

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:251:
this._removeUISourceCode(uiSourceCode);
This is not very efficient if UISourceCode gets many frames one by one.

[lushnikov, 2017-05-10 22:24:33]

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js
(right):

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:111:
return (isContentScript ? 'extensions:' : '') + 'jsSourceMaps:' + target.id();
On 2017/05/10 18:10:33, dgozman wrote:
> inline it?

Done.

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:111:
return (isContentScript ? 'extensions:' : '') + 'jsSourceMaps:' + target.id();
On 2017/05/10 18:10:33, dgozman wrote:
> Let's keep the equal amount of ':' in project ids.

Done.

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:177:
var script = scripts.length ? scripts[0] : null;
On 2017/05/10 18:10:33, dgozman wrote:
> So, this is kind of wrong now?

The whole thing of translating singe uiLocation into a single rawLocation is
sketchy - it asks to return a single rawLocation, whereas there are many of
them.

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:230:
for (var sourceURL of sourceMap.sourceURLs()) {
On 2017/05/10 18:10:33, dgozman wrote:
> Let's combine both for-loops.

Done.

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/bindings/SASSSourceMapping.js
(right):

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/SASSSourceMapping.js:39:
static projectId(target) {
On 2017/05/10 18:10:33, dgozman wrote:
> inline it

Done.

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/SASSSourceMapping.js:106:
if (!headers.length) {
On 2017/05/10 18:10:33, dgozman wrote:
> Let's rewrite below for clarity:
> 
> for (var sassURL of sourceMap.sourceURLS()) {
>   if (headers.length) {
>     var uiSourceCode = ...;
>     changeAttribution
>   } else {
>     this._project.removeFile(sassURL);
>   }
> }
> updateLocations(header);

Done.

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/SASSSourceMapping.js:113:
var attribution = new Set(headers.map(header => header.frameId));
On 2017/05/10 18:10:33, dgozman wrote:
> Doing this here looks ugly. Should we instead introduce two methods:
> NetworkProject.{add,remove}FrameAttribution ?

In case of shadowDOM we might have many headers attributed to the same frame.
Given that
frameAttribution is a set, simply having {add/remove}FrameAttribution would not
be enough.
We'd additionally need to have a multimap to count exact number of headers in
frame.

This line neatly addresses this shadowDOM scenario - wdyt?

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/2869293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:251:
this._removeUISourceCode(uiSourceCode);
On 2017/05/10 18:10:33, dgozman wrote:
> This is not very efficient if UISourceCode gets many frames one by one.

Done.

[lushnikov, 2017-05-10 22:24:38]

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

[commit-bot: I haz the power, 2017-05-10 22:25:40]

Dry run: 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-05-11 00:35:06]

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

[commit-bot: I haz the power, 2017-05-11 00:35:07]

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

[lushnikov, 2017-05-11 21:17:44]

I switched api from changeFrameAttribution to {add,remove}FrameAttribution.

Please, take another look

[lushnikov, 2017-05-11 21:20:25]

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

[commit-bot: I haz the power, 2017-05-11 21:21:48]

Dry run: 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-05-11 22:31:16]

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

[commit-bot: I haz the power, 2017-05-11 22:31:17]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dgozman, 2017-05-11 23:55:16]

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js
(right):

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:63:
FrameAttributionChanged: Symbol('FrameAttributionChanged')
Make it two events.

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:138:
static setInitialFrameAttribution(uiSourceCode, frameId) {
Where is the "copy from" parameter?

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:174:
count, `Failed to remove frame attribution for url = ${uiSourceCode.url()} and
frameId = ${frameId}`);
This is slowing things down unnecessary.

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:182: if
(!frame)
This is sketchy - you will not send a removed event sometimes and it will stick.

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:256: var
node = Array.from(this._uiSourceCodeNodes.get(uiSourceCode)).find(node =>
node.frame() === removedFrame);
Why Array.from? It's already an array.

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:539: 
nit: extra blank line

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:554:
this._uiSourceCodeNodes.remove(uiSourceCode, node);
You mutate the same collection in line 545. That's ok?

[lushnikov, 2017-05-12 01:18:00]

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js
(right):

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:63:
FrameAttributionChanged: Symbol('FrameAttributionChanged')
On 2017/05/11 23:55:16, dgozman wrote:
> Make it two events.

Done.

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:138:
static setInitialFrameAttribution(uiSourceCode, frameId) {
On 2017/05/11 23:55:15, dgozman wrote:
> Where is the "copy from" parameter?

It's not needed until we migrate ResourceScriptMapping on this.

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:174:
count, `Failed to remove frame attribution for url = ${uiSourceCode.url()} and
frameId = ${frameId}`);
On 2017/05/11 23:55:16, dgozman wrote:
> This is slowing things down unnecessary.

Done.

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:182: if
(!frame)
On 2017/05/11 23:55:16, dgozman wrote:
> This is sketchy - you will not send a removed event sometimes and it will
stick.

Done.

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:256: var
node = Array.from(this._uiSourceCodeNodes.get(uiSourceCode)).find(node =>
node.frame() === removedFrame);
On 2017/05/11 23:55:16, dgozman wrote:
> Why Array.from? It's already an array.

Nah, it's a set

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:539: 
On 2017/05/11 23:55:16, dgozman wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/2869293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:554:
this._uiSourceCodeNodes.remove(uiSourceCode, node);
On 2017/05/11 23:55:16, dgozman wrote:
> You mutate the same collection in line 545. That's ok?

That's ok for sets (we have set here):
http://stackoverflow.com/questions/28306756/is-it-safe-to-delete-elements-in-...

[lushnikov, 2017-05-12 01:20:47]

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

[commit-bot: I haz the power, 2017-05-12 01:21:41]

Dry run: 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-05-12 02:56:58]

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

[commit-bot: I haz the power, 2017-05-12 02:56:59]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-05-12 19:12:51]

lgtm

https://codereview.chromium.org/2869293002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js
(right):

https://codereview.chromium.org/2869293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:218:
scripts = scripts.filter(someScript => someScript.isContentScript() ===
script.isContentScript());
var hasOtherScripts = scripts.some(...);

https://codereview.chromium.org/2869293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/2869293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:257:
this._addUISourceCodeNode(uiSourceCode, addedFrame);
// This event does not happen for UISourceCodes without initial attribution.

[lushnikov, 2017-05-15 21:05:02]

https://codereview.chromium.org/2869293002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js
(right):

https://codereview.chromium.org/2869293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:218:
scripts = scripts.filter(someScript => someScript.isContentScript() ===
script.isContentScript());
On 2017/05/12 19:12:50, dgozman wrote:
> var hasOtherScripts = scripts.some(...);

Done.

https://codereview.chromium.org/2869293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js
(right):

https://codereview.chromium.org/2869293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:257:
this._addUISourceCodeNode(uiSourceCode, addedFrame);
On 2017/05/12 19:12:50, dgozman wrote:
> // This event does not happen for UISourceCodes without initial attribution.

Done.

[lushnikov, 2017-05-15 21:25:34]

The CQ bit was checked by lushnikov@chromium.org

[lushnikov, 2017-05-15 21:25:34]

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/2869293002/#ps100001
(title: "address comments")

[commit-bot: I haz the power, 2017-05-15 21:26:57]

CQ is trying da patch.

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

[lushnikov, 2017-05-15 23:01:09]

The CQ bit was unchecked by lushnikov@chromium.org

[lushnikov, 2017-05-15 23:01:09]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2017-05-15 23:02:59]

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-05-16 00:49:21]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494889269779890,
"parent_rev": "3e830769ded798bf6680d5c862445ff2b0733d6b", "commit_rev":
"abb90a5c3d4a074c35fb3b60fdeddda93aaef865"}

[commit-bot: I haz the power, 2017-05-16 00:49:43]

Description was changed from

==========
DevTools: make CompilerScriptMapping / SASSSourceMapping manage UISourceCodes

This patch:
- introduce frame attribution methods in NetworkProject
- change CompilerScriptMapping / SASSSourceMapping to fully manage related
  UISourceCodes by themselves. They create a single UISourceCode per target
  instead of UISourceCode-per-frame approach.

R=dgozman
BUG=670180
==========

to

==========
DevTools: make CompilerScriptMapping / SASSSourceMapping manage UISourceCodes

This patch:
- introduce frame attribution methods in NetworkProject
- change CompilerScriptMapping / SASSSourceMapping to fully manage related
  UISourceCodes by themselves. They create a single UISourceCode per target
  instead of UISourceCode-per-frame approach.

R=dgozman
BUG=670180

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

[commit-bot: I haz the power, 2017-05-16 00:49:44]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/abb90a5c3d4a074c35fb3b60fded...