Bug 147229 – Web Inspector: Editing non-inspector-stylesheet rule selectors fails after the first change

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

* STEPS TO REPRODUCE:
1. Go to http://www.apple.com
2. Inspect the page and modify a non-inspector-stylesheet rule's selector
3. Click away from the selector editor to commit the change
4. Try to edit another rule's selector

Expected Result:
The second rule should have the newly edited selector applied.

Actual Result:
The second rule completely ignores the newly edited selector and reverts back to its original selector text on sidebar refresh.

[Radar WebKit Bug Importer]

<rdar://problem/21964842>

[Devin Rousso]

Created attachment 257375 [details]
Patch

[Brian Burg]

Comment on attachment 257375 [details]
Patch

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

The code change appears correct, but I would like the test to be improved to minimize the chances of it getting skipped.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:640
> +static bool checkStyleRuleSelector(Document* document, const String& selector)

Might as well use a better name, like isValidSelectorListString(const String&, Document*)

> Source/WebCore/inspector/InspectorStyleSheet.cpp:678
> +    if (!mutated)

Is this check necessary? In a debug build, we would have asserted. In a release build, it would clear out the inconsistent state, right?

> LayoutTests/inspector/css/modify-rule-selector.html:11
> +    var selectors = ["span.foo", ".foo"];

It would be better to have the second edited value (.foo) be distinguishable from the original value (.foo).

> LayoutTests/inspector/css/modify-rule-selector.html:29
> +                InspectorTest.log("\nPASS: selector modified");

Please restructure this into a logged state and an assertion, so the same test condition is logged in pass and fail cases. Then we can use the actual selector as reference output, rather than the logged message.

InspectorTest.log("Selector value after mutation: YYY");
InspectorTest.assert(YYY === expectedYYY, "Selector XXX should be updated to the value expectedYYY.");

> LayoutTests/inspector/css/modify-rule-selector.html:43
> +    function onStylesRefreshed()

This test has a complicated control flow structure, so it might be worth writing a little comment at the entry point describing the sequence of events. Later, we can convert this into using promises to make the control flow more obvious.

> LayoutTests/inspector/css/modify-rule-selector.html:47
> +            return

return;

> LayoutTests/inspector/css/modify-rule-selector.html:50
> +        modifyRuleSelector(selectors.splice(0, 1)[0]);

I would prefer not to mutate the list of test cases as the test executes. Most other tests maintain an index variable in this situation.

> LayoutTests/inspector/css/modify-rule-selector.html:54
> +        WebInspector.domTreeManager.querySelector(documentNode.id, ".foo", function(contentNodeId) {

No error handling for failure of requestDocument().

> LayoutTests/inspector/css/modify-rule-selector.html:62
> +            } else {

Better to lead with the exceptional arm of the branch if it's shorter. Plus, less indentation!

> LayoutTests/inspector/css/modify-rule-selector.html:72
> +    <span class="foo"></span>

Missing description of the test.

[Devin Rousso]

Created attachment 257410 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 257410 [details]
Patch

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

> Source/WebCore/inspector/InspectorStyleSheet.cpp:658
> +    bool mutated = styleSheetMutated();
> +    // If the stylesheet is already mutated at this point, that must mean that our data has been modified
> +    // elsewhere. This should never happen as ensureParsedDataReady would return false in that case.
> +    ASSERT(!mutated);

Non-debug builds are now failing to build:

    /Volumes/Data/EWS/WebKit/Source/WebCore/inspector/InspectorStyleSheet.cpp:655:10: error: unused variable 'mutated' [-Werror,-Wunused-variable]
        bool mutated = styleSheetMutated();
             ^
    1 error generated.

You can shorten this to just:

    ASSERT(!styleSheetMutated());

[Devin Rousso]

Created attachment 257428 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 257428 [details]
Patch

Clearing flags on attachment: 257428

Committed r187352: <http://trac.webkit.org/changeset/187352>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This test is super flaky. It always times out on Windows and Gtk, and it frequently times out on Yosemite WK2 (both debug and release).

I'm going to skip it on Windows and Gtk, and to mark it as a flaky timeout on Mac WK2.

[Alexey Proskuryakov]

Added expectations in r187395.