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?
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).
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.
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
(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.
(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.
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.
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.
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.
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.
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.
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.
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.
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.
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?
(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.
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.
(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!
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.
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.
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.
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>().
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.
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.
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.
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
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.
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
2015-10-15 10:01 PDT, Tim Horton
2015-10-15 10:34 PDT, Build Bot
2015-10-16 17:51 PDT, Tim Horton
2015-10-16 18:31 PDT, Tim Horton
2015-10-20 15:50 PDT, Tim Horton
2015-10-22 13:19 PDT, Tim Horton
2015-10-22 13:55 PDT, Tim Horton
2015-10-22 14:52 PDT, Tim Horton
2015-10-22 16:09 PDT, Tim Horton
2015-10-22 16:10 PDT, Tim Horton
2015-10-22 16:10 PDT, Tim Horton
2015-10-22 17:52 PDT, Tim Horton
2015-10-23 11:00 PDT, Tim Horton
2015-10-26 12:15 PDT, Tim Horton
2015-10-26 15:46 PDT, Tim Horton
2015-10-26 15:54 PDT, Tim Horton
2015-10-26 17:22 PDT, Tim Horton
2015-10-26 17:27 PDT, Tim Horton
2015-10-26 22:25 PDT, Tim Horton
2015-10-27 13:11 PDT, Tim Horton
2015-10-27 13:57 PDT, Tim Horton
2015-10-28 14:11 PDT, Tim Horton
2015-10-28 16:07 PDT, Tim Horton
2015-10-28 19:28 PDT, Tim Horton
2015-10-29 16:06 PDT, Tim Horton
2015-10-29 18:36 PDT, Tim Horton
2015-10-29 19:51 PDT, Tim Horton
2015-10-30 11:43 PDT, Tim Horton
2015-10-30 12:34 PDT, Tim Horton
2015-10-30 14:36 PDT, Tim Horton
2015-10-30 14:41 PDT, Tim Horton
2015-10-30 15:22 PDT, Build Bot
2015-10-30 15:28 PDT, Tim Horton
2015-11-02 11:30 PST, Tim Horton
buildbot: commit-queue-
2015-11-02 12:06 PST, Build Bot
2015-11-02 13:09 PST, Tim Horton