[webkit-reviews] review denied: [Bug 104186] Draw selection and cursor handles for touch based text editing : [Attachment 189625] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Varun Jain
<varunjain at chromium.org>'s request for review:
Bug 104186: Draw selection and cursor handles for touch based text editing
https://bugs.webkit.org/show_bug.cgi?id=104186

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=189625&action=review


This patch isn't sufficiently tested given the rather large amount of
implementation logic.  I've left a number of comments below to help you out.  A
good number are stylistic things and issues with fitting in with the rest of
the WebKit codebase.  For these sorts of issues you may find it more productive
to work directly with someone who sits close to you who can guide you in the
right direction.

At a higher level, I'd like to understand why you need to add new APIs for
interaction between WebKit and the embedder for these selection handles.  We
already have a fair amount of API by which WebKit and its embedder can keep
track of and manipulate the current selection range.  It's not clear to me why
we need more for these handles since they seem to simply sit around the
selection range.  It would be helpful if you could describe the situations in
which you expect these to be used.

> Source/WebKit/chromium/public/WebWidget.h:212
> +    // Called to inform the WebView that the selection/cursor on the webpage
has changed.
> +    virtual void updateTouchEditing() { };

no trailing ;

Why does this feature need a new set of WebKit APIs?

> Source/WebKit/chromium/src/TouchEditing.cpp:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

it's 2013

> Source/WebKit/chromium/src/TouchEditing.cpp:42
> +#include "third_party/skia/include/utils/SkMatrix44.h"

in WebKit (unlike in chromium) this sort of include is written as just #include
"SkMatrix44.h".  The dependency on the skia target adds it to the include path

> Source/WebKit/chromium/src/TouchEditing.cpp:57
> +namespace {

No need for an anonymous namespace if everything inside is static

> Source/WebKit/chromium/src/TouchEditing.cpp:71
> +    for (unsigned i = 0; i < 4; ++i) {
> +	   IntPoint point;
> +	   switch (i) {
> +	   case 0: point = roundedIntPoint(targetSpaceQuad.p1()); break;
> +	   case 1: point = roundedIntPoint(targetSpaceQuad.p2()); break;
> +	   case 2: point = roundedIntPoint(targetSpaceQuad.p3()); break;
> +	   case 3: point = roundedIntPoint(targetSpaceQuad.p4()); break;
> +	   }

Really?  There's no better way to map a quad through a transform?  This is
crazy ugly

> Source/WebKit/chromium/src/TouchEditing.cpp:87
> +static const int handlePadding = 20;
> +static const int handleRadius = 10;

Where did these values come from? How could they be changed?

> Source/WebKit/chromium/src/TouchEditing.cpp:140
> +	   m_selectionHandle1.clear();
> +	   m_selectionHandle2.clear();

"selectionHandle1" and "selectionHandle2" are terribly undescriptive names. Is
there nothing better?  Does one handle represent the start and one the end or
something like that?

> Source/WebKit/chromium/src/TouchEditing.cpp:195
> +    if (m_selectionHandle1.get()) {

no ".get()" for checking the nullity of a smart pointer in WebKit (or in
chromium, for that matter).  just if (m_selectionHandle1)

> Source/WebKit/chromium/src/TouchEditing.cpp:197
> +	       || m_selectionHandle2->handleEvent(eventType, location,
controller);

does the existence of m_selelectionHandle1 imply that m_selectionHandle2 is
always also not null?

> Source/WebKit/chromium/src/TouchEditing.cpp:298
> +    WebRect hitRect(m_rect.x - handleRadius - handlePadding,
> +	   m_rect.y + m_rect.height,
> +	   2 * (handleRadius + handlePadding),
> +	   2 * (handleRadius + handlePadding));

there's a lot of nontrivial per-component math here and in the rest of the
function.  I think you should familiarize yourself a bit with the WebCore
geometry classes and use them to perform logical operations like "scale by this
amount" or "translate by this offset".	This will make the code much easier to
understand.

> Source/WebKit/chromium/src/TouchEditing.cpp:302
> +    return hitRect.x < location.x
> +	   && hitRect.x + hitRect.width > location.x
> +	   && hitRect.y < location.y
> +	   && hitRect.y + hitRect.height > location.y;

For instance, bool WebCore::IntRect::contains(const IntPoint&) looks useful
here (although you need to be careful about the edge cases)

> Source/WebKit/chromium/src/TouchEditing.cpp:333
> +    IntRect clipRect(IntPoint(webClipRect.x, webClipRect.y),
IntSize(webClipRect.width, webClipRect.height));

WebRect defines operator WebCore::IntRect() const, why do you need these
per-component operations?

> Source/WebKit/chromium/src/TouchEditing.cpp:336
> +    gc.setFillColor(Color(0, 0, 0, 128), ColorSpaceDeviceRGB);
> +    gc.setStrokeColor(Color(0, 0, 0, 128), ColorSpaceDeviceRGB);

these colors should be in a named constant with some comments indicating how
they were chosen and what the procedure would be to update them

> Source/WebKit/chromium/src/TouchEditing.cpp:381
> +    m_clipLayer->setSublayerTransform(WebTransformationMatrix());
> +    if (!newGraphicsLayer->drawsContent()) {
> +	  
m_clipLayer->setSublayerTransform(WebTransformationMatrix(newGraphicsLayer->tra
nsform()));

I don't understand these sublayer transform modifications. What are they for?

> Source/WebKit/chromium/src/TouchEditing.h:54
> +namespace test {
> +class TouchEditingTest;
> +}

You shouldn't refer to test namespaces or classes in production code.

> Source/WebKit/chromium/src/TouchEditing.h:67
> +    virtual WebCore::FrameSelection* getCurrentSelection() = 0;
> +    virtual WebCore::GraphicsLayer* getRootGraphicsLayer() = 0;

getters in WebKit (functions that return a value) should not have a 'get'
prefix. That prefix is used only when the function computes values into out
params instead of returning them

> Source/WebKit/chromium/src/TouchEditing.h:94
> +    class TouchEditingHandle : public WebContentLayerClient {

It's quite rare to use inner classes in WebKit or to define multiple concrete
classes in a single header.  I think you should define this class in its own
header file with its own cpp

> Source/WebKit/chromium/src/TouchEditing.h:96
> +	   TouchEditingHandle(TouchEditing*);

explicit for one-argument constructors

> Source/WebKit/chromium/src/TouchEditing.h:125
> +	   TouchEditing* m_touchEditing;
> +	   OwnPtr<WebContentLayer> m_contentLayer;
> +	   OwnPtr<WebLayer> m_clipLayer;
> +	   WebCore::Path m_path;
> +	   WebCore::GraphicsLayerChromium* m_currentGraphicsLayer;
> +	   bool m_hasCapture;
> +	   WebRect m_rect;
> +	   WebRect m_transformedRect;

why are these all public?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3582
> +bool WebViewImpl::handleInputEventForTouchEditing(const WebInputEvent&
inputEvent)

This function has far too much code particular to touch handling to be in
WebViewImpl, which is already an overburdened class.  Figure out a logical
place for the details of handling this sort of input outside of WebViewImpl.cpp
and move this code there

> Source/WebKit/chromium/src/WebViewImpl.h:630
> +    // Exposed for tests.
> +    TouchEditing* touchEditing() { return m_touchEditing.get(); }

Blank line before comment.  It's pretty unusual to expose classes like this
just for tests.  I suspect if you extract more of the logic related to touch
handling out of WebViewImpl you may not need to expose so many of WebViewImpl's
internals to your tests.

> Source/WebKit/chromium/src/WebViewImpl.h:686
> +    bool touchEditingEnabled();
> +    bool pageHasSelectionOrCaret();
> +    bool handleInputEventForTouchEditing(const WebInputEvent&);
> +    void createTouchEditing();
> +    void clearTouchEditing();

5 new helper functions for this?  Ouch

> Source/WebKit/chromium/tests/TouchEditingTest.cpp:88
> +TEST_F(TouchEditingTest, verifyWebViewImplIntegration)

Adding 'verify' to a unit test name doesn't add anything.  All tests exists to
verify something, that's why they are tests

> Source/WebKit/chromium/tests/TouchEditingTest.cpp:137
> +    // Now tap else where to hide all handles.

There seem to be far fewer test scenarios than cases in the code under test. 
For instance, I don't see tests for any of the RenderLayer / transform / etc
logic.	Make sure that your tests all of the logic they are intended to cover.