Bug 35844 – [Qt] Add support for LayoutTestController commands.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Robert Hogan]

Add support for LayoutTestController commands:
          setSmartInsertDeleteEnabled
          setSelectTrailingWhitespaceEnabled
          execCommand
          isCommandEnabled

        Unskip tests:
        editing/deleting/smart-editing-disabled.html
        editing/execCommand/19089.html
        editing/execCommand/delete-image-in-anchor.html
        editing/execCommand/enabling-and-selection-2.html
        editing/selection/doubleclick-whitespace-crash.html
        editing/selection/doubleclick-whitespace-img-crash.html
        editing/selection/doubleclick-whitespace.html
        editing/selection/select-line.html

        Add Qt-specific results for tests:
        editing/deleting/5300379.html
        editing/deleting/smart-delete-003.html
        editing/deleting/smart-delete-004.html
        editing/selection/5195166-1.html
        editing/selection/5195166-2.html

(These last are render tree tests and the only differences are minor pixel/width differences)

[Robert Hogan]

Created attachment 50171 [details]
Patch

My local results for:

editing/deleting/5300379.html
editing/deleting/smart-delete-003.html
editing/deleting/smart-delete-004.html
editing/selection/5195166-1.html
editing/selection/5195166-2.html

are slightly different to those in LayoutTests/platform/qt/*expected.txt. But presumably the buildbot has the right fonts installed and I don't, so I haven't updated them.

[Holger Freyther]

The patch is fine, for the API it might be better to have an enum for each command and then a generic isCommandEnabled, setCommandEnabled function?

[Robert Hogan]

(In reply to comment #2)
> The patch is fine, for the API it might be better to have an enum for each
> command and then a generic isCommandEnabled, setCommandEnabled function?

There isn't a setCommandEnabled function required by the LayoutTestController. Also, isCommandEnabled is only ever called by the DRT so not sure of value of enumerating every valid command since those are already enumerated by WebCore.

Have a feeling I've missed your point though.

[Holger Freyther]

(In reply to comment #3)
> (In reply to comment #2)
> > The patch is fine, for the API it might be better to have an enum for each
> > command and then a generic isCommandEnabled, setCommandEnabled function?
> 
> There isn't a setCommandEnabled function required by the LayoutTestController.
> Also, isCommandEnabled is only ever called by the DRT so not sure of value of
> enumerating every valid command since those are already enumerated by WebCore.
> 
> Have a feeling I've missed your point though.

The point is. You will be adding ~10-15 public API methods (setters and getters) that have little usage outside of the DRT. This raises my concern that this might be the wrong approach.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords

[Robert Hogan]

> 
> The point is. You will be adding ~10-15 public API methods (setters and
> getters) that have little usage outside of the DRT. This raises my concern that
> this might be the wrong approach.

Think we're talking at cross-purposes here. This is the isCommandEnabled API the patch adds to the DRT:

+bool LayoutTestController::isCommandEnabled(const QString& name) const
+{
+    return qt_drt_isCommandEnabled(m_drt->webPage(), name);
+}
+

+bool QWEBKIT_EXPORT qt_drt_isCommandEnabled(QWebPage* page, const QString& name)
+{
+    return page->handle()->page->focusController()->focusedOrMainFrame()->editor()->command(name).isEnabled();
+}
+

No need to add any enums surely? (Apologies if I'm completely missing your point).

[Simon Hausmann]

Comment on attachment 50171 [details]
Patch

Looks good in general, yay for passing more tests.

The part I'm unhappy about are the functions added to QWebPage:

* We usually don't have toggle() functions for plain boolean properties.
* The added properties are lacking documentation!
* I'd like to see the names discussed/proposed on the mailing list first, for a wider review.
* The selectTrailingWhitespaceEnabled property is lacking a Q_PROPERTY declaration

An alternative to the public API would and a cleanup of all the _drt_ functions would be to collect all those ugly qt_drt_foo functions, put them into WebCoreSupport/DumpRenderTreeSupport.cpp/h, _export_ the entire class and access it from the DRT by including the private header file.

[Robert Hogan]

(In reply to comment #7)
> 
> * We usually don't have toggle() functions for plain boolean properties.
> * The added properties are lacking documentation!
> * I'd like to see the names discussed/proposed on the mailing list first, for a
> wider review.
> * The selectTrailingWhitespaceEnabled property is lacking a Q_PROPERTY
> declaration
> 
I was wrong to add these as API, they are only used by EditorClientQt. I've changed it so that they are private values accessible by EditorClient and moved the set*() API into DRTSupport.

> An alternative to the public API would and a cleanup of all the _drt_ functions
> would be to collect all those ugly qt_drt_foo functions, put them into
> WebCoreSupport/DumpRenderTreeSupport.cpp/h, _export_ the entire class and
> access it from the DRT by including the private header file.

I went ahead and did this, though I suspect it will have to be a separate patch as the patch itself now in excess of 1K lines.

I've briefly tested it and it seems to be working fine, though I'd appreciate comments on the following before splitting/finalizing it for commit and finishing testing:

- How does use of the value of QWebPagePrivate::drtRun work exactly? Even at the moment it's referenced in FrameLoaderClientQt.cpp and I don't see how that class knows which instance of qwebpage it's referencing (or why it doesn't matter). I know it's a static value for the class, but does that mean once it's set it's true for all instances? Bit of a C++ 101 question but there you are! (My lack of clarity here means I'm not sure if I'm setting the value correctly in my patch.)

[Robert Hogan]

Created attachment 50896 [details]
Updated Patch - with QDRTSupport

[Robert Hogan]

Created attachment 50960 [details]
Updated Patch - with QDRTSupport

[WebKit Review Bot]

Attachment 50960 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:261:  QDRTSupport::garbageCollector_collect is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:266:  QDRTSupport::garbageCollector_collectOnAlternateThread is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebpage/tst_qwebpage.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsecurityorigin.cpp"
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:20:  #ifndef header guard has wrong style, please use: DumpRenderTreeSupport_h  [build/header_guard] [5]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:40:  dump_editing_callbacks is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:41:  dump_frame_loader is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:42:  dump_resource_load_callbacks is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:51:  webpage_groupName is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:52:  webpage_setGroupName is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:53:  dump_set_accepts_editing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:59:  garbageCollector_collect is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:60:  garbageCollector_collectOnAlternateThread is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:66:  webinspector_executeScript is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:67:  webinspector_show is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:68:  webinspector_close is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.h"
Total errors found: 30 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

We need to get this in quickly before the patch gets stale and hard to maintain.

[Simon Hausmann]

Comment on attachment 50960 [details]
Updated Patch - with QDRTSupport


> diff --git a/WebKit/qt/ChangeLog b/WebKit/qt/ChangeLog
> index 546ac34..e73082a 100644
> --- a/WebKit/qt/ChangeLog
> +++ b/WebKit/qt/ChangeLog
> @@ -1,3 +1,49 @@
> [phone]  Robert Hogan  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Add support for LayoutTestController commands.
> +
> +        Add support for LayoutTestController commands:
> +          setSmartInsertDeleteEnabled
> +          setSelectTrailingWhitespaceEnabled
> +          execCommand
> +          isCommandEnabled
> +
> +        Unskip tests:
> +        editing/deleting/smart-editing-disabled.html
> +        editing/execCommand/19089.html
> +        editing/execCommand/delete-image-in-anchor.html
> +        editing/execCommand/enabling-and-selection-2.html
> +        editing/selection/doubleclick-whitespace-crash.html
> +        editing/selection/doubleclick-whitespace-img-crash.html
> +        editing/selection/doubleclick-whitespace.html
> +        editing/selection/select-line.html
> +        editing/deleting/5300379.html
> +        editing/deleting/smart-delete-003.html
> +        editing/deleting/smart-delete-004.html
> +        editing/selection/5195166-1.html
> +        editing/selection/5195166-2.html
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=35844
> +
> +        * Api/qwebpage.cpp:
> +        (qt_drt_executeCoreCommandByName):
> +        (qt_drt_isCommandEnabled):
> +        (QWebPagePrivate::QWebPagePrivate):
> +        (QWebPage::setSmartInsertDeleteEnabled):
> +        (QWebPage::smartInsertDeleteEnabled):
> +        (QWebPage::toggleSmartInsertDeleteEnabled):
> +        (QWebPage::setSelectTrailingWhitespaceEnabled):
> +        (QWebPage::selectTrailingWhitespaceEnabled):
> +        * Api/qwebpage.h:
> +        * Api/qwebpage_p.h:
> +        * WebCoreSupport/EditorClientQt.cpp:
> +        (WebCore::EditorClientQt::smartInsertDeleteEnabled):
> +        (WebCore::EditorClientQt::toggleSmartInsertDelete):
> +        (WebCore::EditorClientQt::isSelectTrailingWhitespaceEnabled):
> +        * WebCoreSupport/EditorClientQt.h:
> +

It looks like the ChangeLog is missing the qt_drt_* refactoring.

>  2010-03-17  Csaba Osztrogonác  <[e-mail]>
>  
>          Reviewed by Kenneth Rohde Christiansen.
> diff --git a/WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp b/WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp
> new file mode 100644
> index 0000000..ccf13a3
> --- /dev/null
> +++ b/WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp
> @@ -0,0 +1,357 @@
> +/*
> + * Copyright (C) 2007 Apple Inc.  All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> + */

This doesn't look like the right copyright / license. The code comes mostly from qweb* classes and you should add your name, too, I think.


Otherwise this looks great!

[Simon Hausmann]

Tor Arne, what do you think about the qt_drt_* refactoring?

[Robert Hogan]

Created attachment 51181 [details]
Updated Patch

Updated per comments (and rebased). Still unsure about QWebPagePrivate::drtRun. I think it would be best if Ossy could confirm he has a clean test run on this before landing, running the layout tests on my machine, even with a clean head, is a total bust.

[WebKit Review Bot]

Attachment 51181 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:23:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:31:  "Frame.h" already included at WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:28  [build/include] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebpage/tst_qwebpage.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsecurityorigin.cpp"
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.h:23:  #ifndef header guard has wrong style, please use: DumpRenderTreeSupport_h  [build/header_guard] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.h"
Total errors found: 12 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Hogan]

Created attachment 51184 [details]
Updated Patch per Stylebot

[WebKit Review Bot]

Attachment 51184 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/WebCoreSupport/DumpRenderTreeSupport.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebpage/tst_qwebpage.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsecurityorigin.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.h"
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Csaba Osztrogonác]

I tested with additional header, #include "SVGSMILElement.h",
but unfortunately I got 7 failing tests. I will submit the diff here.

Please add this include, and follow advices of style bot. :)
If you run WebKitTools/Scripts/check-webkit-style locally before 
upload the patch, you can spare with resources of style bot.

[Robert Hogan]

Created attachment 51248 [details]
Patch

Ossy, I'm pretty sure I've fixed any failures here. You might get layout differences but these should be pixel-widths only.

[Robert Hogan]

You might notice in the change to expected results for smart-delete-004 that the expected platform-specific result for qt has been changed from 'foo  baz' to 'foo baz'. The latter is the expected result for mac.

[Csaba Osztrogonác]

Created attachment 51254 [details]
patch (for your patch) to make buildbot happy

(In reply to comment #20)
> Created an attachment (id=51248) [details]
> Patch
> 
> Ossy, I'm pretty sure I've fixed any failures here. You might get layout
> differences but these should be pixel-widths only.

I tested the last patch, the differences are only pixel 
differences, I attached the diff to make our buildbot happy.

[Kenneth Rohde Christiansen]

I'm not such a big fan of the refactoring:

1) before the webpage related methods were in qwebpage.cpp and that made sense, as they sometime turn into real methods.
2) now some of the methods are prefixed with webPage and others related to QWebPage are not, very inconsistent.
3) would it make more sense to add these methods to the *Private classes? such as QWebPagePrivate? or create a QWebPageDRT? I opt for the first one; we could even prefix all methods with DRT if needed, like QWebFramePrivate::drtClearName(qwebframe);

[Simon Hausmann]

(In reply to comment #23)
> I'm not such a big fan of the refactoring:
> 
> 1) before the webpage related methods were in qwebpage.cpp and that made sense,
> as they sometime turn into real methods.

You have to admit though that on the other hand speading all those drt methods throughout the API is a big mess.

> 2) now some of the methods are prefixed with webPage and others related to
> QWebPage are not, very inconsistent.

That sounds fixable (for a follow-up patch).

> 3) would it make more sense to add these methods to the *Private classes? such
> as QWebPagePrivate? or create a QWebPageDRT? I opt for the first one; we could
> even prefix all methods with DRT if needed, like
> QWebFramePrivate::drtClearName(qwebframe);

What's the advantage of grouping them in QWebPagePrivate if some of them operate on frames only and others are global functions? It seems just as unrelated as having a dedicated DRT class, that at least makes it clear that this is private DRT API, nothing else.

[Kenneth Rohde Christiansen]

(In reply to comment #24)
> (In reply to comment #23)
> > I'm not such a big fan of the refactoring:
> > 
> > 1) before the webpage related methods were in qwebpage.cpp and that made sense,
> > as they sometime turn into real methods.
> 
> You have to admit though that on the other hand speading all those drt methods
> throughout the API is a big mess.

True to some degree, but they are related to the others methods where it is being implemented. Just like we implement QWebPagePrivate methods inside qwebpage.cpp; so the methods are grouped with related methods.

> What's the advantage of grouping them in QWebPagePrivate if some of them
> operate on frames only and others are global functions? 

The global ones we could just implement directly in the DumpRenderTree.cpp right?

> It seems just as
> unrelated as having a dedicated DRT class, that at least makes it clear that
> this is private DRT API, nothing else.

That is why I suggested creating QWebPageDRT and QWebFrameDRT, though that might be overkill.

Anyway, the patch instantiates a QDRTSupport class, even though all methods basically act like static methods.

[Kenneth Rohde Christiansen]

If we do a refactoring we should discuss it, and do it part per part. I'm sure a lot of this code can actually be cleaned up.

Anyway, the reason we are doing these classes are that DRT doesn't depend on WebCore; it is basically a workaround to the point that the DRT should only use our public API, right?

I think the refactoring should be separate from this patch, don't you agree?

[Robert Hogan]

(In reply to comment #26)
> Anyway, the reason we are doing these classes are that DRT doesn't depend on
> WebCore; it is basically a workaround to the point that the DRT should only use
> our public API, right?
> 

There's no reason why most of it couldn't be public undocumented API. If there is a reason for keeping it private then this refactoring or something similar seems to be required long term.

So is there a good reason for keeping it private?

(In reply to comment #25)
>Anyway, the patch instantiates a QDRTSupport class, even though all methods
>basically act like static methods.

Yes, I should probably just make all the methods static and skip the instantiation. No good reason I didn't do that in the first place.

[Robert Hogan]

Created attachment 51363 [details]
Updated Patch

Use static members from QDRTSupport rather than instantiating the class.

[Simon Hausmann]

(In reply to comment #26)
> I think the refactoring should be separate from this patch, don't you agree?

That's true, I agree with that. Robert, could you separate it? Just leave out the smart insertion.

Please send me a private email as a reminder, so that I can be a bit quicker in the review of the refactoring (sorry :-(

[Robert Hogan]

Created attachment 52416 [details]
Refactor DRT Support

As per Kenneth and Simon's request - patch with DRT support refactoring only.

[Robert Hogan]

Created attachment 52417 [details]
Removed stray changelog entry

Oops, stray changelog entry.

[Kenneth Rohde Christiansen]

Comment on attachment 52417 [details]
Removed stray changelog entry

As it is an internal class we should call it QtDRTSupport (or QtDrtSupport to following Qt naming conventions), as we do with most other internal Qt classes (not privates) in WebKit.

We still need to look at the method names. For instance it is not obvious what QtDRTSupport::run() does. Also, I would like you to group the methods logically together in the header. Also, you have methods in the header with signature (QWebFrame* qFrame, ...), please leave our the qFrame as the style guide dictates.

[Robert Hogan]

Created attachment 53050 [details]
Updated Patch

I've renamed it DumpRenderTreeSupportQt to match the filename. I've also rejigged one or two of the function names and reordered the functions in the header.

[WebKit Commit Bot]

Comment on attachment 53050 [details]
Updated Patch

Clearing flags on attachment: 53050

Committed r57433: <http://trac.webkit.org/changeset/57433>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Buildfix for --debug build landed: http://trac.webkit.org/changeset/57447

[Csaba Osztrogonác]

(In reply to comment #36)
> Buildfix for --debug build landed: http://trac.webkit.org/changeset/57447

Sorry, I mean http://trac.webkit.org/changeset/57448 :)

[Robert Hogan]

Re-opening to add layouttestcontroller commands.

[Robert Hogan]

Created attachment 53084 [details]
LayoutTestController Commands Patch

[Luiz Agostini]

(In reply to comment #33)
> Created an attachment (id=53050) [details]
> Updated Patch
> 
> I've renamed it DumpRenderTreeSupportQt to match the filename. I've also
> rejigged one or two of the function names and reordered the functions in the
> header.

patch 53050 that is now landed removed static function qt_wrt_setViewMode from qwebpage. It makes no sense to me since the only relation it has with DRT is the fact that there is a LayoutTest for viewModes.

Now the hidden api used to change view modes is a static method of class DumpRenderTreeSupportQt. Was it actually the intention of the patch?

[Robert Hogan]

(In reply to comment #40)
> 
> Now the hidden api used to change view modes is a static method of class
> DumpRenderTreeSupportQt. Was it actually the intention of the patch?

It sounds like it was inappropriate to move it and that it should be moved back.

[Luiz Agostini]

(In reply to comment #41)
> (In reply to comment #40)
> > 
> > Now the hidden api used to change view modes is a static method of class
> > DumpRenderTreeSupportQt. Was it actually the intention of the patch?
> 
> It sounds like it was inappropriate to move it and that it should be moved
> back.

bug 37505 moves it back.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57793 might have broken Qt Linux Release

[Csaba Osztrogonác]

(In reply to comment #43)
> http://trac.webkit.org/changeset/57793 might have broken Qt Linux Release

Fixed by http://trac.webkit.org/changeset/57794 , thx Robert.