DevTools: make StylesSourceMapping in charge of creating and removing UISourceCodes

DevTools: make StylesSourceMapping in charge of creating and removing UISourceCodes

This patch makes StylesSourceMapping manage lifetime of stylesheet
UISourceCodes. This patch also makes StyleFile to sync all the changes
between all the stylesheets, fixing the editing of the stylesheets inside
the ShadowDOM.

BUG=671867
R=dgozman

[lushnikov, 2017-01-27 04:47:56]

please, take a look

I'll add a test slightly later to validate new behavior

[dgozman, 2017-01-27 23:49:13]

Looks promising!

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

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:198:
getFile(url, frame, isContentScript) {
uiSourceCode(frame, url, isContentScript)

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:206:
removeFile(uiSourceCode) {
removeUISourceCode

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:266:
createFileForStyleSheetHeader(header) {
Let's get rid of this one as well. We should expose something like
addUISourceCode(frame, contentProvider, isContentScript, mimeType).

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
(left):

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

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

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:120:
var uiSourceCode =
Why do we have to consult with NetworkProject first?

Can we instead have our own map of StyleFiles, and each of them would
create/destroy UISourceCode and put it into NetworkProject?

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:274:
if (!error)
Filtered them already.

[lushnikov, 2017-01-30 14:19:15]

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

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:206:
removeFile(uiSourceCode) {
On 2017/01/27 23:49:12, dgozman wrote:
> removeUISourceCode

I was a bit hesitant to add just another one "removeUISourceCode" method

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
(left):

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:57:
.addEventListener(SDK.ResourceTreeModel.Events.MainFrameNavigated,
this._unbindAllUISourceCodes, this)
On 2017/01/27 23:49:13, dgozman wrote:
> Why we don't need this anymore?

We receive "styleSheetRemoved" events in this case

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

https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:120:
var uiSourceCode =
On 2017/01/27 23:49:12, dgozman wrote:
> Why do we have to consult with NetworkProject first?
> 
> Can we instead have our own map of StyleFiles, and each of them would
> create/destroy UISourceCode and put it into NetworkProject?

NetworkProject already has the knowledge of creating UISourceCodes-per-frame.
I'd like to keep this logic in one place for now.

[dgozman, 2017-01-30 19:44:59]

On 2017/01/30 14:19:15, lushnikov wrote:
>
https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
> File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js
> (right):
> 
>
https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
> third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:206:
> removeFile(uiSourceCode) {
> On 2017/01/27 23:49:12, dgozman wrote:
> > removeUISourceCode
> 
> I was a bit hesitant to add just another one "removeUISourceCode" method

Adding "files" is even more confusing. I'd leave "file" word for persistence
only.

> 
>
https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
> File
> third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
> (left):
> 
>
https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:57:
> .addEventListener(SDK.ResourceTreeModel.Events.MainFrameNavigated,
> this._unbindAllUISourceCodes, this)
> On 2017/01/27 23:49:13, dgozman wrote:
> > Why we don't need this anymore?
> 
> We receive "styleSheetRemoved" events in this case

Thanks!

> 
>
https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
> File
> third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js
> (right):
> 
>
https://codereview.chromium.org/2662513003/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:120:
> var uiSourceCode =
> On 2017/01/27 23:49:12, dgozman wrote:
> > Why do we have to consult with NetworkProject first?
> > 
> > Can we instead have our own map of StyleFiles, and each of them would
> > create/destroy UISourceCode and put it into NetworkProject?
> 
> NetworkProject already has the knowledge of creating UISourceCodes-per-frame.
> I'd like to keep this logic in one place for now.

I think that is conceptually wrong. Domain-specific binding should manage
whatever relations between SDK objects it is interested in, while NetworkProject
just tags UISourceCodes and provides that information to the outer world if
needed.

[lushnikov, 2017-02-15 00:29:06]

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

[commit-bot: I haz the power, 2017-02-15 00:30:43]

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-02-15 03:17:29]

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

[commit-bot: I haz the power, 2017-02-15 03:17:30]

Dry run: This issue passed the CQ dry run.

[lushnikov, 2017-02-15 05:17:26]

please, take a look

The patchset#2 addresses all the comments, incapsulating
UISourceCode management inside StylesSourceMapping

[lushnikov, 2017-02-15 05:18:48]

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

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/CSSWorkspaceBinding.js:197:
var uiSourceCode =
in future, this green chunk should be:

   uiLocation = uiLocation ||
Bindings.ResourceBinding.rawLocationToUILocation(rawLocation);

This is needed to support inline stylesheets

[dgozman, 2017-02-15 19:57:12]

Let's try to move all UISourceCode management to StyleFile.

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

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:103:
var resource = SDK.ResourceTreeModel.resourceForURL(url);
- header.target().model(SDK.ResourceTreeModel).resourceForURL(url)
- add a utility method which takes url + target and returns metadata.

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:192:
var styleSheetId = event.data.styleSheetId;
@type ?

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:193:
var header = this._cssModel.styleSheetHeaderForId(styleSheetId);
Let's pass a header in an event instead?

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:197:
var styleFile = uiSourceCode ? this._styleFiles.get(uiSourceCode) : null;
if (!uiSourceCode) return;

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:347:
errors = errors.filter(error => !!error);
errors.filter(error => !!error).map(console.error);

[lushnikov, 2017-02-23 02:02:35]

please, take another look!

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

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:103:
var resource = SDK.ResourceTreeModel.resourceForURL(url);
On 2017/02/15 19:57:11, dgozman wrote:
> - header.target().model(SDK.ResourceTreeModel).resourceForURL(url)
> - add a utility method which takes url + target and returns metadata.

Done along this lines.

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:192:
var styleSheetId = event.data.styleSheetId;
On 2017/02/15 19:57:11, dgozman wrote:
> @type ?

Done.

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:193:
var header = this._cssModel.styleSheetHeaderForId(styleSheetId);
On 2017/02/15 19:57:11, dgozman wrote:
> Let's pass a header in an event instead?

I'll postpone this change - this CL already has a lot going on

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:197:
var styleFile = uiSourceCode ? this._styleFiles.get(uiSourceCode) : null;
On 2017/02/15 19:57:11, dgozman wrote:
> if (!uiSourceCode) return;

Done.

https://codereview.chromium.org/2662513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:347:
errors = errors.filter(error => !!error);
On 2017/02/15 19:57:11, dgozman wrote:
> errors.filter(error => !!error).map(console.error);

Done.

[lushnikov, 2017-02-23 02:02:40]

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

[commit-bot: I haz the power, 2017-02-23 02:03:17]

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

[dgozman, 2017-02-23 02:17:28]

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

https://codereview.chromium.org/2662513003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/NavigatorView.js:307:
this.uiSourceCodeAdded(uiSourceCode);
Remove.

[pfeldman, 2017-02-23 02:23:23]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2017-02-23 02:23:24]

It is entirely unclear why this makes things better, needs a design doc.

[commit-bot: I haz the power, 2017-02-23 05:48:50]

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

[commit-bot: I haz the power, 2017-02-23 05:48:51]

Dry run: This issue passed the CQ dry run.