[webkit-reviews] review denied: [Bug 36520] [DRT/Chromium] Add LayoutTestController : [Attachment 51484] Proposed patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied TAMURA, Kent
<tkent at chromium.org>'s request for review:
Bug 36520: [DRT/Chromium] Add LayoutTestController
https://bugs.webkit.org/show_bug.cgi?id=36520

Attachment 51484: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=51484&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
Thanks for working on this. I know it's a slow process, but it's a very
important one.

> +#include "WebViewHost.h"
> +#include "base/basictypes.h"
> +#include "base/debug_util.h"
> +#include "base/file_path.h"
> +#include "base/message_loop.h"
> +#include "base/path_service.h"
> +#include "base/string_util.h"

Ouch, that's a lot of base dependencies. We should work to eliminate them.
With these upstream, changing base code will be very difficult -- think of all
the three-sided patches it will take.
Let's convert them to WebKit API uses or build up webkit_support library.

> +
> +LayoutTestController::LayoutTestController(TestShell* shell)
> +    : ALLOW_THIS_IN_INITIALIZER_LIST(m_timeoutFactory(this))
> +    , m_shell(shell)
> +    , ALLOW_THIS_IN_INITIALIZER_LIST(m_workQueue(this))

We can just drop ALLOW_THIS_IN_INITIALIZER_LIST in WebKit.

> +{
> +    if (arguments.size() > 0 && arguments[0].isString()) {
> +	   GURL currentURL = m_shell->webView()->mainFrame()->url();
> +	   GURL fullURL = currentURL.Resolve(arguments[0].toString());

WebURL?

> +	   m_userStyleSheetLocation =
GURL(TestShell::rewriteLocalUrl(arguments[0].toString()));

Ditto.

> +	  
m_shell->webView()->settings()->setUserStyleSheetLocation(m_userStyleSheetLocat
ion);


> +    GURL location(TestShell::rewriteLocalUrl(url));

Ditto.

> +    } else {
> +	   result->set(false);
> +    }

No braces here?

> +	   result->set(pauseTransitionAtTimeOnElementWithId(propertyName, time,
elementId));
> +    } else {
> +	   result->set(false);
> +    }

Ditto.