[webkit-reviews] review granted: [Bug 124803] Merge WebKit2 iOS specific code : [Attachment 217729] Patch2

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 124803: Merge WebKit2 iOS specific code
https://bugs.webkit.org/show_bug.cgi?id=124803

Attachment 217729: Patch2
https://bugs.webkit.org/attachment.cgi?id=217729&action=review

------- Additional Comments from Tim Horton <thorton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217729&action=review


r+ from me and andersca with comments that don't necessarily need to all be
addressed in this patch but should be addressed eventually.

> ChangeLog:8
> +	   * UIProcess/API/C/WKNativeEvent.h:

Please remove this long list and just note that you did so.

> UIProcess/API/C/WKPage.cpp:894
>  // -- DEPRECATED --

WKPageSetInvalidMessageFunction shouldn't be in this NPAPI block.

> UIProcess/API/ios/PageClientImplIOS.h:36
> +    class PageClientImpl : public PageClient {

None of this should be indented.

> UIProcess/API/ios/UIWKRemoteView.mm:1
> +/*

We should get rid of this file, we won't need it.

> UIProcess/API/ios/WKContentView.h:50
> +NS_CLASS_AVAILABLE_IOS(8_0)

This should use WK_API_CLASS, not this.

> UIProcess/API/ios/WKContentView.mm:267
> +	   [_delegate contentView:self
> +didChangeViewportArgumentsSize:CGSizeMake(arguments.width, arguments.height)

> +		     initialScale:arguments.zoom
> +		     minimumScale:arguments.minZoom
> +		     maximumScale:arguments.maxZoom
> +		allowsUserScaling:arguments.userZoom];

This should be on a single line

> UIProcess/API/ios/WKInteractionView.mm:71
> + at property (copy,nonatomic) NSArray *selectionRects;

space after the comma

> UIProcess/API/ios/WKInteractionView.mm:182
> +- (void)setScrollView:(UIWebScrollView*)scrollView

space before the star

> UIProcess/API/ios/WKInteractionView.mm:350
> +	   case UIGestureRecognizerStateBegan:

unindent cases

> UIProcess/API/ios/WKInteractionView.mm:493
> +    if (action == @selector(_promptForReplace:))
> +	   // FIXME: need to implement
> +	   return NO;
> +
> +    if (action == @selector(select:))
> +	   // Disable select in password fields so that you can't see word
boundaries.
> +	   return !_page->editorState().isInPasswordField && [self hasContent]
&& !_page->editorState().selectionIsNone &&
!_page->editorState().selectionIsRange;

Curly braces, please, even though they're not required.

> UIProcess/API/ios/WKInteractionView.mm:761
> +    WKInteractionView* view = static_cast<WKInteractionView*>(context);

stars are on the wrong side

> UIProcess/API/ios/WKInteractionView.mm:767
> +static void selectionChangedWithTouch(const WebCore::IntPoint& point,
uint32_t touch, WKErrorRef error, void* context)

We usually 'using namespace WebCore' in implementation files to avoid all of
these WebCore::s

> UIProcess/API/ios/WKInteractionView.mm:773
> +    WKInteractionView* view = static_cast<WKInteractionView*>(context);

stars on the wrong side

> UIProcess/API/ios/WKInteractionView.mm:1331
> +    return (id)[UIFont fontWithFamilyName:_autocorrectionData.fontName
traits:(UIFontTrait)_autocorrectionData.fontTraits size:scaledSize];

Why is there an (id) here? This method returns a UIFont.

> UIProcess/API/ios/WKView.mm:118
> +- (void)contentViewdidCommitLoadForMainFrame:(WKContentView *)contentView

I thought the capitalization of this method was fixed recently?

> UIProcess/API/mac/WKBrowsingContextController.mm:85
> +    id<WKBrowsingContextLoadDelegateInternal> _loadDelegateInternal;

space after id, says andersca

> UIProcess/API/mac/WKBrowsingContextController.mm:86
> +#endif //PLATFORM(IOS)

space after //

> UIProcess/API/mac/WKBrowsingContextController.mm:506
> +#endif //PLATFORM(IOS)

space after //

> UIProcess/API/mac/WKProcessGroup.mm:238
> +- (WKGeolocationProviderIOS *) _geolocationProvider

no space between ) and _

> UIProcess/API/mac/WKProcessGroupPrivate.h:29
> +#if PLATFORM(IOS)

This can't be in API/SPI headers.

> UIProcess/API/mac/WKProcessGroupPrivate.h:36
> +#if PLATFORM(IOS)

Nor this.

> UIProcess/PageClient.h:48
> +OBJC_CLASS UIWKView;

I think we don't need this.

> UIProcess/WebPageProxy.h:1191
> +    HashMap<uint64_t, RefPtr<TouchesCallback> > m_touchesCallbacks;
> +    HashMap<uint64_t, RefPtr<AutocorrectionDataCallback> >
m_autocorrectionCallbacks;

no space between >>

> UIProcess/ios/TiledCoreAnimationDrawingAreaProxyIOS.h:1
> +/*

We should get rid of TCADA*IOS

> UIProcess/ios/WebPageProxyIOS.mm:50
> +    // Just return the iOS 5.1 user agent for now.
> +    return "Mozilla/5.0 (iPad; CPU OS 5_1 like Mac OS X) AppleWebKit/534.46
(KHTML, like Gecko) Version/5.1 Mobile/9B176 Safari/7534.48.3";

We should fix this.

> UIProcess/mac/WebContextMac.mm:38
> +#if !PLATFORM(IOS)
>  #import <QuartzCore/CARemoteLayerServer.h>
> +#endif

This should be in its own block. Also we really don't need it but we'll clean
it up later.