Bug 92398

Created attachment 154700 [details] Patch

Comment on attachment 154700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154700&action=review Thanks for changing all of the ports! that's great. That said, R-'ing for the typo on the mac port :) Also, it might be nice if someone else who was more familiar with the DRT/WTR/C++ code took a look. Tony, what do you think? In your earlier change, I missed that the format for the line written to stdin was "test'--pixel-test'hash" ... I was hoping to extend this to support arbitrary arguments, which I suppose you can do by just splicing them between single quotes, but it seems a little odd. why not just separate the args with spaces, e.g., test hash --pixel-test --foo --bar, etc.? > Tools/DumpRenderTree/efl/DumpRenderTree.cpp:232 > +static bool getExpectedPixelHash(const String& testURL, String& pixelHashOut) I don't know the EFL implementation at all, but (perhaps naively) wouldn't it be better to parse the url in the caller to see if you needed to get the hash? Or maybe at least change the name of this function? > Tools/DumpRenderTree/mac/DumpRenderTree.mm:876 > + printSeparators = (optind < argc - 1 || (dumpPixelsForCurrentTest && dumpTree); does this compile? I think you're missing a closing paren ...

Comment on attachment 154700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154700&action=review It's very brave of you to try to change all the ports at the same time! :) It seems like the code for parsing pathOrURL and the command line arguments is the same for all DRTs (except maybe EFL). Can we move that into a helper file? Something like CyclicRedundancyCheck.h or PixelDumpSupport.h. This was probably discussed in an earlier bug, but why is ' used as a separator? Can't we just search for -- after a space? > Tools/ChangeLog:10 > + on the standart input that means that running the current typo: standart -> standard > Tools/Scripts/webkitpy/layout_tests/port/driver.py:342 > + command += "'" + '--pixel-test' Nit: Can this just be command += "'--pixel-test" ?

(In reply to comment #3) > (From update of attachment 154700 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154700&action=review > > It's very brave of you to try to change all the ports at the same time! :) > I know... :) > It seems like the code for parsing pathOrURL and the command line arguments is the same for all DRTs (except maybe EFL). Can we move that into a helper file? Something like CyclicRedundancyCheck.h or PixelDumpSupport.h. > > This was probably discussed in an earlier bug, but why is ' used as a separator? Can't we just search for -- after a space? Both are good idea. I will do that way in the next patch.

> > In your earlier change, I missed that the format for the line written to stdin was "test'--pixel-test'hash" ... I was hoping to extend this to support arbitrary arguments, which I suppose you can do by just splicing them between single quotes, but it seems a little odd. why not just separate the args with spaces, e.g., test hash --pixel-test --foo --bar, etc.? > I have tried to implement this but than realized that ' as separator is used because spaces are used to separate paths (which are simply the argv array in when running in standalone mode). I don't think it's worth to add a bunch of command line parsing logic to drt.

Created attachment 155320 [details] patch for ews's

Comment on attachment 155320 [details] patch for ews's Attachment 155320 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13396347

Comment on attachment 155320 [details] patch for ews's Attachment 155320 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13394378

Attachment 155320 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/D..." exit_code: 1 Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:34: You should add a blank line after implementation file's own header. [build/include_order] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 155320 [details] patch for ews's Attachment 155320 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13387457

(In reply to comment #5) > I have tried to implement this but than realized that ' as separator is used because spaces are used to separate paths (which are simply the argv array in when running in standalone mode). I don't think it's worth to add a bunch of command line parsing logic to drt. DRT supports running more than one test at a time using argv? News to me; do all ports support that?

Comment on attachment 155320 [details] patch for ews's Attachment 155320 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13385588

(In reply to comment #11) > (In reply to comment #5) > > I have tried to implement this but than realized that ' as separator is used because spaces are used to separate paths (which are simply the argv array in when running in standalone mode). I don't think it's worth to add a bunch of command line parsing logic to drt. > > DRT supports running more than one test at a time using argv? News to me; do all ports support that? Yes, all of them.

Created attachment 155325 [details] Patch

Comment on attachment 155325 [details] Patch Attachment 155325 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13385598

Comment on attachment 155325 [details] Patch Attachment 155325 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13381862

Comment on attachment 155325 [details] Patch Attachment 155325 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13391413

(In reply to comment #5) > > > > In your earlier change, I missed that the format for the line written to stdin was "test'--pixel-test'hash" ... I was hoping to extend this to support arbitrary arguments, which I suppose you can do by just splicing them between single quotes, but it seems a little odd. why not just separate the args with spaces, e.g., test hash --pixel-test --foo --bar, etc.? > > > > I have tried to implement this but than realized that ' as separator is used because spaces are used to separate paths (which are simply the argv array in when running in standalone mode). I don't think it's worth to add a bunch of command line parsing logic to drt. I still don't see why you need '. Any token starting with -- is a flag; everything else is a filename/URL.

> > I still don't see why you need '. Any token starting with -- is a flag; everything else is a filename/URL. I did not say it's not possible to get rid of it. I said I don't want to bother with it. I don't make it worst than it was and improving it is out of the scope of this patch. (And anyway, it's not a common use case when one want to pass -p and the hash manually.)

Created attachment 155474 [details] Patch

Comment on attachment 155474 [details] Patch Attachment 155474 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13388723

Comment on attachment 155474 [details] Patch Attachment 155474 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13384893

Comment on attachment 155474 [details] Patch Attachment 155474 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13386787

Comment on attachment 155474 [details] Patch Attachment 155474 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13387710

Comment on attachment 155474 [details] Patch Attachment 155474 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13385847

Created attachment 155489 [details] Patch

Comment on attachment 155489 [details] Patch Attachment 155489 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13400395

Comment on attachment 155489 [details] Patch Attachment 155489 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13400410

Created attachment 155513 [details] Patch

Comment on attachment 155513 [details] Patch Attachment 155513 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13390754

Created attachment 155519 [details] Patch

Comment on attachment 155519 [details] Patch Attachment 155519 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13385931

Could somebody tell me how can I include the common DumpRenderTree.h in the chromium DumpRenderTree.cpp? Include path is not set, #include "../DumpRenderTree.h" doesn't work. I don't know how to add include paths in the gpy build. Could you catch me on IRC? Thanks!

Created attachment 155569 [details] Patch

Comment on attachment 155569 [details] Patch Attachment 155569 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13410019

Created attachment 155580 [details] Patch

Comment on attachment 155580 [details] Patch Attachment 155580 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13403260

Comment on attachment 155580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155580&action=review r+ with the nit that I'd change the delimiter back to ' if we're not going to use spaces. I assume you'd have no complaints with someone uploading a separate patch to switch to spaces? Also, fix the .gyp file, please ;). Tony, are you okay with this? > Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:136 > + '../' you probably need a comma here, else I expect this is messing up the compile path: you'd get '../<(DEPTH)' :). Also, this should probably be '<(tools_dir)/DumpRenderTree' rather than a relative path. > Tools/Scripts/old-run-webkit-tests:754 > + $suffixPixelTest = ":--pixel-test"; if we're not able to use spaces, it seems like we might as well stay with single quotes as the delimiter. > Tools/Scripts/webkitpy/layout_tests/port/driver.py:339 > + command += ":" + driver_input.image_hash as above.

Comment on attachment 155580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155580&action=review >> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:136 >> + '../' > > you probably need a comma here, else I expect this is messing up the compile path: you'd get '../<(DEPTH)' :). > > Also, this should probably be '<(tools_dir)/DumpRenderTree' rather than a relative path. Heh, I suggested the relative path. Either is fine. > Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:12 > + CommandTokenizer(const std::string& input) Nit: explicit > Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:53 > + return m_next != std::string(); Nit: !m_next.empty() > Tools/WebKitTestRunner/TestController.cpp:516 > + CommandTokenizer(const std::string& input) explicit > Tools/WebKitTestRunner/TestController.cpp:557 > + return m_next != std::string(); !m_next.empty()

(In reply to comment #39) > > > > Also, this should probably be '<(tools_dir)/DumpRenderTree' rather than a relative path. > > Heh, I suggested the relative path. Either is fine. > I've found using relative paths to be fragile and confusing, especially when trying to move files around and/or sort through webkit-in-chromium vs. chromium-in-webkit checkouts. Using the variables made things a lot clearer (again, to me).

Landed in http://trac.webkit.org/changeset/124313. Keeping bug open because I forgot to remove --pixel-tests from the Qt DRT.

Need to roll out. Adding Tools/DumpRenderTree as include path does not work because there are some files with the same name in the chromium dir. Do you agree to rename these with a Chromium suffix? It would be: AccessibilityController.h/cpp -> AccessibilityControllerChromium.h/cpp AccessibilityUIElement.h/cpp -> AccessibilityUIElementChromium.h/cpp

(In reply to comment #42) > Need to roll out. Adding Tools/DumpRenderTree as include path does not work because there are some files with the same name in the chromium dir. Do you agree to rename these with a Chromium suffix? It would be: > AccessibilityController.h/cpp -> AccessibilityControllerChromium.h/cpp > AccessibilityUIElement.h/cpp -> AccessibilityUIElementChromium.h/cpp Actually we could also change the includes, like -#include "AccessibilityController.h" +#include "chromium/TestRunner/AccessibilityController.h" Which solution do you prefer? :)

> Actually we could also change the includes, like > -#include "AccessibilityController.h" > +#include "chromium/TestRunner/AccessibilityController.h" > > Which solution do you prefer? :) Using an include path like "chromium/TestRunner/AccessibilityController.h" in WebKit isn't correct.

Perhaps we should just copy DumpRenderTree.h down into the chromium dir ... it looks like that's what we've done (more or less) for other files. I don't like the duplication, but sharing more code properly is probably a bigger effort than what should be required to make this patch land.

Created attachment 156011 [details] Patch

(In reply to comment #45) > Perhaps we should just copy DumpRenderTree.h down into the chromium dir ... it looks like that's what we've done (more or less) for other files. I don't like the duplication, but sharing more code properly is probably a bigger effort than what should be required to make this patch land. Well, I don't like this idea, I think renaming those files is much more straightforward, so I did that.

Wow, finally it builds everywhere :)

Comment on attachment 156011 [details] Patch Going to land it tomorrow as it is late here and I would like to watch bots.

Landed in r124581. ELF regression is fixed in r124595.

*** Bug 91538 has been marked as a duplicate of this bug. ***

How can I manually run DRT with the path to a test and -p to dump pixels now?

(In reply to comment #52) > How can I manually run DRT with the path to a test and -p to dump pixels now? Assuming your environment is set up properly, path-to-DumpRenderTree path-to-LayoutTests/foo/bar.html'-pixel-test should work (just tested this earlier today).