Bug 105258 – Web Inspector: Implement the "disabled" attribute toggling for CSS stylesheets

• Status: RESOLVED
• Resolution: LATER
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

Upstreaming https://code.google.com/p/chromium/issues/detail?id=155476.

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 179899 [details]
Patch

[Build Bot]

Comment on attachment 179899 [details]
Patch

Attachment 179899 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15402334

New failing tests:
http/tests/inspector/resource-har-pages.html

[Alexander Pavlov (apavlov)]

Created attachment 180861 [details]
Patch

[Pavel Feldman]

Comment on attachment 180861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180861&action=review

> Source/WebCore/inspector/CodeGeneratorInspector.py:1071
> +                            return "%s%s" % (helper.full_name_prefix_for_use, fixed_type_name.class_name)

Why did this change? What is the diff between the old generated and new generated code?

> Source/WebCore/inspector/Inspector.json:2364
> +                    { "name": "header", "$ref": "CSSStyleSheetHeader", "description": "Updated stylesheet header." }

Why do we need this?

> Source/WebCore/inspector/Inspector.json:2482
> +                "name": "activeStyleSheetsUpdated",

I think you should have styleSheetAdded / styleSheetRemoved notifications instead.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:360
> +    SetStyleSheetDisabledAction(InspectorStyleSheet* styleSheet, bool disabled)

I think you should split stylesheet disable and stylesheet tracking changes.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:618
> +    if (m_styleSheetHeadersRequested && m_state->getBoolean(CSSAgentState::cssAgentEnabled)) {

m_styleSheetHeadersRequested can't be true without m_state->getBoolean(CSSAgentState::cssAgentEnabled) being true.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:724
> +            removedSheets.remove(newCSSSheet);

i.e. just dispatch them here.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:377
> +            this._resourceBinding._styleSheetIdToHeader[styleSheetId] = newHeader;

just patch it here.

> Source/WebCore/inspector/front-end/StylesSourceMapping.js:242
> +        setText.call(this);

You should not make content setting asynchronous.

[Alexander Pavlov (apavlov)]

Comment on attachment 180861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180861&action=review

>> Source/WebCore/inspector/CodeGeneratorInspector.py:1071
>> +                            return "%s%s" % (helper.full_name_prefix_for_use, fixed_type_name.class_name)
> 
> Why did this change? What is the diff between the old generated and new generated code?

<         void activeStyleSheetsUpdated(PassRefPtr<TypeBuilder::Array<const TypeBuilder::CSS::StyleSheetId&> > removed, PassRefPtr<TypeBuilder::Array<TypeBuilder::CSS::CSSStyleSheetHeader> > added);
---
>         void activeStyleSheetsUpdated(PassRefPtr<TypeBuilder::Array<TypeBuilder::CSS::StyleSheetId> > removed, PassRefPtr<TypeBuilder::Array<TypeBuilder::CSS::CSSStyleSheetHeader> > added);

The former is clearly wrong.

>> Source/WebCore/inspector/Inspector.json:2364
>> +                    { "name": "header", "$ref": "CSSStyleSheetHeader", "description": "Updated stylesheet header." }
> 
> Why do we need this?

Will move into the styleSheetChanged event data

>> Source/WebCore/inspector/Inspector.json:2482
>> +                "name": "activeStyleSheetsUpdated",
> 
> I think you should have styleSheetAdded / styleSheetRemoved notifications instead.

OK

>> Source/WebCore/inspector/InspectorCSSAgent.cpp:360
>> +    SetStyleSheetDisabledAction(InspectorStyleSheet* styleSheet, bool disabled)
> 
> I think you should split stylesheet disable and stylesheet tracking changes.

OK

>> Source/WebCore/inspector/InspectorCSSAgent.cpp:618
>> +    if (m_styleSheetHeadersRequested && m_state->getBoolean(CSSAgentState::cssAgentEnabled)) {
> 
> m_styleSheetHeadersRequested can't be true without m_state->getBoolean(CSSAgentState::cssAgentEnabled) being true.

Agreed on sending styleSheetAdded() for all active stylesheets in enable()

>> Source/WebCore/inspector/InspectorCSSAgent.cpp:724
>> +            removedSheets.remove(newCSSSheet);
> 
> i.e. just dispatch them here.

OK

>> Source/WebCore/inspector/front-end/CSSStyleModel.js:377
>> +            this._resourceBinding._styleSheetIdToHeader[styleSheetId] = newHeader;
> 
> just patch it here.

We'll patch it on styleSheetChanged, which will provide us with a CSSStyleSheetHeader

>> Source/WebCore/inspector/front-end/StylesSourceMapping.js:242
>> +        setText.call(this);
> 
> You should not make content setting asynchronous.

OK

[Brian Burg]

Not sure we want this feature, closing for now.