Bug 150174 – WKView being inside WKWebView leads to weird API issues

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

[Tim Horton]

Having a WKView nested inside a WKWebView causes fun but subtle troubles, most frequently regarding overriding NSResponder methods; if the WKView eats the event, the client's override on WKWebView has no effect. Also, I slightly fear clients noticing that there's a WKView in there and doing... things... to it that they otherwise shouldn't/can't.

As we all know, every problem in software can be solved by another layer of abstraction, so I propose we move all of the code backing WKView to a new class (I've called it WebViewImpl for now), forwarding externally-facing WKView API *directly* to the new class, without any logic at all in WKView. Then, for whatever externally-facing WKWebView API we wish to expose, we can also call straight into WebViewImpl, and avoid duplication of any interesting logic.

WebViewImpl will be a C++ class, which confers two interesting benefits: 1) it will make mistakes during translation obvious, because 'self' is meaningless, so we'll have to reason about each of those individually, and 2) WebViewImpl and PageClientImpl can eventually merge, which will allow a bit /less/ indirection in the case of WebPageProxy asking a question WebViewImpl knows how to answer directly.

This also provides an opportunity to take a careful stroll through the mass of code that is WKView and clean things up... yay?

[Tim Horton]

Created attachment 263166 [details]
prelim

Attaching a patch that adds WebViewImpl and moves a few things from WKView so we can discuss approaches for different things.

I don't think it's possible (or, at least, worth it) to do a partial adoption in WKWebView; that will have to be done wholesale (because we have to get rid of the inner WKView).

[Alexey Proskuryakov]

This may be an obvious comment, but we need to be super careful with notifications when moving things to C++. See bug 142945 for one example where notifications clashed between WKView and NSView; I think that we can have the same kind of problem between WKWebView and its subclasses.

[Build Bot]

Comment on attachment 263166 [details]
prelim

Attachment 263166 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/291987

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 263170 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Tim Horton]

(In reply to comment #2)
> This may be an obvious comment, but we need to be super careful with
> notifications when moving things to C++. See bug 142945 for one example
> where notifications clashed between WKView and NSView; I think that we can
> have the same kind of problem between WKWebView and its subclasses.

In this model, in the interest of moving things to WebViewImpl, *all* notifications will end up being dispatched to a different object (similar to WKWindowVisibilityObserver) that lives in WebViewImpl, and *none* will be observed by WKWebView/WKView. So I think that will leave us even better off than we are now.

[Tim Horton]

Created attachment 263354 [details]
Patch

[Tim Horton]

Created attachment 263355 [details]
Patch

[Darin Adler]

Comment on attachment 263355 [details]
Patch

Great refactoring. Not a big fan of the “word” impl. Could we find a name better than WebViewImpl?

[Tim Horton]

(In reply to comment #8)
> Comment on attachment 263355 [details]
> Patch
> 
> Great refactoring. Not a big fan of the “word” impl. Could we find a name
> better than WebViewImpl?

Anders and I haven't come up with a better name, but we both agree we don't love "impl". I'm going to continue trucking along with this name; if we come up with something better, it will be a fairly trivial find-and-replace to change it.

[Tim Horton]

Part 1: http://trac.webkit.org/changeset/191307

[Csaba Osztrogonác]

(In reply to comment #10)
> Part 1: http://trac.webkit.org/changeset/191307

It broke the API tests everywhere, see http://build.webkit.org for details.

[Tim Horton]

(In reply to comment #11)
> (In reply to comment #10)
> > Part 1: http://trac.webkit.org/changeset/191307
> 
> It broke the API tests everywhere, see http://build.webkit.org for details.

It was rolled out before this comment :D Going to see what's up.

[Tim Horton]

Let's try this again http://trac.webkit.org/changeset/191320

[Tim Horton]

Created attachment 263626 [details]
part 2 prelim

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:408:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:440:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:828:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 7 files


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

[Radar WebKit Bug Importer]

<rdar://problem/23221685>

[Tim Horton]

Created attachment 263851 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:408:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:440:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:828:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 7 files


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

[Tim Horton]

Created attachment 263856 [details]
fix mavericks build

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:408:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:440:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:828:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 7 files


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

[Tim Horton]

Created attachment 263868 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:408:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:440:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:828:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 7 files


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

[Tim Horton]

Created attachment 263875 [details]
Part 2

[Tim Horton]

Created attachment 263876 [details]
Part 3 - plugin complex text

[Tim Horton]

Created attachment 263877 [details]
Part 4 - tooltips

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:408:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:440:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:830:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 7 files


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

[Tim Horton]

Created attachment 263882 [details]
Part 5 - compositing and thumbnail view

[Anders Carlsson]

Comment on attachment 263875 [details]
Part 2

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

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.h:231
> +    CGRect m_contentPreparationRect { {0, 0}, {0, 0} };

Why not use CGRectZero?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.h:242
> +    CGSize m_resizeScrollOffset { 0, 0 };

CGSizeZero etc

[Tim Horton]

Created attachment 263932 [details]
Part 3 - plugin complex text

[Tim Horton]

Comment on attachment 263875 [details]
Part 2

Part 2 -> http://trac.webkit.org/changeset/191499

[Tim Horton]

Part 3: http://trac.webkit.org/changeset/191508

[Tim Horton]

Part 4: http://trac.webkit.org/changeset/191509

[Tim Horton]

Part 5: http://trac.webkit.org/changeset/191510

[Tim Horton]

Created attachment 264060 [details]
Part 6 - dragging

[Anders Carlsson]

Comment on attachment 264060 [details]
Part 6 - dragging

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

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1806
> +    RetainPtr<NSMutableSet> types = adoptNS([[NSMutableSet alloc] initWithArray:PasteboardTypes::forEditing()]);

auto!

[Tim Horton]

Created attachment 264084 [details]
Part 7 - gestures and snapshots

[Tim Horton]

Part 6: http://trac.webkit.org/changeset/191606

[Tim Horton]

Created attachment 264088 [details]
Part 7 - gestures and snapshots

[Tim Horton]

Created attachment 264102 [details]
Part 8 - accessibility and cleanup

[Tim Horton]

Created attachment 264104 [details]
Part 7 - gestures and snapshots

[Tim Horton]

Created attachment 264120 [details]
Part 9 - pasteboard and spellcheck

[Tim Horton]

Part 7: http://trac.webkit.org/changeset/191634

[Tim Horton]

Created attachment 264153 [details]
Part 8 - accessibility and cleanup

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1551:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1553:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 5 files


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

[Anders Carlsson]

Comment on attachment 264153 [details]
Part 8 - accessibility and cleanup

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

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.h:247
> +    void toolTipChanged(String oldToolTip, String newToolTip);

const String& ?

[Tim Horton]

Part 8: http://trac.webkit.org/changeset/191637

[Tim Horton]

Created attachment 264157 [details]
Part 9 - pasteboard and spellcheck

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1936:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1937:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1938:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1939:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1940:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1941:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:2036:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 7 in 7 files


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

[Tim Horton]

Part 9: http://trac.webkit.org/changeset/191648

[Tim Horton]

Created attachment 264242 [details]
Part 10 - WKViewInternal tidbits

[Tim Horton]

Part 10: http://trac.webkit.org/changeset/191690

[Tim Horton]

Created attachment 264263 [details]
Part 11 - Get rid of wkView() getters

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:88:  _WKRemoteObjectRegistry is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 12 files


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

[Tim Horton]

Part 11: http://trac.webkit.org/changeset/191702

[Tim Horton]

Created attachment 264289 [details]
Part 12 - UI validation and others

[Darin Adler]

Comment on attachment 264289 [details]
Part 12 - UI validation and others

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

I reviewed all the code you moved and added comments for later consideration. Not relevant to just moving the code.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.h:185
> +    // FIXME: Can't have antique API types here.

What does “can’t” mean?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:696
> +    if (DrawingAreaProxy* drawingArea = m_page.drawingArea())
> +        return drawingArea->type() == DrawingAreaTypeRemoteLayerTree;
> +    
> +    return false;

I’d write this differently:

    auto* drawingArea = m_page.drawingArea();
    return drawingArea && drawingArea->type() == DrawingAreaTypeRemoteLayerTree;

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:840
> +    BOOL expandsToFit = minimumSizeForAutoLayout.width > 0;

Should be bool, not BOOL.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1194
> +    WTF::Optional<WebCore::ScrollbarOverlayStyle> coreScrollbarStyle;
> +
> +    switch (scrollbarStyle) {
> +    case _WKOverlayScrollbarStyleDark:
> +        coreScrollbarStyle = WebCore::ScrollbarOverlayStyleDark;
> +        break;
> +    case _WKOverlayScrollbarStyleLight:
> +        coreScrollbarStyle = WebCore::ScrollbarOverlayStyleLight;
> +        break;
> +    case _WKOverlayScrollbarStyleDefault:
> +        coreScrollbarStyle = WebCore::ScrollbarOverlayStyleDefault;
> +        break;
> +    case _WKOverlayScrollbarStyleAutomatic:
> +    default:
> +        break;
> +    }
> +
> +    m_page.setOverlayScrollbarStyle(coreScrollbarStyle);

I’d like this better with a separate helper function to make the styles that can use "return".

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1199
> +    WTF::Optional<WebCore::ScrollbarOverlayStyle> coreScrollbarStyle = m_page.overlayScrollbarStyle();

Maybe use auto here?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1204
> +    if (!coreScrollbarStyle)
> +        return _WKOverlayScrollbarStyleAutomatic;
> +
> +    switch (coreScrollbarStyle.value()) {

Could use valueOr here instead of the explicit ! check. Then wouldn’t need the local variable.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1526
> +static const SelectorNameMap* createSelectorExceptionMap()

In modern WebKit code we’d have this return the map, not a heap-allocated.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1538
> +    map->add(@selector(insertNewlineIgnoringFieldEditor:), "InsertNewline");
> +    map->add(@selector(insertParagraphSeparator:), "InsertNewline");
> +    map->add(@selector(insertTabIgnoringFieldEditor:), "InsertTab");
> +    map->add(@selector(pageDown:), "MovePageDown");
> +    map->add(@selector(pageDownAndModifySelection:), "MovePageDownAndModifySelection");
> +    map->add(@selector(pageUp:), "MovePageUp");
> +    map->add(@selector(pageUpAndModifySelection:), "MovePageUpAndModifySelection");
> +    map->add(@selector(scrollPageDown:), "ScrollPageForward");
> +    map->add(@selector(scrollPageUp:), "ScrollPageBackward");

This is an anti pattern in Modern WebKit code. In new code we try to use a loop instead of a sequence of unrolled add calls, since the code is huge. Also would be more efficient if we used ASCIILiteral here.

Also, if we changed executeEditCommand to take a StringView we could just use const char* or StringVIew for the map.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1546
> +    static const SelectorNameMap* exceptionMap = createSelectorExceptionMap();

In modern WebKit code this would be a NeverDestroyed<SelectorNameMap>.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1768
> +    size_t size = items.size();
> +    for (size_t i = 0; i < size; ++i) {
> +        ValidationItem item = items[i].get();

Modern for loop?

[Tim Horton]

(In reply to comment #56)
> Comment on attachment 264289 [details]
> Part 12 - UI validation and others
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264289&action=review
> 
> I reviewed all the code you moved and added comments for later
> consideration. Not relevant to just moving the code.

I did say:

> This also provides an opportunity to take a careful stroll through the mass of code that is WKView and clean things up... yay?

So I'm really glad you did actually review the moved code!


> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.h:185
> > +    // FIXME: Can't have antique API types here.
> 
> What does “can’t” mean?

"Can't" means that because this class is going to be shared between the antique and modern API, we need to use internal types here, not API types. Not actually a problem yet, but it will be in a few days.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:696
> > +    if (DrawingAreaProxy* drawingArea = m_page.drawingArea())
> > +        return drawingArea->type() == DrawingAreaTypeRemoteLayerTree;
> > +    
> > +    return false;
> 
> I’d write this differently:
> 
>     auto* drawingArea = m_page.drawingArea();
>     return drawingArea && drawingArea->type() ==
> DrawingAreaTypeRemoteLayerTree;

Ok!

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:840
> > +    BOOL expandsToFit = minimumSizeForAutoLayout.width > 0;
> 
> Should be bool, not BOOL.

Quite.

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1194
> > +    WTF::Optional<WebCore::ScrollbarOverlayStyle> coreScrollbarStyle;
> > +
> > +    switch (scrollbarStyle) {
> > +    case _WKOverlayScrollbarStyleDark:
> > +        coreScrollbarStyle = WebCore::ScrollbarOverlayStyleDark;
> > +        break;
> > +    case _WKOverlayScrollbarStyleLight:
> > +        coreScrollbarStyle = WebCore::ScrollbarOverlayStyleLight;
> > +        break;
> > +    case _WKOverlayScrollbarStyleDefault:
> > +        coreScrollbarStyle = WebCore::ScrollbarOverlayStyleDefault;
> > +        break;
> > +    case _WKOverlayScrollbarStyleAutomatic:
> > +    default:
> > +        break;
> > +    }
> > +
> > +    m_page.setOverlayScrollbarStyle(coreScrollbarStyle);
> 
> I’d like this better with a separate helper function to make the styles that
> can use "return".

Seems reasonable, but also this is going to end up somewhere else (this is one of those "can't have API types here" functions).

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1199
> > +    WTF::Optional<WebCore::ScrollbarOverlayStyle> coreScrollbarStyle = m_page.overlayScrollbarStyle();
> 
> Maybe use auto here?
> 
> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1204
> > +    if (!coreScrollbarStyle)
> > +        return _WKOverlayScrollbarStyleAutomatic;
> > +
> > +    switch (coreScrollbarStyle.value()) {
> 
> Could use valueOr here instead of the explicit ! check. Then wouldn’t need
> the local variable.

Woah. Had not seen that before. Neat!

> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1526
> > +static const SelectorNameMap* createSelectorExceptionMap()
> 
> In modern WebKit code we’d have this return the map, not a heap-allocated.
> 
> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1538
> > +    map->add(@selector(insertNewlineIgnoringFieldEditor:), "InsertNewline");
> > +    map->add(@selector(insertParagraphSeparator:), "InsertNewline");
> > +    map->add(@selector(insertTabIgnoringFieldEditor:), "InsertTab");
> > +    map->add(@selector(pageDown:), "MovePageDown");
> > +    map->add(@selector(pageDownAndModifySelection:), "MovePageDownAndModifySelection");
> > +    map->add(@selector(pageUp:), "MovePageUp");
> > +    map->add(@selector(pageUpAndModifySelection:), "MovePageUpAndModifySelection");
> > +    map->add(@selector(scrollPageDown:), "ScrollPageForward");
> > +    map->add(@selector(scrollPageUp:), "ScrollPageBackward");
> 
> This is an anti pattern in Modern WebKit code. In new code we try to use a
> loop instead of a sequence of unrolled add calls, since the code is huge.
> Also would be more efficient if we used ASCIILiteral here.

A loop over what? Pairs in a Vector? That seems annoying. Maybe you meant something else? Or maybe that is worth it? I'll look around and see if there are other examples.

[Darin Adler]

Comment on attachment 264289 [details]
Part 12 - UI validation and others

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

>>> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1526
>>> +static const SelectorNameMap* createSelectorExceptionMap()
>> 
>> In modern WebKit code we’d have this return the map, not a heap-allocated.
> 
> A loop over what? Pairs in a Vector? That seems annoying. Maybe you meant something else? Or maybe that is worth it? I'll look around and see if there are other examples.

Typically we define a custom struct, and static const array and then loop over that:

    struct SelectorCommandName {
        SEL selector;
        ASCIILiteral commandName;
    };

    static const SelectorCommandName names[] = {
        // ...
    };

    for (auto& name : names)
        map.add(name.selector, name.commandName);

Not too annoying.

[Tim Horton]

(In reply to comment #58)
> Comment on attachment 264289 [details]
> Part 12 - UI validation and others
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264289&action=review
> 
> >>> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1526
> >>> +static const SelectorNameMap* createSelectorExceptionMap()
> >> 
> >> In modern WebKit code we’d have this return the map, not a heap-allocated.
> > 
> > A loop over what? Pairs in a Vector? That seems annoying. Maybe you meant something else? Or maybe that is worth it? I'll look around and see if there are other examples.
> 
> Typically we define a custom struct, and static const array and then loop
> over that:
> 
>     struct SelectorCommandName {
>         SEL selector;
>         ASCIILiteral commandName;
>     };
> 
>     static const SelectorCommandName names[] = {
>         // ...
>     };
> 
>     for (auto& name : names)
>         map.add(name.selector, name.commandName);
> 
> Not too annoying.

Ah! Yes, that doesn't seem too bad. Thanks!

[Tim Horton]

Comment on attachment 264289 [details]
Part 12 - UI validation and others

Part 12: http://trac.webkit.org/changeset/191757

[Tim Horton]

Created attachment 264359 [details]
Part 13 - more spellcheck

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1608:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 6 files


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

[Tim Horton]

Part 13: http://trac.webkit.org/changeset/191761

[Tim Horton]

Created attachment 264371 [details]
Part 14 - NSTextInputClient

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3161:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3336:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3378:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3831:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3890:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3906:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 5 files


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

[Tim Horton]

Created attachment 264372 [details]
Part 14 - NSTextInputClient

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3161:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3336:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3378:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3831:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3890:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3906:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 5 files


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

[Darin Adler]

Comment on attachment 264372 [details]
Part 14 - NSTextInputClient

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

Really unpleasant how repetitive the completion handler related code is. Also surprising how much of this file is logging!

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3106
> +static void extractUnderlines(NSAttributedString *string, Vector<WebCore::CompositionUnderline>& result)

Should return a Vector instead of using an out argument.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3111
> +    int i = 0;
> +    while (i < length) {

Should be a for loop. Also, isn’t there a more efficient way to iterate an attributed string?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3118
> +                color = WebCore::colorFromNSColor([colorAttr colorUsingColorSpaceName:NSDeviceRGBColorSpace]);

Almost certain this colorUsingColorSpaceName call is not helpful since colorFromNSColor already deals with this, but I can fix this in my Color patches I suppose.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3128
> +void WebViewImpl::collectKeyboardLayoutCommandsForEvent(NSEvent *event, Vector<WebCore::KeypressCommand>& commands)

This should just return a vector instead of modifying an out argument.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3149
> +        completionHandler(NO, Vector<WebCore::KeypressCommand>());

Could use { } instead of Vector<WebCore::KeypressCommand>().

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3232
> +    Vector<WebCore::KeypressCommand>* keypressCommands = m_collectedKeypressCommands;

I don’t see any value in putting this into a local variable. If we did want to, I would use auto* instead of repeating the type and put it the local variable definition inside the if statement.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3252
> +    RetainPtr<id> completionHandler = adoptNS([completionHandlerPtr copy]);

Maybe auto instead of RetainPtr<id>? Kind of strange how tricky it is to retain a block in a lambda. Maybe we should have these take std::function and let them get converted from a block to a function at the call site? Probably too risky to make that change at this time.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3273
> +    RetainPtr<id> completionHandler = adoptNS([completionHandlerPtr copy]);

Ditto.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3294
> +    RetainPtr<id> completionHandler = adoptNS([completionHandlerPtr copy]);

Ditto.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3312
> +    RetainPtr<id> completionHandler = adoptNS([completionHandlerPtr copy]);

Ditto.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3329
> +    RetainPtr<id> completionHandler = adoptNS([completionHandlerPtr copy]);

Ditto.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3365
> +void WebViewImpl::characterIndexForPoint(NSPoint thePoint, void(^completionHandlerPtr)(NSUInteger))

Ditto.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3526
> +void WebViewImpl::keyUp(NSEvent *theEvent)

Should just be "event", not "theEvent".

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3539
> +void WebViewImpl::keyDown(NSEvent *theEvent)

Should just be "event", not "theEvent".

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3566
> +void WebViewImpl::flagsChanged(NSEvent *theEvent)

Should just be "event", not "theEvent".

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3577
> +    // Don't make an event from the num lock and function keys
> +    if (!keyCode || keyCode == 10 || keyCode == 63)
> +        return;

Surprising that there is no constant for any of these three key codes. Comment mysteriously mentions two keys while the if statement covers three values!

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3624
> +    WKViewInterpretKeyEventsParameters* parameters = m_interpretKeyEventsParameters;

I’d use auto* here. Might even put this into a reference instead of a pointer after the null check, not sure. Or maybe not bother using a local variable for this at all?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3647
> +    WKViewInterpretKeyEventsParameters* parameters = m_interpretKeyEventsParameters;

I’d use auto* here. Or maybe not bother using a local variable for this at all?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3676
> +    BOOL isAttributedString = [string isKindOfClass:[NSAttributedString class]];

Should be bool instead of BOOL.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3730
> +    WKViewInterpretKeyEventsParameters* parameters = m_interpretKeyEventsParameters;

A little strange to put this into a local variable just to null check it one time below.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3786
> +    // Use pointer to get parameters passed to us by the caller of interpretKeyEvents.

Comment doesn’t seem useful.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3787
> +    WKViewInterpretKeyEventsParameters* parameters = m_interpretKeyEventsParameters;

I’d use auto* here. Or maybe not bother using a local variable for this at all?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3806
> +    // Use pointer to get parameters passed to us by the caller of interpretKeyEvents.

Comment doesn’t seem useful.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3807
> +    WKViewInterpretKeyEventsParameters* parameters = m_interpretKeyEventsParameters;

I’d use auto* here. Or maybe not bother using a local variable for this at all?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3855
> +NSAttributedString *WebViewImpl::attributedSubstringForProposedRange(NSRange nsRange, NSRangePointer actualRange)

I would name the argument proposedRange or range, not nsRange.

The NSRangePointer typedef is peculiar. Is it just the same thing as NSRange*?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3879
> +NSUInteger WebViewImpl::characterIndexForPoint(NSPoint thePoint)

Should just be point, not thePoint.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3899
> +NSRect WebViewImpl::firstRectForCharacterRange(NSRange theRange, NSRangePointer actualRange)

Should be proposedRange or range, not theRange.

The NSRangePointer typedef is peculiar. Is it just the same thing as NSRange*?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3961
> +        BOOL handledByInputMethod = interpretKeyEvent(event, commands);

Should be bool instead of BOOL.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3979
> +void WebViewImpl::keyDown(NSEvent *theEvent)

Should be event, not theEvent.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4006
> +    BOOL handledByInputMethod = interpretKeyEvent(theEvent, commands);

Should be bool instead of BOOL.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4011
> +        handledByInputMethod = NO;

Should be false instead NO.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4017
> +void WebViewImpl::flagsChanged(NSEvent *theEvent)

Should be event, not theEvent.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4033
> +    unsigned short keyCode = [theEvent keyCode];
> +
> +    // Don't make an event from the num lock and function keys
> +    if (!keyCode || keyCode == 10 || keyCode == 63)
> +        return;

Annoying to have this code repeated twice; I suggest factoring into a helper function.

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4035
> +    m_page.handleKeyboardEvent(NativeWebKeyboardEvent(theEvent, false, Vector<WebCore::KeypressCommand>()));

Consider { } instead of Vector<WebCore::KeypressCommand>().

[Tim Horton]

Comment on attachment 264372 [details]
Part 14 - NSTextInputClient

Part 14: http://trac.webkit.org/changeset/191791

[Tim Horton]

Created attachment 264405 [details]
Part 15 - printing and random bits

[Tim Horton]

Comment on attachment 264405 [details]
Part 15 - printing and random bits

Part 15: http://trac.webkit.org/changeset/191802

[Tim Horton]

Created attachment 264409 [details]
Part 16 - mouse event handling

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4178:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4186:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4201:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4209:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4226:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4238:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 6 in 4 files


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

[Tim Horton]

Comment on attachment 264409 [details]
Part 16 - mouse event handling

Part 16: http://trac.webkit.org/changeset/191806

[Tim Horton]

Created attachment 264419 [details]
Part 17 - move WebPageProxy and PageClientImpl

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/PageClientImpl.mm:181:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


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

[Tim Horton]

Created attachment 264420 [details]
Part 17 - move WebPageProxy and PageClientImpl

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/PageClientImpl.mm:181:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


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

[Build Bot]

Comment on attachment 264420 [details]
Part 17 - move WebPageProxy and PageClientImpl

Attachment 264420 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/360141

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 264427 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Tim Horton]

Created attachment 264428 [details]
Part 17 - move WebPageProxy and PageClientImpl

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/PageClientImpl.mm:185:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


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

[Tim Horton]

Comment on attachment 264428 [details]
Part 17 - move WebPageProxy and PageClientImpl

Part 17: http://trac.webkit.org/changeset/191824

[Tim Horton]

Created attachment 264604 [details]
Part 19 - remove WKView from inside of WKWebView

[Build Bot]

Comment on attachment 264604 [details]
Part 19 - remove WKView from inside of WKWebView

Attachment 264604 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/373114

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 264611 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Tim Horton]

Created attachment 264617 [details]
Part 19 - remove WKView from inside of WKWebView

[Tim Horton]

Part 19 (and the end of this bug hopefully): http://trac.webkit.org/changeset/191907