[webkit-reviews] review granted: [Bug 35014] Modifying UA rules from page JS crashes : [Attachment 55818] Fix Bug 35014 - Modifying UA rules from page JS crashes

Wed May 12 11:54:54 PDT 2010

Darin Adler <darin at apple.com> has granted Yuzo Fujishima <yuzo at google.com>'s
request for review:
Bug 35014: Modifying UA rules from page JS crashes
https://bugs.webkit.org/show_bug.cgi?id=35014

Attachment 55818: Fix Bug 35014 - Modifying UA rules from page JS crashes
https://bugs.webkit.org/attachment.cgi?id=55818&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +<!doctype html>
> +<html>
> +<head>
> +<title>Test for Bug 35014 - Modifying UA rules from page JS crashes</title>
> +</head>
> +<body>
> +<script>
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();
> +
> +window.getMatchedCSSRules(document.body, "",
false)[0].style.marginTop="200px";
> +</script>
> +PASS
> +</body>
> +</html>

I don't like that this test claims to PASS even if it hasn't even run.
Typically the test would replace a "test has not run" message with a "test
passed if we do not see a crash" message *after* running the line of code in
question.

> -    if (root->isCSSStyleSheet())
> -	   static_cast<CSSStyleSheet*>(root)->doc()->updateStyleSelector();
> +    if (root->isCSSStyleSheet()) {
> +	   Document* doc = static_cast<CSSStyleSheet*>(root)->doc();
> +	   if (doc)
> +	       doc->updateStyleSelector();
> +    }

It's great to add a null check. More idiomatic for WebKit would be:

    if (root->isCSSStyleSheet()) {
	if (Document* document = static_cast<CSSStyleSheet*>(root)->doc());
	    document->updateStyleSelector();
    }

r=me, but this could be improved a bit