[webkit-reviews] review denied: [Bug 24300] Add CSSKeepVisitedLinksPrivate option to WebCore : [Attachment 31746] Updated patch with layout test

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied robert <robert at roberthogan.net>'s
request for review:
Bug 24300: Add CSSKeepVisitedLinksPrivate option to WebCore
https://bugs.webkit.org/show_bug.cgi?id=24300

Attachment 31746: Updated patch with layout test
https://bugs.webkit.org/attachment.cgi?id=31746&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
This is looking good!  Below are some minor style issues.  Once we address
those, we're all set.

> +function runTest() {
> +    if (window.layoutTestController) {
> +	   layoutTestController.notifyDone();
> +    }
> +}

No braces around one-line if statements.

> +    Settings* settings = m_document->settings();
> +    if (!settings || settings->cssIgnoreVisitedLinks())
> +	   m_IgnoreVisitedLinks = true;

If |settings| is null, then m_IgnoreVisitedLinks gets set to true, which is
isn't what we want (as I said in my previous review).

> @@ -262,7 +265,6 @@ QString DumpRenderTree::dumpFramesAsText(QWebFrame*
frame)
>  
>  QString DumpRenderTree::dumpBackForwardList()
>  {
> -    QString result;
>      result.append(QLatin1String("\n============== Back Forward List
==============\n"));
>      result.append(QLatin1String("FIXME: Unimplemented!\n"));
>     
result.append(QLatin1String("===============================================\n"
));

I don't understand why you removed this line.  Is this just unrelated cleanup?

>      QWebPage *m_page;
> +    HistoryManager *historyManager;

These * are misplaced.	I see that you're matching the style of this file,
however.

> +HistoryManager::HistoryManager(QObject *parent)
> +    : QWebHistoryInterface(parent)
> +    ,m_count(0)

Missing a space after the , here.

> +    m_count++;

Prefer pre-increment (e.g, ++m_count).

Will this test fail on other platforms since you only implemented this API for
QT?  Maybe you should update the skipped test lists for those platforms to
avoid turning the tree red?