Bug 74292 – Web Inspector: [Styles] Update selected DOM element styles whenever applicable media queries change

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Whenever media query aspects (like orientation, max-width, etc.) change, it would be useful to dynamically update the matched rules for the current DOM element.

[Alexander Pavlov (apavlov)]

Created attachment 119208 [details]
Patch

[Pavel Feldman]

Comment on attachment 119208 [details]
Patch

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

> Source/WebCore/inspector/CodeGeneratorInspector.py:207
> +    skip_js_bind_domains = set(["Runtime", "DOMDebugger"])

I think we should unskip everything. Why does it even exists?

> Source/WebCore/inspector/Inspector.json:1749
> +                "description": "Fires whenever a MediaQuery result changes (the current implementation considers only viewport-dependent media features.)"

This is not clear to me. Could you elaborate more on when the event is actually fired?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:237
> +        m_frontend->mediaQueryResultChanged();

Now that CSS domain emits events, we should make sure that it only does so when the agent is enabled. You need to introduce enable/disable methods pair as in the other domains and only generate messages while enabled. Actually, you might want to set CSS agent to the instrumentingAgents set in the ::enable.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:859
> +WebInspector.CSSDispatcher = function(cssModel)

Please jsdoc this new class.

[Alexander Pavlov (apavlov)]

Created attachment 119237 [details]
Patch

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (From update of attachment 119208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119208&action=review
> 
> > Source/WebCore/inspector/CodeGeneratorInspector.py:207
> > +    skip_js_bind_domains = set(["Runtime", "DOMDebugger"])
> 
> I think we should unskip everything. Why does it even exists?

I will file a bug for this.

> > Source/WebCore/inspector/Inspector.json:1749
> > +                "description": "Fires whenever a MediaQuery result changes (the current implementation considers only viewport-dependent media features.)"
> 
> This is not clear to me. Could you elaborate more on when the event is actually fired?

This event fires (in the suggested implementation) whenever a viewport-dependent media feature value changes. Some examples are: orientation, width, height, aspect ratio. You resize the browser or rotate your smartphone and receive the notification, because entirely different CSS rules may get applied to the inspected element, and we want to observe this live.

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:237
> > +        m_frontend->mediaQueryResultChanged();
> 
> Now that CSS domain emits events, we should make sure that it only does so when the agent is enabled. You need to introduce enable/disable methods pair as in the other domains and only generate messages while enabled. Actually, you might want to set CSS agent to the instrumentingAgents set in the ::enable.

Done.

> > Source/WebCore/inspector/front-end/CSSStyleModel.js:859
> > +WebInspector.CSSDispatcher = function(cssModel)
> 
> Please jsdoc this new class.

Done.

[Pavel Feldman]

Comment on attachment 119237 [details]
Patch

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

> Source/WebCore/inspector/InspectorCSSAgent.cpp:197
> +    , m_enabled(false)

you don't actually need to mimic the m_state->get(cssAgentEnabled) state.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:216
> +    m_instrumentingAgents->setInspectorCSSAgent(this);

Can we do it even later? Like in enable?

> Source/WebCore/page/FrameView.cpp:1005
> +            InspectorInstrumentation::didMediaQueryResultChange(document);

We only use did* notation when there is a corresponding will* signal. ::mediaQueryResultChanged would be Ok in this case.

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> (From update of attachment 119237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119237&action=review
> 
> > Source/WebCore/inspector/InspectorCSSAgent.cpp:197
> > +    , m_enabled(false)
> 
> you don't actually need to mimic the m_state->get(cssAgentEnabled) state.

Right, removed.

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:216
> > +    m_instrumentingAgents->setInspectorCSSAgent(this);
> 
> Can we do it even later? Like in enable?

Done.

> > Source/WebCore/page/FrameView.cpp:1005
> > +            InspectorInstrumentation::didMediaQueryResultChange(document);
> 
> We only use did* notation when there is a corresponding will* signal. ::mediaQueryResultChanged would be Ok in this case.

Done.

[Alexander Pavlov (apavlov)]

Committed r102903: <http://trac.webkit.org/changeset/102903>