WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
36520
[DRT/Chromium] Add LayoutTestController
https://bugs.webkit.org/show_bug.cgi?id=36520
Summary
[DRT/Chromium] Add LayoutTestController
Kent Tamura
Reported
2010-03-24 01:16:55 PDT
[DRT/Chromium] Add LayoutTestController
Attachments
Proposed patch
(66.06 KB, patch)
2010-03-24 01:30 PDT
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Proposed patch (rev.2)
(66.06 KB, patch)
2010-03-24 23:44 PDT
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Proposed patch (rev.3)
(66.10 KB, patch)
2010-03-24 23:51 PDT
,
Kent Tamura
dglazkov
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Kent Tamura
Comment 1
2010-03-24 01:30:29 PDT
Created
attachment 51484
[details]
Proposed patch
Dimitri Glazkov (Google)
Comment 2
2010-03-24 09:18:08 PDT
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
Comment 3
2010-03-24 23:44:20 PDT
Created
attachment 51599
[details]
Proposed patch (rev.2)
Kent Tamura
Comment 4
2010-03-24 23:51:59 PDT
Created
attachment 51600
[details]
Proposed patch (rev.3)
Kent Tamura
Comment 5
2010-03-24 23:58:38 PDT
> > +#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 6
2010-03-25 14:18:44 PDT
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)
Comment 7
2010-03-25 14:19:58 PDT
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
Comment 8
2010-03-26 00:09:38 PDT
(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
Comment 9
2010-03-26 00:59:49 PDT
Landed as
r56612
.
Dimitri Glazkov (Google)
Comment 10
2010-03-26 09:24:05 PDT
(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!
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug