Bug 82460 – [Chromium] Selectively retrieve text around a touched point

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Leandro Graciá Gil]

Implement an embedding API for the new editing/SurroundingText class in WebCore, as well as the possibility of highlighting the retrieved text range.

[Leandro Graciá Gil]

Created attachment 134291 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 134291 [details]
Patch

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

> Source/WebKit/chromium/public/WebHitTestInfo.h:46
> +    WebHitTestInfo();
> +    WebHitTestInfo(const WebHitTestInfo&);

Do we need to mark these functions as WEBKIT_EXPORT?  (I'm not sure.)

> Source/WebKit/chromium/public/WebView.h:437
> +#if defined(ANDROID)

Why is this specific to Android?  Presumably the API makes sense on other platforms as well.

[Adam Barth]

Comment on attachment 134291 [details]
Patch

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

This generally looks good.  I have a couple minor questions below.

> Source/WebKit/chromium/src/WebSurroundingText.cpp:57
> +    if (!node->inDocument() && element && element->inDocument())

How can a node's parent be inDocument() without the node itself being inDocument() ?

> Source/WebKit/chromium/src/WebSurroundingText.cpp:59
> +    m_private.reset(new SurroundingText(node->renderer()->positionForPoint(hitTestInfo.point()), maxLength));

Won't this crash if the node doesn't have a renderer?  Maybe that's impossible if the node is in a HitTestInfo (but then when do you need to check whether the node is inDocument)?  I wonder if you mean to be null-checking renderer() rather than checking inDocument().

> Source/WebKit/chromium/src/WebViewImpl.cpp:3470
> +#if OS(ANDROID)

Again this shouldn't be Android-specific.

[Adam Barth]

(Also, I'm going to ask fishd to double-check the review of the WebKit API aspects of this change.  I'm still learning how to review those changes properly.)

[Leandro Graciá Gil]

Created attachment 134369 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 134291 [details]
Patch

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

>> Source/WebKit/chromium/public/WebHitTestInfo.h:46
>> +    WebHitTestInfo(const WebHitTestInfo&);
> 
> Do we need to mark these functions as WEBKIT_EXPORT?  (I'm not sure.)

I've never seen the constructors of WebKit embedders using WEBKIT_EXPORT, but I'm not 100% sure either.

>> Source/WebKit/chromium/public/WebView.h:437
>> +#if defined(ANDROID)
> 
> Why is this specific to Android?  Presumably the API makes sense on other platforms as well.

Fixed.

>> Source/WebKit/chromium/src/WebSurroundingText.cpp:57
>> +    if (!node->inDocument() && element && element->inDocument())
> 
> How can a node's parent be inDocument() without the node itself being inDocument() ?

This actually came from a code snippet in WebCore that said it was the correct way to do this. However, it seems it wasn't so correct since I can't find it anymore.

>> Source/WebKit/chromium/src/WebSurroundingText.cpp:59
>> +    m_private.reset(new SurroundingText(node->renderer()->positionForPoint(hitTestInfo.point()), maxLength));
> 
> Won't this crash if the node doesn't have a renderer?  Maybe that's impossible if the node is in a HitTestInfo (but then when do you need to check whether the node is inDocument)?  I wonder if you mean to be null-checking renderer() rather than checking inDocument().

I'm following your suggestion. It makes more sense than the code snippet this was based on.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3470
>> +#if OS(ANDROID)
> 
> Again this shouldn't be Android-specific.

Fixed.

[WebKit Review Bot]

Comment on attachment 134369 [details]
Patch

Attachment 134369 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12159271

[Leandro Graciá Gil]

Created attachment 134400 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #9)
> Created an attachment (id=134400) [details]
> Patch

Fixing the build issues in the bot.

[Leandro Graciá Gil]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=134400) [details] [details]
> > Patch
> 
> Fixing the build issues in the bot.

Any reviews on this when possible? Thanks.

[Leandro Graciá Gil]

Created attachment 134829 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #12)
> Created an attachment (id=134829) [details]
> Patch

Updated the exported names of some methods in WebSurroundingText to match the names in WebCore's SurroundingText after the review changes (hitOffsetInContent -> positionOffsetInContent and minor variable name changes).

[Darin Fisher (:fishd, Google)]

Comment on attachment 134829 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:439
> +    virtual void selectRange(const WebRange&) = 0;

should we deprecate the former selectRange method in terms of this one?

> Source/WebKit/chromium/public/WebHitTestInfo.h:45
> +    WebHitTestInfo();

these will break the shared library build of chrome if not tagged with WEBKIT_EXPORT,
but we generally do not export constructors and destructors.  you should make these
in terms of assign and reset methods.  see WebNode.h

> Source/WebKit/chromium/public/WebHitTestInfo.h:56
> +    WEBKIT_EXPORT WebNode node() const;

what if more than one node got hit?

> Source/WebKit/chromium/public/WebSurroundingText.h:43
> +    WebSurroundingText();

ditto

> Source/WebKit/chromium/public/WebWidget.h:173
> +    virtual WebHitTestInfo hitTestInfoForWindowPos(const WebPoint&) { return WebHitTestInfo(); }

since hit testing returns dom nodes, i think this API belongs on WebView.
do you need it to live on WebWidget?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3470
> +WebVector<WebFloatQuad> WebViewImpl::getTouchHighlightQuads(const WebRange& webRange, WebColor& outTapHighlightColor)

why is the parameter prefixed with "out"?  is this to convey the fact that it is
an output parameter?  normally, we don't use "out" and "in" prefixes like this.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3478
> +    if (node && node->renderer())

it seems like getting the tapHighlightColor is really a function of the firstNode
represented by the given range.  it really doesn't seem to have anything to do with
highlight quads (i.e., the second half of this function).  does it really make sense
for this function to aggregate both of these things?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3484
> +    Frame* frame = page()->focusController()->focusedOrMainFrame();

how do you know that the given range corresponds to this frame?  shouldn't
you verify that?  should you perhaps get the frame from the range?

Range has a document(), which has a frame().

[Leandro Graciá Gil]

Created attachment 135343 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 134829 [details]
Patch

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

>> Source/WebKit/chromium/public/WebFrame.h:439
>> +    virtual void selectRange(const WebRange&) = 0;
> 
> should we deprecate the former selectRange method in terms of this one?

I don't think so. I see other code making use of the former not related to the one that will use this API. For example, RenderViewImpl::OnSelectRange.

>> Source/WebKit/chromium/public/WebHitTestInfo.h:45
>> +    WebHitTestInfo();
> 
> these will break the shared library build of chrome if not tagged with WEBKIT_EXPORT,
> but we generally do not export constructors and destructors.  you should make these
> in terms of assign and reset methods.  see WebNode.h

Making these in terms of init, assign and reset methods following the example of other embedders using WebPrivateOwnPtr.

>> Source/WebKit/chromium/public/WebHitTestInfo.h:56
>> +    WEBKIT_EXPORT WebNode node() const;
> 
> what if more than one node got hit?

In this case we're interested to expose only the point-based hit result, not the rect-based one. Therefore the result is the inner node of the HitTestResult instead of a set of nodes. I'm renaming the property to innerNode() make this clearer.

>> Source/WebKit/chromium/public/WebSurroundingText.h:43
>> +    WebSurroundingText();
> 
> ditto

Done.

>> Source/WebKit/chromium/public/WebWidget.h:173
>> +    virtual WebHitTestInfo hitTestInfoForWindowPos(const WebPoint&) { return WebHitTestInfo(); }
> 
> since hit testing returns dom nodes, i think this API belongs on WebView.
> do you need it to live on WebWidget?

Thanks for spotting this. This actually belongs to WebView and it seems I was missing the changes to WebViewImpl.h/cpp in this patch. Fixed.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3470
>> +WebVector<WebFloatQuad> WebViewImpl::getTouchHighlightQuads(const WebRange& webRange, WebColor& outTapHighlightColor)
> 
> why is the parameter prefixed with "out"?  is this to convey the fact that it is
> an output parameter?  normally, we don't use "out" and "in" prefixes like this.

Fixed.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3478
>> +    if (node && node->renderer())
> 
> it seems like getting the tapHighlightColor is really a function of the firstNode
> represented by the given range.  it really doesn't seem to have anything to do with
> highlight quads (i.e., the second half of this function).  does it really make sense
> for this function to aggregate both of these things?

I'm breaking down this method in two: one for getting the tap highlight color for a given node, and another to get the list of quads covering a text range.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3484
>> +    Frame* frame = page()->focusController()->focusedOrMainFrame();
> 
> how do you know that the given range corresponds to this frame?  shouldn't
> you verify that?  should you perhaps get the frame from the range?
> 
> Range has a document(), which has a frame().

Fixed.

[Alexandre Elias]

Drive-by comment: are you sure it's necessary to have WebSurroundingText and WebHitTestInfo classes at all?  How about just creating a WebViewImpl function that takes a point to hit test and returns surrounding text as a WebString?  Then you could examine this string on the Chrome side with all your detectors, without needing to manipulate WebKit objects there.

[Leandro Graciá Gil]

(In reply to comment #17)
> Drive-by comment: are you sure it's necessary to have WebSurroundingText and WebHitTestInfo classes at all?  How about just creating a WebViewImpl function that takes a point to hit test and returns surrounding text as a WebString?  Then you could examine this string on the Chrome side with all your detectors, without needing to manipulate WebKit objects there.

I don't think that would be enough. The reason is that we don't need only the string containing the surrounding text, but also to be able to find where in the string we tapped and to get a proper range based on a substring of it. These last features (especially the last one) are the reason of why we need WebSurroundingText.

WebHitTestInfo on the other hand is required in order to provide other kind of checks based on, for example, the node being hit. We might want to perform some operations only if we don't have click listeners among the tapped node's ancestors.

[Alexandre Elias]

I see.  Makes sense, thanks for the explanation.

[Leandro Graciá Gil]

Created attachment 135597 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #20)
> Created an attachment (id=135597) [details]
> Patch

Protected WebSurroundingText against a null visible position during construction. Also renamed WebHitTestInfo::point to localPoint in order to match WebCore naming.

[Darin Fisher (:fishd, Google)]

Comment on attachment 135597 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:440
>  

nit: please preserve two blank lines before section comment

> Source/WebKit/chromium/public/WebHitTestInfo.h:29
> +#include "WebNode.h"

nit: you can just forward declare WebNode, WebPoint and WebURL.  no need to #include here.

> Source/WebKit/chromium/public/WebHitTestInfo.h:43
> +class WebHitTestInfo {

is WebHitTestInfo the best name for this?  or perhaps it should be named
WebHitTestResult?  this might be a better name given that it is the return
value from a function named hitTest*, right?

> Source/WebKit/chromium/public/WebHitTestInfo.h:45
> +    WebHitTestInfo() { init(); }

why do you need to initialize in the constructor?  reset() makes
the object null (maybe you should have an isNull function?), so
it seems like the default constructor could also let the object
start out being null.  See WebHistoryItem for example.

> Source/WebKit/chromium/public/WebHitTestInfo.h:59
> +    WEBKIT_EXPORT WebNode innerNode() const;

what does inner node mean?  is there an outer node?  given only this
interface, innerNode seems like a peculiar name.  the comment doesn't
help at all.

> Source/WebKit/chromium/public/WebHitTestInfo.h:61
> +    // Point coordinates of the hit.

what coordinate system does this use?  is it document coordinates or
window (viewport) coordinates?

> Source/WebKit/chromium/public/WebHitTestInfo.h:64
> +    // True iff the hit was on an editable field or node.

nit: we generally avoid "iff"

what if the hit test was for an INPUT element?  would this be false?
is that why you are calling it "isContentEditable" instead of "isEditable"?

> Source/WebKit/chromium/public/WebHitTestInfo.h:74
> +    WEBKIT_EXPORT void init();

nit: "init" -> "initialize"

> Source/WebKit/chromium/public/WebSurroundingText.h:43
> +    WebSurroundingText() { init(); }

why do you need to make the object non-null in the default constructor?
provide an isNull method since reset() can make the object null?

> Source/WebKit/chromium/public/WebSurroundingText.h:50
> +    WEBKIT_EXPORT WebSurroundingText(const WebHitTestInfo&, size_t maxLength);

nit: normally constructors are not exported from the webkit API:
http://trac.webkit.org/wiki/ChromiumWebKitAPI#Methods

there should instead be methods to initialize or assign values to
a WebSurroundingText object.

> Source/WebKit/chromium/public/WebSurroundingText.h:52
> +    // Creates a new text content walker centered in the selected offset of the given text node.

why does the comment say "text content walker" instead of "surrounding text"?  it seems like
this function is the constructor for WebSurroundingText instead of WebTextContentWalker.
was that perhaps the old name of this class in an earlier revision of this patch?

> Source/WebKit/chromium/public/WebSurroundingText.h:56
> +    // Text content retrieved by the walker.

nit: "walker"?

> Source/WebKit/chromium/public/WebSurroundingText.h:57
> +    WEBKIT_EXPORT WebString content() const;

nit: perhaps this method should be named textContent.

> Source/WebKit/chromium/public/WebSurroundingText.h:59
> +    // Position of the initial text node offset in the content string.

these two functions are a bit awkwardly named and confusing to understand.

> Source/WebKit/chromium/public/WebSurroundingText.h:63
> +    WEBKIT_EXPORT WebRange rangeFromContentOffsets(size_t startOffsetInContent, size_t endOffsetInContent);

how is this different from WebRange::fromDocumentRange?

> Source/WebKit/chromium/public/WebSurroundingText.h:66
> +    WEBKIT_EXPORT void init();

nit: "init" -> "initialize"

> Source/WebKit/chromium/public/WebView.h:443
> +    virtual WebHitTestInfo hitTestInfoForWindowPos(const WebPoint&) = 0;

It seems like this function could just be named hitTest.  It is normal for
coordinates passed to a WebView or WebWidget to be in viewport space, which
is what "ForWindowPos" refers to.  You shouldn't need to repeat that here.
Plus, there is no companion function to hit test in document space.

> Source/WebKit/chromium/public/WebView.h:449
> +    virtual WebVector<WebFloatQuad> getTextQuadsForRange(const WebRange&) = 0;

Why are you using WebFloatQuad here?  What are these coordinates?  Why not
use a WebRect or a WebFloatRect?

It is very unclear what the result is that this function computes.  Does it
describe a bounding region for the DOM range?

> Source/WebKit/chromium/src/WebSurroundingText.cpp:47
> +    VisiblePosition visiblePosition(node->renderer()->positionForPoint(hitTestInfo.localPoint()));

it seems like the only information that you really need from the hit test data structure
is a node and a point.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3514
> +    WebColor tapHighlightColor = RenderStyle::initialTapHighlightColor().rgb();

this function does not seem to be dependent on the WebView at all.
it seems like it could either be a WebNode function or it should
live on some other interface.  probably some other interface since
WebNode is just supposed to be a reflection of a DOM node.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3516
> +      return tapHighlightColor;

nit: indentation

> Source/WebKit/chromium/src/WebViewImpl.cpp:3536
> +    range->textQuads(quads);

hmm... so WebCore makes this a function of Range.  we could similarly make it
a function of WebRange.  this function doesn't really depend on |this| at all!
you infer the Frame and the FrameView from the given Range!!

one thing that is perhaps awkward is that WebRange is meant to be a reflection
of a DOM range.  same issue that comes up with getNodeTapHighlightColor.

[Leandro Graciá Gil]

Comment on attachment 135597 [details]
Patch

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

>> Source/WebKit/chromium/public/WebFrame.h:440
>>  
> 
> nit: please preserve two blank lines before section comment

Fixed.

>> Source/WebKit/chromium/public/WebHitTestInfo.h:29
>> +#include "WebNode.h"
> 
> nit: you can just forward declare WebNode, WebPoint and WebURL.  no need to #include here.

Done.

>> Source/WebKit/chromium/public/WebHitTestInfo.h:43
>> +class WebHitTestInfo {
> 
> is WebHitTestInfo the best name for this?  or perhaps it should be named
> WebHitTestResult?  this might be a better name given that it is the return
> value from a function named hitTest*, right?

Agreed. Changing it.

>> Source/WebKit/chromium/public/WebHitTestInfo.h:45
>> +    WebHitTestInfo() { init(); }
> 
> why do you need to initialize in the constructor?  reset() makes
> the object null (maybe you should have an isNull function?), so
> it seems like the default constructor could also let the object
> start out being null.  See WebHistoryItem for example.

Fixed as suggested.

>> Source/WebKit/chromium/public/WebHitTestInfo.h:59
>> +    WEBKIT_EXPORT WebNode innerNode() const;
> 
> what does inner node mean?  is there an outer node?  given only this
> interface, innerNode seems like a peculiar name.  the comment doesn't
> help at all.

Renaming to node and clarifying a bit the comment.

>> Source/WebKit/chromium/public/WebHitTestInfo.h:61
>> +    // Point coordinates of the hit.
> 
> what coordinate system does this use?  is it document coordinates or
> window (viewport) coordinates?

These are coordinates relative to the node.

>> Source/WebKit/chromium/public/WebHitTestInfo.h:64
>> +    // True iff the hit was on an editable field or node.
> 
> nit: we generally avoid "iff"
> 
> what if the hit test was for an INPUT element?  would this be false?
> is that why you are calling it "isContentEditable" instead of "isEditable"?

After double-checking, it looks like we can get rid of this and a few other entries in this file.

>> Source/WebKit/chromium/public/WebHitTestInfo.h:74
>> +    WEBKIT_EXPORT void init();
> 
> nit: "init" -> "initialize"

Done.

>> Source/WebKit/chromium/public/WebSurroundingText.h:43
>> +    WebSurroundingText() { init(); }
> 
> why do you need to make the object non-null in the default constructor?
> provide an isNull method since reset() can make the object null?

Fixed as suggested.

>> Source/WebKit/chromium/public/WebSurroundingText.h:50
>> +    WEBKIT_EXPORT WebSurroundingText(const WebHitTestInfo&, size_t maxLength);
> 
> nit: normally constructors are not exported from the webkit API:
> http://trac.webkit.org/wiki/ChromiumWebKitAPI#Methods
> 
> there should instead be methods to initialize or assign values to
> a WebSurroundingText object.

Fixed.

>> Source/WebKit/chromium/public/WebSurroundingText.h:52
>> +    // Creates a new text content walker centered in the selected offset of the given text node.
> 
> why does the comment say "text content walker" instead of "surrounding text"?  it seems like
> this function is the constructor for WebSurroundingText instead of WebTextContentWalker.
> was that perhaps the old name of this class in an earlier revision of this patch?

Sorry, this "text content walker" was the old name of the SurroundingText class in WebCore before it got reviewed and landed. Updating the comments.

>> Source/WebKit/chromium/public/WebSurroundingText.h:56
>> +    // Text content retrieved by the walker.
> 
> nit: "walker"?

Fixed.

>> Source/WebKit/chromium/public/WebSurroundingText.h:57
>> +    WEBKIT_EXPORT WebString content() const;
> 
> nit: perhaps this method should be named textContent.

Fixed.

>> Source/WebKit/chromium/public/WebSurroundingText.h:59
>> +    // Position of the initial text node offset in the content string.
> 
> these two functions are a bit awkwardly named and confusing to understand.

Fixed.

>> Source/WebKit/chromium/public/WebSurroundingText.h:63
>> +    WEBKIT_EXPORT WebRange rangeFromContentOffsets(size_t startOffsetInContent, size_t endOffsetInContent);
> 
> how is this different from WebRange::fromDocumentRange?

This is not related to the current selection, and the range it returns is purely based on a substring of the returned text content.

>> Source/WebKit/chromium/public/WebSurroundingText.h:66
>> +    WEBKIT_EXPORT void init();
> 
> nit: "init" -> "initialize"

Fixed.

>> Source/WebKit/chromium/public/WebView.h:443
>> +    virtual WebHitTestInfo hitTestInfoForWindowPos(const WebPoint&) = 0;
> 
> It seems like this function could just be named hitTest.  It is normal for
> coordinates passed to a WebView or WebWidget to be in viewport space, which
> is what "ForWindowPos" refers to.  You shouldn't need to repeat that here.
> Plus, there is no companion function to hit test in document space.

This method is being removed as further code (in a later patch) is moving to WebViewImpl.

>> Source/WebKit/chromium/public/WebView.h:449
>> +    virtual WebVector<WebFloatQuad> getTextQuadsForRange(const WebRange&) = 0;
> 
> Why are you using WebFloatQuad here?  What are these coordinates?  Why not
> use a WebRect or a WebFloatRect?
> 
> It is very unclear what the result is that this function computes.  Does it
> describe a bounding region for the DOM range?

WebFloatQuad it's a wrapper around the FloatQuad type that the DOM range. Unlike rects, FloatQuads provide a transform-friendly result. Check Range::textQuads for details.

>> Source/WebKit/chromium/src/WebSurroundingText.cpp:47
>> +    VisiblePosition visiblePosition(node->renderer()->positionForPoint(hitTestInfo.localPoint()));
> 
> it seems like the only information that you really need from the hit test data structure
> is a node and a point.

Yes, I'm cleaning the structure up.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3514
>> +    WebColor tapHighlightColor = RenderStyle::initialTapHighlightColor().rgb();
> 
> this function does not seem to be dependent on the WebView at all.
> it seems like it could either be a WebNode function or it should
> live on some other interface.  probably some other interface since
> WebNode is just supposed to be a reflection of a DOM node.

I'm temporarily removing this method. It will be added later in a separate patch introducing the rest of the content detection functionality. Following your advice it won't be a member of WebViewImpl, but moving it to WebNode doesn't sound good either. Perhaps a static method in WebViewImpl.cpp?

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3516
>> +      return tapHighlightColor;
> 
> nit: indentation

Fixed.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3536
>> +    range->textQuads(quads);
> 
> hmm... so WebCore makes this a function of Range.  we could similarly make it
> a function of WebRange.  this function doesn't really depend on |this| at all!
> you infer the Frame and the FrameView from the given Range!!
> 
> one thing that is perhaps awkward is that WebRange is meant to be a reflection
> of a DOM range.  same issue that comes up with getNodeTapHighlightColor.

Agreed, it makes more sense to move this to WebRange.

[Leandro Graciá Gil]

Created attachment 137574 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 137574 [details]
Patch

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

> Source/WebKit/chromium/src/WebRange.cpp:124
> +

nit: delete extra new line

> Source/WebKit/chromium/src/WebSurroundingText.cpp:43
> +void WebSurroundingText::initialize(const WebHitTestResult& hitTestInfo, size_t maxLength)

it could also make sense for these to be static create functions.

[Leandro Graciá Gil]

Comment on attachment 137574 [details]
Patch

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

>> Source/WebKit/chromium/src/WebRange.cpp:124
>> +
> 
> nit: delete extra new line

Fixing directly in the commit.

>> Source/WebKit/chromium/src/WebSurroundingText.cpp:43
>> +void WebSurroundingText::initialize(const WebHitTestResult& hitTestInfo, size_t maxLength)
> 
> it could also make sense for these to be static create functions.

After checking other similar files in chromium/src it looks like it's more usual and appropriate to use initialize for cases like this. Static create methods are normally used when there is a constructor to build the WebKit embedder object from a reference-counted WebCore object, so that we could create embedder objects as we please for a WebCore object. In this case the embedder creates and owns the WebCore object, making not much sense to have such constructor. So, I'll leave it as initialize and proceed with landing. If you have any objections please let me know.

[Leandro Graciá Gil]

Committed r115463: <http://trac.webkit.org/changeset/115463>

[Dimitri Glazkov (Google)]

Reverted r115463 for reason:

Broke Mac build.

Committed r115467: <http://trac.webkit.org/changeset/115467>

[Leandro Graciá Gil]

Committed r115472: <http://trac.webkit.org/changeset/115472>

[Steve Block]

Comment on attachment 137574 [details]
Patch

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

> Source/WebKit/chromium/public/WebHitTestResult.h:38
> +class WebPoint;

I think this should be 'struct WebPoint'

[Leandro Graciá Gil]

That's exactly what was fixed when landing this for the second time. The mac bot (apparently clang) detected that problem and a similar one. Both are fixed in the landed code.

(In reply to comment #30)
> (From update of attachment 137574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137574&action=review
> 
> > Source/WebKit/chromium/public/WebHitTestResult.h:38
> > +class WebPoint;
> 
> I think this should be 'struct WebPoint'