DevTools: make StyleSourceMapping in charge of managing UISourceCodes

DevTools: make StyleSourceMapping in charge of managing UISourceCodes

This patch:
- move style-related UISourceCode management from NetworkProject into
  StylesSourceMapping
- create only a single UISourceCode per target for css header

As a result, all the headers are synced perfectly across shadow roots
and frames.

BUG=671867, 670180
R=dgozman

Review-Url: https://codereview.chromium.org/2893523002
Cr-Commit-Position: refs/heads/master@{#479168}
Committed: https://chromium.googlesource.com/chromium/src/+/c5b8d597dda99a31ae014519984d51c994f3f55d

[lushnikov, 2017-05-17 02:56:55]

Description was changed from

==========
DevTools: make StyleSourceMapping in charge of managing UISourceCodes

This patch:
- move style-related UISourceCode management from NetworkProject into
  StylesSourceMapping
- create only a single UISourceCode per target for css header

As a result, all the headers are synced perfectly across shadow roots
and frames.

BUG=671867
R=dgozman
==========

to

==========
DevTools: make StyleSourceMapping in charge of managing UISourceCodes

This patch:
- move style-related UISourceCode management from NetworkProject into
  StylesSourceMapping
- create only a single UISourceCode per target for css header

As a result, all the headers are synced perfectly across shadow roots
and frames.

BUG=671867,670180
R=dgozman
==========

[lushnikov, 2017-05-17 02:57:36]

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

[lushnikov, 2017-05-17 02:58:45]

please, take a look!

[commit-bot: I haz the power, 2017-05-17 02:59:18]

Dry run: CQ is trying da patch.

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

[lushnikov, 2017-05-17 03:54:23]

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

[commit-bot: I haz the power, 2017-05-17 03:54:37]

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-17 04:58:51]

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

[commit-bot: I haz the power, 2017-05-17 04:58:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgozman, 2017-05-17 16:46:20]

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/elements/styles-1/edit-resource-referred-by-multiple-styletags-expected.txt
(right):

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/elements/styles-1/edit-resource-referred-by-multiple-styletags-expected.txt:8:
Both headers and uiSourceCode content:
Nice test! I was about to ask for one :-)

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

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/CSSWorkspaceBinding.js:238:
var uiSourceCode = Bindings.NetworkProject.uiSourceCodeForResourceURL(
Should we have a ResourceMapping which will do this instead? It can register
itself as mapping both in JS and CSS worlds, and only resolve locations but not
perform any content syncing.

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

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:222:
uiSourceCode[Bindings.NetworkProject._forceCanonicalMimeType] = true;
I feel like we should incorporate mime type into metadata. WDYT?

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

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js:94:
Bindings.metadataForURL = function(target, frameId, url) {
resourceMetadataForURL?

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
(left):

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:59:
.addEventListener(SDK.ResourceTreeModel.Events.MainFrameNavigated,
this._unbindAllUISourceCodes, this)
Why don't we need this anymore?

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:248:
_styleSheetChanged(event) {
I feel like moving all these method could be postponed until next patch, which
will make reasoning much easier. WDYT?

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

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:214:
headers() {
I find these "API" methods between tightly coupled classes redundant. WDYT?

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:265:
newContent = await fromProvider.requestContent();
Per recent discussions, you have to add comment before this line:
// -------- ASYNC ---------

But I'd rather split this method into parts.

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:285:
var errors = await Promise.all(promises);
Same here.

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

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:100:
subscribeToStyleSheetChanged(headerId, callback) {
Why subscribe here and not route through StyleSourceMapping? Should be easy
enough.

[lushnikov, 2017-05-20 01:24:18]

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

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/CSSWorkspaceBinding.js:238:
var uiSourceCode = Bindings.NetworkProject.uiSourceCodeForResourceURL(
On 2017/05/17 16:46:20, dgozman wrote:
> Should we have a ResourceMapping which will do this instead? It can register
> itself as mapping both in JS and CSS worlds, and only resolve locations but
not
> perform any content syncing.

Done - introduced here: https://codereview.chromium.org/2893073002/

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

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:222:
uiSourceCode[Bindings.NetworkProject._forceCanonicalMimeType] = true;
On 2017/05/17 16:46:20, dgozman wrote:
> I feel like we should incorporate mime type into metadata. WDYT?

Done - here: https://codereview.chromium.org/2889013002/

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
(left):

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:59:
.addEventListener(SDK.ResourceTreeModel.Events.MainFrameNavigated,
this._unbindAllUISourceCodes, this)
On 2017/05/17 16:46:20, dgozman wrote:
> Why don't we need this anymore?

1. CSSModel handles MainFrameNavigation by itself (that's why we don't have it
in NetworkProject)
2. The StyleSheetChanged event got moved inside the StyleFile since it's the one
who takes action.

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:248:
_styleSheetChanged(event) {
On 2017/05/17 16:46:20, dgozman wrote:
> I feel like moving all these method could be postponed until next patch, which
> will make reasoning much easier. WDYT?

This is possible, though I feel it's harder to validate this code wrt the
changes I'm doing in this patch rather then validate the new code (it's 130
lines of old code vs 60 lines of new code)

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

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:214:
headers() {
On 2017/05/17 16:46:20, dgozman wrote:
> I find these "API" methods between tightly coupled classes redundant. WDYT?

Agreed and inlined both headers() and uiSourceCode() methods. The
addHeader/removeHeader calls still make sense to me.

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:265:
newContent = await fromProvider.requestContent();
On 2017/05/17 16:46:20, dgozman wrote:
> Per recent discussions, you have to add comment before this line:
> // -------- ASYNC ---------
> 
> But I'd rather split this method into parts.

Added the ASYNC comment.

The previous version of this patch had this method split into two:
https://codereview.chromium.org/2662513003/

However, I find it harder to reason about, since the two methods are pumped
through
the same throttler instance.

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:285:
var errors = await Promise.all(promises);
On 2017/05/17 16:46:20, dgozman wrote:
> Same here.

Done.

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

https://codereview.chromium.org/2893523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:100:
subscribeToStyleSheetChanged(headerId, callback) {
On 2017/05/17 16:46:20, dgozman wrote:
> Why subscribe here and not route through StyleSourceMapping? Should be easy
> enough.

Subscribe/unsubscribe is a natural API to get updates per CSSStyleSheetHeader.
We needed it in a few places before, so I finally decided to introduce the API

[lushnikov, 2017-05-20 01:28:01]

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

[commit-bot: I haz the power, 2017-05-20 01:28:38]

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-20 04:43:22]

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

[commit-bot: I haz the power, 2017-05-20 04:43:23]

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

[lushnikov, 2017-06-09 18:45:31]

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

[commit-bot: I haz the power, 2017-06-09 18:45:55]

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-06-09 21:20:11]

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

[commit-bot: I haz the power, 2017-06-09 21:20:13]

Dry run: This issue passed the CQ dry run.

[lushnikov, 2017-06-12 22:12:47]

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

[commit-bot: I haz the power, 2017-06-12 22:13:23]

Dry run: CQ is trying da patch.

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

[lushnikov, 2017-06-12 23:49:18]

@dgozman's comments

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
(right):

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:147:
this._styleFiles.clear();
remove project

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:179:
this._cssModel.subscribeToStyleSheetChanged(header.id,
this._styleSheetChangedBound);
Let's route through parent mapping.

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:244:
var newContent = null;
check for this._terminated

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:273:
errors.filter(error => !!error).forEach(console.error);
let's drop these

[commit-bot: I haz the power, 2017-06-13 00:46:12]

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

[commit-bot: I haz the power, 2017-06-13 00:46:13]

Dry run: This issue passed the CQ dry run.

[lushnikov, 2017-06-13 01:17:43]

all done; please, take another look!

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
(right):

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:147:
this._styleFiles.clear();
On 2017/06/12 23:49:17, lushnikov wrote:
> remove project

Done.

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:179:
this._cssModel.subscribeToStyleSheetChanged(header.id,
this._styleSheetChangedBound);
On 2017/06/12 23:49:17, lushnikov wrote:
> Let's route through parent mapping.

Done.

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:244:
var newContent = null;
On 2017/06/12 23:49:17, lushnikov wrote:
> check for this._terminated

Done.

https://codereview.chromium.org/2893523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:273:
errors.filter(error => !!error).forEach(console.error);
On 2017/06/12 23:49:18, lushnikov wrote:
> let's drop these

Done.

[lushnikov, 2017-06-13 01:20:27]

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

[commit-bot: I haz the power, 2017-06-13 01:21:00]

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-06-13 03:18:06]

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

[commit-bot: I haz the power, 2017-06-13 03:18:07]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-06-13 19:09:38]

lgtm

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/elements/styles-1/edit-resource-referred-by-multiple-styletags.html
(right):

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/styles-1/edit-resource-referred-by-multiple-styletags.html:50:
function waitForStyleFile() {
nit: you can use addSnifferPromise

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
(right):

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:145:
_styleSheetChanged(event) {
JSDoc

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:218:
if (this._isUpdatingHeaders || !this._headers.has(header))
Let's assert instead.

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:251:
return;
this._styleFileSyncedForTest();

[lushnikov, 2017-06-13 19:25:22]

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/elements/styles-1/edit-resource-referred-by-multiple-styletags.html
(right):

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/styles-1/edit-resource-referred-by-multiple-styletags.html:50:
function waitForStyleFile() {
On 2017/06/13 19:09:38, dgozman wrote:
> nit: you can use addSnifferPromise

Done.

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
(right):

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:145:
_styleSheetChanged(event) {
On 2017/06/13 19:09:38, dgozman wrote:
> JSDoc

Done.

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:218:
if (this._isUpdatingHeaders || !this._headers.has(header))
On 2017/06/13 19:09:38, dgozman wrote:
> Let's assert instead.

Done.

https://codereview.chromium.org/2893523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:251:
return;
On 2017/06/13 19:09:38, dgozman wrote:
> this._styleFileSyncedForTest();

Done.

[lushnikov, 2017-06-13 19:29:32]

The CQ bit was checked by lushnikov@chromium.org

[lushnikov, 2017-06-13 19:29:32]

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

[commit-bot: I haz the power, 2017-06-13 19:30:02]

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-06-13 21:57:32]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497382172070420,
"parent_rev": "89a0f29866b240fb8e058c00af8bc09c4927639e", "commit_rev":
"c5b8d597dda99a31ae014519984d51c994f3f55d"}

[commit-bot: I haz the power, 2017-06-13 21:57:44]

Description was changed from

==========
DevTools: make StyleSourceMapping in charge of managing UISourceCodes

This patch:
- move style-related UISourceCode management from NetworkProject into
  StylesSourceMapping
- create only a single UISourceCode per target for css header

As a result, all the headers are synced perfectly across shadow roots
and frames.

BUG=671867,670180
R=dgozman
==========

to

==========
DevTools: make StyleSourceMapping in charge of managing UISourceCodes

This patch:
- move style-related UISourceCode management from NetworkProject into
  StylesSourceMapping
- create only a single UISourceCode per target for css header

As a result, all the headers are synced perfectly across shadow roots
and frames.

BUG=671867,670180
R=dgozman

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

[commit-bot: I haz the power, 2017-06-13 21:57:45]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/c5b8d597dda99a31ae014519984d...