Bug 36520 – [DRT/Chromium] Add LayoutTestController

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kent Tamura]

[DRT/Chromium] Add LayoutTestController

[Kent Tamura]

Created attachment 51484 [details]
Proposed patch

[Dimitri Glazkov (Google)]

Comment on attachment 51484 [details]
Proposed patch

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_userStyleSheetLocation);


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

Ditto.

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

No braces here?

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

Ditto.

[Kent Tamura]

Created attachment 51599 [details]
Proposed patch (rev.2)

[Kent Tamura]

Created attachment 51600 [details]
Proposed patch (rev.3)

[Kent Tamura]

> > +#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.

ok, I'm adding some wrapper functions to webkit_support (See http://codereview.chromium.org/1331001/show ), and remov almost all of them.

string_util.h remains.  I'll address it later.

> > +    : 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.

Done.

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

We need GURL because WebURL doesn't have Resolve().

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

We need GURL because WebURL doesn't have a constructor with a string.

> > +        m_shell->webView()->settings()->setUserStyleSheetLocation(m_userStyleSheetLocation);
> > +    GURL location(TestShell::rewriteLocalUrl(url));
> 
> Ditto.

Ditto.

> > +    } else {
> > +        result->set(false);
> > +    }
> 
> No braces here?

Done.

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

Done.

[Dimitri Glazkov (Google)]

Comment on attachment 51600 [details]
Proposed patch (rev.3)

I am ok with checking this in as-is as long as we all understand this is far from final and is going in because we want to have something running to hack on. Please spread FIXMEs liberally and start filing bugs, so that we don't forget. Nate and I did this for V8 bindings refactoring, and it worked out really well (https://bugs.webkit.org/showdependencytree.cgi?id=32630&hide_resolved=0).

We will need to get rid of direct base dependencies, and convert all std::string uses to WebString, and remove STL use.

On GURL::Resolve, we may have to poke this method through WebURL wrapper.

> +    if (!m_queue.isEmpty()) {
> +        // We delay processing queued work to avoid recursion problems.
> +        m_timer.Start(base::TimeDelta(), this, &WorkQueue::processWork);

How did base/time squeak by here?

[Dimitri Glazkov (Google)]

BTW, I see you're already doing the bug tree part very well: https://bugs.webkit.org/showdependencytree.cgi?id=28395&hide_resolved=0. Good work :)

[Kent Tamura]

(In reply to comment #6)
> (From update of attachment 51600 [details])
> I am ok with checking this in as-is as long as we all understand this is far
> from final and is going in because we want to have something running to hack
> on. Please spread FIXMEs liberally and start filing bugs, so that we don't
> forget. Nate and I did this for V8 bindings refactoring, and it worked out
> really well
> (https://bugs.webkit.org/showdependencytree.cgi?id=32630&hide_resolved=0).
> 
> We will need to get rid of direct base dependencies, and convert all
> std::string uses to WebString, and remove STL use.

Removing base/ dependencies are understandable though I have no idea how to remove Task interface for now.
Should we really remove std::string?  I thought WebString was just a bridge between WebCore and Chromium code and we should have used String/AtomicString in WebCore, string16/string in Chromium.  We need to add many functions to WebString in order to replace string with WebString.

> On GURL::Resolve, we may have to poke this method through WebURL wrapper.

Same question as above.  Should we make WebKit API classes fully-functional clases, not just wrappers?

> 
> > +    if (!m_queue.isEmpty()) {
> > +        // We delay processing queued work to avoid recursion problems.
> > +        m_timer.Start(base::TimeDelta(), this, &WorkQueue::processWork);
> 
> How did base/time squeak by here?

LayoutTestController.h still have base/timer.h.
I have added FIXME there.


I'll land this with some FIXME additions, and filed Bug#36641 for dependency cleanup.

[Kent Tamura]

Landed as r56612.

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 51600 [details] [details])
> > I am ok with checking this in as-is as long as we all understand this is far
> > from final and is going in because we want to have something running to hack
> > on. Please spread FIXMEs liberally and start filing bugs, so that we don't
> > forget. Nate and I did this for V8 bindings refactoring, and it worked out
> > really well
> > (https://bugs.webkit.org/showdependencytree.cgi?id=32630&hide_resolved=0).
> > 
> > We will need to get rid of direct base dependencies, and convert all
> > std::string uses to WebString, and remove STL use.
> 
> Removing base/ dependencies are understandable though I have no idea how to
> remove Task interface for now.
> Should we really remove std::string?  I thought WebString was just a bridge
> between WebCore and Chromium code and we should have used String/AtomicString
> in WebCore, string16/string in Chromium.  We need to add many functions to
> WebString in order to replace string with WebString.

std::string use removal is definitely a lower priority.

> > On GURL::Resolve, we may have to poke this method through WebURL wrapper.
> 
> Same question as above.  Should we make WebKit API classes fully-functional
> clases, not just wrappers?

Not fully functional, but just enough to be functional :)

> > 
> > > +    if (!m_queue.isEmpty()) {
> > > +        // We delay processing queued work to avoid recursion problems.
> > > +        m_timer.Start(base::TimeDelta(), this, &WorkQueue::processWork);
> > 
> > How did base/time squeak by here?
> 
> LayoutTestController.h still have base/timer.h.
> I have added FIXME there.
> 
> 
> I'll land this with some FIXME additions, and filed Bug#36641 for dependency
> cleanup.

Thanks!