Bug 141772 - Add new tool to test editable Webviews
Summary: Add new tool to test editable Webviews
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified OS X 10.10
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-18 15:14 PST by Enrica Casucci
Modified: 2015-02-19 10:42 PST (History)
4 users (show)

See Also:


Attachments
Patch (98.74 KB, patch)
2015-02-18 15:23 PST, Enrica Casucci
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2015-02-18 15:14:39 PST
We need a tool that can test both WK1 and WK2 views in editable mode.
Comment 1 Enrica Casucci 2015-02-18 15:23:51 PST
Created attachment 246851 [details]
Patch
Comment 2 WebKit Commit Bot 2015-02-18 15:26:08 PST
Attachment 246851 [details] did not pass style-queue:


ERROR: Tools/WebEditingTester/WK2WebDocumentController.h:31:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/WebEditingTester/AppDelegate.h:29:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/WebEditingTester/AppDelegate.m:26:  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]
ERROR: Tools/WebEditingTester/AppDelegate.m:96:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/WebEditingTester/AppDelegate.m:206:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/WebEditingTester/WK2WebDocumentController.m:26:  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]
ERROR: Tools/WebEditingTester/main.m:28:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/WebEditingTester/WK1WebDocumentController.m:26:  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]
ERROR: Tools/WebEditingTester/WK1WebDocumentController.m:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebEditingTester/WebDocumentController.m:26:  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]
ERROR: Tools/WebEditingTester/WebDocumentController.h:33:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 11 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Enrica Casucci 2015-02-18 16:03:18 PST
I've fixed the style issues.
Comment 4 Tim Horton 2015-02-18 16:46:24 PST
Comment on attachment 246851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246851&action=review

> Tools/WebEditingTester/AppDelegate.h:34
> +    IBOutlet NSPanel *operationsPanel;

Underscore?

> Tools/WebEditingTester/AppDelegate.m:68
> +    for (WebDocumentController* documents in _webDocuments)

Star's on the wrong side.

> Tools/WebEditingTester/AppDelegate.m:95
> +    static const char *operationNames[] = {

Is there any reason this isn't an NSArray of NSStrings? @[ @"alignCenter", @"...", ... ]
Also, the names are only ever used with trailing colons, so maybe they should be here that way, instead of appending them below?

Also, is it possible to detect or generate or something this list so we don't have to keep it up to date?

> Tools/WebEditingTester/AppDelegate.m:238
> +            [[button cell] setControlSize:NSMiniControlSize];

More dot notation in more places! (if you want :D)

Also, probably this project should use ARC?

> Tools/WebEditingTester/Info.plist:12
> +	<string>Apple.WebEditingTester</string>

org.webkit.WebEditingTester, maybe?

> Tools/WebEditingTester/WebDocumentController.m:39
> +    self.window.styleMask |= NSFullSizeContentViewWindowMask;

This seems like something you should be able to do in Interface Builder, no?
Comment 5 Enrica Casucci 2015-02-19 10:27:30 PST
Thanks for the review!
I've addressed your feedback.

> > Tools/WebEditingTester/AppDelegate.m:95
> > +    static const char *operationNames[] = {
> 
> Is there any reason this isn't an NSArray of NSStrings? @[ @"alignCenter",
> @"...", ... ]
> Also, the names are only ever used with trailing colons, so maybe they
> should be here that way, instead of appending them below?
> 
yes... just copied from Blot :-)
> Also, is it possible to detect or generate or something this list so we
> don't have to keep it up to date?
I'm not sure about that. Many are NSResponder methods but we have some WebKit specific ones.
Comment 6 Enrica Casucci 2015-02-19 10:42:02 PST
Committed revision 180348.