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.
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.
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.
Created attachment 263354 [details] Patch
Created attachment 263355 [details] Patch
Comment on attachment 263355 [details] Patch Great refactoring. Not a big fan of the “word” impl. Could we find a name better than WebViewImpl?
(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.
Part 1: http://trac.webkit.org/changeset/191307
(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.
(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.
Let's try this again http://trac.webkit.org/changeset/191320
Created attachment 263626 [details] part 2 prelim
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.
<rdar://problem/23221685>
Created attachment 263851 [details] Patch
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.
Created attachment 263856 [details] fix mavericks build
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.
Created attachment 263868 [details] Patch
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.
Created attachment 263875 [details] Part 2
Created attachment 263876 [details] Part 3 - plugin complex text
Created attachment 263877 [details] Part 4 - tooltips
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.
Created attachment 263882 [details] Part 5 - compositing and thumbnail view
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
Created attachment 263932 [details] Part 3 - plugin complex text
Comment on attachment 263875 [details] Part 2 Part 2 -> http://trac.webkit.org/changeset/191499
Part 3: http://trac.webkit.org/changeset/191508
Part 4: http://trac.webkit.org/changeset/191509
Part 5: http://trac.webkit.org/changeset/191510
Created attachment 264060 [details] Part 6 - dragging
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!
Created attachment 264084 [details] Part 7 - gestures and snapshots
Part 6: http://trac.webkit.org/changeset/191606
Created attachment 264088 [details] Part 7 - gestures and snapshots
Created attachment 264102 [details] Part 8 - accessibility and cleanup
Created attachment 264104 [details] Part 7 - gestures and snapshots
Created attachment 264120 [details] Part 9 - pasteboard and spellcheck
Part 7: http://trac.webkit.org/changeset/191634
Created attachment 264153 [details] Part 8 - accessibility and cleanup
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.
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& ?
Part 8: http://trac.webkit.org/changeset/191637
Created attachment 264157 [details] Part 9 - pasteboard and spellcheck
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.
Part 9: http://trac.webkit.org/changeset/191648
Created attachment 264242 [details] Part 10 - WKViewInternal tidbits
Part 10: http://trac.webkit.org/changeset/191690
Created attachment 264263 [details] Part 11 - Get rid of wkView() getters
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.
Part 11: http://trac.webkit.org/changeset/191702
Created attachment 264289 [details] Part 12 - UI validation and others
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!
Comment on attachment 264289 [details] Part 12 - UI validation and others Part 12: http://trac.webkit.org/changeset/191757
Created attachment 264359 [details] Part 13 - more spellcheck
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.
Part 13: http://trac.webkit.org/changeset/191761
Created attachment 264371 [details] Part 14 - NSTextInputClient
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.
Created attachment 264372 [details] Part 14 - NSTextInputClient
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>().
Comment on attachment 264372 [details] Part 14 - NSTextInputClient Part 14: http://trac.webkit.org/changeset/191791
Created attachment 264405 [details] Part 15 - printing and random bits
Comment on attachment 264405 [details] Part 15 - printing and random bits Part 15: http://trac.webkit.org/changeset/191802
Created attachment 264409 [details] Part 16 - mouse event handling
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.
Comment on attachment 264409 [details] Part 16 - mouse event handling Part 16: http://trac.webkit.org/changeset/191806
Created attachment 264419 [details] Part 17 - move WebPageProxy and PageClientImpl
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.
Created attachment 264420 [details] Part 17 - move WebPageProxy and PageClientImpl
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.
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.
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
Created attachment 264428 [details] Part 17 - move WebPageProxy and PageClientImpl
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.
Comment on attachment 264428 [details] Part 17 - move WebPageProxy and PageClientImpl Part 17: http://trac.webkit.org/changeset/191824
Created attachment 264604 [details] Part 19 - remove WKView from inside of WKWebView
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.
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
Created attachment 264617 [details] Part 19 - remove WKView from inside of WKWebView
Part 19 (and the end of this bug hopefully): http://trac.webkit.org/changeset/191907