Bug 49804 – Web Inspector: [REGRESSION] Contents of rules inside @media not displayed/editable

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

[Alexander Pavlov (apavlov)]

If a rule R resides inside a @media rule, R's properties are not displayed and R is not editable.

[Alexander Pavlov (apavlov)]

Created attachment 74557 [details]
[PATCH] Suggested fix

[Eric Seidel (no email)]

Attachment 74557 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6269056

[Pavel Feldman]

Comment on attachment 74557 [details]
[PATCH] Suggested fix

Please test this thoroughly.

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

> WebCore/inspector/InspectorStyleSheet.h:161
> +    bool setText(const String&, bool reparsePageStyleSheet);

Consider splitting this into two methods.

[Early Warning System Bot]

Attachment 74557 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6256092

[Eric Seidel (no email)]

Attachment 74557 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6108135

[Alexander Pavlov (apavlov)]

Created attachment 74567 [details]
[PATCH] Patch attached to test compilability, review not required

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        A       LayoutTests/inspector/resources/styles-new-API-1.css
        M       LayoutTests/inspector/resources/styles-new-API.css
        M       LayoutTests/inspector/styles-new-API-expected.txt
        M       WebCore/ChangeLog
        M       WebCore/css/CSSImportRule.cpp
        M       WebCore/inspector/InspectorCSSAgent.cpp
        M       WebCore/inspector/InspectorStyleSheet.cpp
        M       WebCore/inspector/InspectorStyleSheet.h
Committed r72540

[Alexey Proskuryakov]

This only includes an Inspector regression test, but has changes that look like they would affect Web content behavior. Can you add a test for the CSSImportRule.cpp change?

[Pavel Feldman]

Comment on attachment 74567 [details]
[PATCH] Patch attached to test compilability, review not required

Clearing r? from landed patch (it was set for bots).

[Alexander Pavlov (apavlov)]

(In reply to comment #8)
> This only includes an Inspector regression test, but has changes that look like they would affect Web content behavior. Can you add a test for the CSSImportRule.cpp change?

Alexey, I'm happy to add a test in a follow-up patch if you can give me a hint on how this test should look like. If you notice, the only CSSImportRule change is an additional NULL check that eliminates a crash whenever the parent CSSStyleSheet (i.e. the one that includes an @import statement) has no parent document - this can be easily seen from the last (non-changed) line of the related snippet in the patch. This can only occur in an artificial setting like parsing text for a hand-crafted CSSStyleSheet created with CSSStyleSheet::create() (with no arguments).

Apparently, WebKit had never encountered such kind of a behavior (otherwise the crash would not have gone unnoticed), and parsing a CSSStyleSheet created for any live node in a real document would invoke addPendingStylesheet() on that document without ever remove..()-ing it. In turn, this would subsequently prevent CSSStyleSelector::styleRulesForElement() from returning ANY matched rules for any element (that's what had been happening until this patch was committed - you can try inspecting element styles at, say, apple.com).

So, I'm not sure which aspect of the involved CSSImportRule change the test should cover. Should it just confirm that the code does not crash for such a stylesheet (already implicitly covered by the updated styles-new-API.html test) or something more solid still holds for the code in question?

[Alexey Proskuryakov]

That explains why no test is needed, thanks.