Bug 39680 – [Qt] Expose the editing behavior setting in DRT to test all editing code paths

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

[Antonio Gomes]

Expose the editing behavior setting to DRT, as discussed in https://lists.webkit.org/pipermail/webkit-dev/2010-March/012118.html

It is a follow of of bug 38603, witch missed the Qt bits.

Patch coming ...

[Antonio Gomes]

Created attachment 57034 [details]
(committed with r60189, reviewed by Ojan Vafai) patch v1

[Antonio Gomes]

Comment on attachment 57034 [details]
(committed with r60189, reviewed by Ojan Vafai) patch v1

(From update of attachment 57034 [details])
Clearing flags on attachment: 57034

Committed r60189: <http://trac.webkit.org/changeset/60189>

[Laszlo Gombos]

This patch introduced the following warning:

webkit/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function âstatic void DumpRenderTreeSupportQt::setEditingBehavior(QWebPage*, const QString&):

webkit/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:479: warning: "coreEditingBehavior" may be used uninitialized in this function

[Laszlo Gombos]

Created attachment 57403 [details]
proposed patch

[Antonio Gomes]

(In reply to comment #3)
> This patch introduced the following warning:
> 
> webkit/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function âstatic void DumpRenderTreeSupportQt::setEditingBehavior(QWebPage*, const QString&):
> 
> webkit/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:479: warning: "coreEditingBehavior" may be used uninitialized in this function

My bad. I saw that, and would patch it when I was on a clean tree, but good you came first :)


     else if (editingBehavior == "mac")
         coreEditingBehavior = EditingMacBehavior;
+    else
+        return;

Personally I would add an ASSERT_NOT_REACHED() as well. This shouldn't get never executed at all!

other than that, lgtm, thank you Laszlo.

[Eric Seidel (no email)]

Comment on attachment 57403 [details]
proposed patch

Should we log in this case?

[Eric Seidel (no email)]

Seems like programmer error to pass a non-supported value here as part of a test case.

[Eric Seidel (no email)]

Comment on attachment 57403 [details]
proposed patch

We could even ASSERT_NOT_REACHED instead of loging.  That's probably better.

Yeah this should just ASSERT_NOT_REACHED.

[Laszlo Gombos]

Created attachment 57572 [details]
patch v2 - adding ASSERT_NOT_REACHED

[Eric Seidel (no email)]

Comment on attachment 57572 [details]
patch v2 - adding ASSERT_NOT_REACHED

When I add ASSERT_NOT_REACHED I often put a comment as to why it should not be reached.  In this case:

# You've hit this ASSERT either because the layout test is using an invalid string or because Qt's DumpRenderTree is missing support for that editing behavior.

You've added an unconditional return due to missing {}

[Antonio Gomes]

Created attachment 57973 [details]
(committed with r60778, reviewed by Kenneth and Eric) patch v2.1 - adding ASSERT_NOT_REACHED 

given the long delay, I felt free to re-upload the patch adding ASSERT_NOT_REACHED() and enclosing "{...}" to the return statment.

[Antonio Gomes]

Comment on attachment 57973 [details]
(committed with r60778, reviewed by Kenneth and Eric) patch v2.1 - adding ASSERT_NOT_REACHED 

Clearing flags on attachment: 57973

Committed r60778: <http://trac.webkit.org/changeset/60778>