[webkit-reviews] review granted: [Bug 34235] User Stylesheets do not get parsed using the correct compatibility mode in a Document : [Attachment 47579] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 34235: User Stylesheets do not get parsed using the correct compatibility
mode in a Document
https://bugs.webkit.org/show_bug.cgi?id=34235

Attachment 47579: Patch
https://bugs.webkit.org/attachment.cgi?id=47579&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   documentStyle->setHtmlHacks(true); // enable html specific rendering
tricks

If I was moving this code, I would have changed it into a sentence comment and
capitalized HTML.

> -    visuallyOrdered = false;
> +    m_visuallyOrdered = false;

Since you have to touch this line anyway, I suggest using constructor syntax
instead of an assignment statement for it.

> +    if (Settings* docSettings = settings())

In other places we do "this->settings()" so we can name the local variable
settings too. But you just pointed out to me this is another case of moved
code.

> +    bool visuallyOrdered() const { return m_visuallyOrdered; }

We try to name boolean member functions so they finish a sentence "document
<xxx>" so we would normally call this isVisuallyOrdered or
hasVisuallyOrderedEncoding or whatever. Same for RenderStyle, etc. Not saying
you need to change this now.

The code looks good. You pointed out the regression test does not work. review+
assuming you make a new version of the test that does work.