Bug 42193 – Support layoutTestController.dumpEditingDelegates in WebKitTestRunner

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Maciej Stachowiak]

Support layoutTestController.dumpEditingDelegates in WebKitTestRunner

Many tests (mostly editing tests) depend on this. We may want to find a way to do this without (at least initially) having actual editing delegates. Also, many of these failures likely mask deeper failures.

[Maciej Stachowiak]

<rdar://problem/8186753>

[Adam Roben (:aroben)]

A temporary option is to use the hack we use for Windows: strip all the editing delegate callbacks when doing the diff between expected and actual results. Obviously this doesn't help with generating new results, but it does make the tests runnable until we get an editing delegate hooked up.

[Alexey Proskuryakov]

Created attachment 62356 [details]
step 1

[Darin Adler]

Comment on attachment 62356 [details]
step 1

> +    bool shouldBeginEditing(WebPage* page, WebCore::Range*);

Omit the argument name "page" here?

> +WKStringRef WKBundleNodeGetNodeName(WKBundleNodeRef node)
> +{
> +    return toRef(toWK(node)->nodeName().impl());
> +}

Looks to me like this will return a stale pointer if nodeName() returns a temporary string. If nodeName() was guaranteed to always return a string that is owned by the caller, then I’d expect a return type of const String& rather than String. I think this needs to be a copy function.

> +    OwnPtr<Vector<char> > str = WKStringToUTF8(WKBundleNodeGetNodeName(node));
> +    str->shrink(str->size() - 1);

Doing this shrink seems strange. Maybe we should change WKStringToUTF8 to return a string instead of an OwnPtr<Vector<char> > in a later patch.

r=me if you change it to WKBundleNodeCopyNodeName

[Alexey Proskuryakov]

Comment on attachment 62356 [details]
step 1

Committed <http://trac.webkit.org/changeset/63930>.

Keeping open to add the rest of delegate methods.

[James Robinson]

It appears this broke both the windows builders on the waterfall.

[Alexey Proskuryakov]

Gradually fixing it (so far, r63937 and r63938).

[Alexey Proskuryakov]

Created attachment 62375 [details]
adding more - work in progress

[Alexey Proskuryakov]

Created attachment 62520 [details]
step 2

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
51:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/WebProcess/InjectedBundle/API/c/WKBundleAPICast.h:52:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/WebProcess/InjectedBundle/API/c/WKBundleAPICast.h:55:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/WebProcess/InjectedBundle/API/c/WKBundleAPICast.h:56:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/WebProcess/InjectedBundle/API/c/WKBundleAPICast.h:57:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/WebProcess/InjectedBundle/API/c/WKBundleAPICast.h:58:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/WebProcess/InjectedBundle/API/c/WKBundleAPICast.h:59:  More than one command on the same line  [whitespace/newline] [4]
WebKit2/WebProcess/InjectedBundle/API/c/WKBundleAPICast.h:60:  More than one command on the same line  [whitespace/newline] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:453:  InjectedBundlePage::_shouldEndEditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:458:  InjectedBundlePage::_shouldInsertNode is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:463:  InjectedBundlePage::_shouldInsertText is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:468:  InjectedBundlePage::_shouldDeleteRange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:473:  InjectedBundlePage::_shouldChangeSelectedRange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:478:  InjectedBundlePage::_shouldApplyStyle is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:483:  InjectedBundlePage::_didBeginEditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:488:  InjectedBundlePage::_didEndEditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:493:  InjectedBundlePage::_didChange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:498:  InjectedBundlePage::_didChangeSelection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 44 in 12 files


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

[Sam Weinig]

Comment on attachment 62520 [details]
step 2

r=me. I wonder if it makes sense to break up the test runners InjectedBundlePage soon.

[WebKit Commit Bot]

Comment on attachment 62520 [details]
step 2

Clearing flags on attachment: 62520

Committed r64020: <http://trac.webkit.org/changeset/64020>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.