Bug 42193

Summary: Support layoutTestController.dumpEditingDelegates in WebKitTestRunner
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: WebKit2Assignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, commit-queue, jamesr, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
step 1
none
adding more - work in progress
none
step 2 none

Description Maciej Stachowiak 2010-07-13 13:50:59 PDT
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.
Comment 1 Maciej Stachowiak 2010-07-13 13:58:36 PDT
<rdar://problem/8186753>
Comment 2 Adam Roben (:aroben) 2010-07-14 08:25:19 PDT
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.
Comment 3 Alexey Proskuryakov 2010-07-22 16:20:51 PDT
Created attachment 62356 [details]
step 1
Comment 4 Darin Adler 2010-07-22 17:11:40 PDT
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
Comment 5 Alexey Proskuryakov 2010-07-22 17:50:45 PDT
Comment on attachment 62356 [details]
step 1

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

Keeping open to add the rest of delegate methods.
Comment 6 James Robinson 2010-07-22 18:39:07 PDT
It appears this broke both the windows builders on the waterfall.
Comment 7 Alexey Proskuryakov 2010-07-22 18:57:11 PDT
Gradually fixing it (so far, r63937 and r63938).
Comment 8 Alexey Proskuryakov 2010-07-22 20:19:56 PDT
Created attachment 62375 [details]
adding more - work in progress
Comment 9 Alexey Proskuryakov 2010-07-24 22:25:21 PDT
Created attachment 62520 [details]
step 2
Comment 10 WebKit Review Bot 2010-07-24 22:27:46 PDT
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.
Comment 11 Sam Weinig 2010-07-25 00:06:58 PDT
Comment on attachment 62520 [details]
step 2

r=me. I wonder if it makes sense to break up the test runners InjectedBundlePage soon.
Comment 12 WebKit Commit Bot 2010-07-25 00:56:48 PDT
Comment on attachment 62520 [details]
step 2

Clearing flags on attachment: 62520

Committed r64020: <http://trac.webkit.org/changeset/64020>
Comment 13 WebKit Commit Bot 2010-07-25 00:56:54 PDT
All reviewed patches have been landed.  Closing bug.