Bug 66681 – Need API for getting surrounding text from webkit in chromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Peng Huang]

The API is for getting surrounding text from webkit in chromium.
Some input methods may generate different results depend on the text around input cursor.

[Peng Huang]

Created attachment 104692 [details]
Patch

[Hajime Morrita]

Comment on attachment 104692 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:154
> +    virtual bool surrounding(WebString& text, size_t& cursor, size_t& anchor) { return false; }

You can return WebSize. See (Source/WebKit/public/WebSize.h)

> Source/WebKit/chromium/src/WebViewImpl.cpp:1512
> +bool WebViewImpl::surrounding(WebString& text, size_t& cursor, size_t& anchor)

It's sad to have this logic in WebKit layer.
If we have this in WebCore and make it available from LayoutTestcontroller or window.internals, It can be testable
and also would be helpful for other ports.

[Hajime Morrita]

CC-ed rniwa to see editing API usage, fishd for the Chromium API addition.

[Ryosuke Niwa]

Comment on attachment 104692 [details]
Patch

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

>> Source/WebKit/chromium/public/WebWidget.h:154
>> +    virtual bool surrounding(WebString& text, size_t& cursor, size_t& anchor) { return false; }
> 
> You can return WebSize. See (Source/WebKit/public/WebSize.h)

The standard term used is focus and anchor.  "cursor" usually refers to mouse cursor, and it's confusing.  Alos, Why isn't this function const?  Also the last time I checked, Chromium's coding style guide forbid mutable reference and instead forces pass by pointer.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1530
> +    Element* scope = selectionRoot ? selectionRoot : frame->document()->documentElement();

Wait a minute, if rootEditableElement can be null, then element->innerText() would have blown up by null-pointer access, no?  r- because of this.

Also, this line appears to be much longer than 80 characters, yet chromium coding guideline mandates the column length to be at most 80 characters.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1539
> +    anchor = TextIterator::rangeLength(testRange.get());
> +
> +    ExceptionCode ec;
> +    testRange->setEnd(selection->extent().containerNode(), selection->extent().offsetInContainerNode(), ec);
> +    cursor = TextIterator::rangeLength(testRange.get());

You should call TextIterator::locationAndLengthFromRange instead.

[Peng Huang]

Comment on attachment 104692 [details]
Patch

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

>>> Source/WebKit/chromium/public/WebWidget.h:154
>>> +    virtual bool surrounding(WebString& text, size_t& cursor, size_t& anchor) { return false; }
>> 
>> You can return WebSize. See (Source/WebKit/public/WebSize.h)
> 
> The standard term used is focus and anchor.  "cursor" usually refers to mouse cursor, and it's confusing.  Alos, Why isn't this function const?  Also the last time I checked, Chromium's coding style guide forbid mutable reference and instead forces pass by pointer.

struct WebSize {
    int width;
    int height;
...}

cursor, and anchor are indexes in plain text. I think it is different with WebSize.

I will change the names.

For coding style, I think the code is in webkit, probably it is better to follow webkit coding guideline? And I also find some functions use mutable reference in this header file. I am confused.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1512
>> +bool WebViewImpl::surrounding(WebString& text, size_t& cursor, size_t& anchor)
> 
> It's sad to have this logic in WebKit layer.
> If we have this in WebCore and make it available from LayoutTestcontroller or window.internals, It can be testable
> and also would be helpful for other ports.

Could you give more detail information? So I could look into it.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1530

> 
> Wait a minute, if rootEditableElement can be null, then element->innerText() would have blown up by null-pointer access, no?  r- because of this.
> 
> Also, this line appears to be much longer than 80 characters, yet chromium coding guideline mandates the column length to be at most 80 characters.

I will fixed rootEditableElement issue.

Probably those code is in webkit repo. It should follow webkit coding guideline. For example indent is 2 spaces in chrome, but here is 4 spaces.

[Ryosuke Niwa]

(In reply to comment #5)
> For coding style, I think the code is in webkit, probably it is better to follow webkit coding guideline? And I also find some functions use mutable reference in this header file. I am confused.

Mn... it appears to me WebViewImpl.cpp follows Chromium coding convention but I can be wrong. Darin probably knows what we should be doing here.

> >> Source/WebKit/chromium/src/WebViewImpl.cpp:1512
> >> +bool WebViewImpl::surrounding(WebString& text, size_t& cursor, size_t& anchor)
> > 
> > It's sad to have this logic in WebKit layer.
> > If we have this in WebCore and make it available from LayoutTestcontroller or window.internals, It can be testable
> > and also would be helpful for other ports.
> 
> Could you give more detail information? So I could look into it.

The logic to extract surrounding text exists in many WebKit ports.  I'm actually surprised that we don't already have a logic to do this for IME.

> Probably those code is in webkit repo. It should follow webkit coding guideline. For example indent is 2 spaces in chrome, but here is 4 spaces.

That's true.  But most of lines in that file appear to fit in 80-characters.  So I'm a bit confused here as well.

[Ryosuke Niwa]

+ap since he has recently done some nice refactoring in this area.  Maybe we want to do the same for surrounding text.  I've seen so many ports implementing this function incorrectly :(

[Alexey Proskuryakov]

Doesn't Chromium already implement attributedSubstringForProposedRange:proposedRange: on Mac? That's how input methods normally get surrounding text, so it's unclear why any new code is needed.

[Darin Fisher (:fishd, Google)]

Comment on attachment 104692 [details]
Patch

by the way, the method name "surrounding" is perhaps a bit too vague.  maybe it should be named getSurroundingText?

[Tony Chang]

Comment on attachment 104692 [details]
Patch

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

Since the file is in the webkit repo, we should follow the webkit coding style.  Fitting into 80 cols does not violate the webkit coding style.

This file has a mix of styles, but we should try to write new code using webkit style.

> Source/WebKit/chromium/ChangeLog:4
> +        Add API for getting surrounding text from webkit in chromium
> +        https://bugs.webkit.org/show_bug.cgi?id=66681

Please explain why this you need this in the changelog (copying the text from comment #0 would be sufficient).

> Source/WebKit/chromium/src/WebViewImpl.cpp:1541
> +    // TODO truncate the text into reansonable size

WebKit style uses FIXME rather than TODO.

[Hajime Morrita]

> struct WebSize {
>     int width;
>     int height;
> ...}
> 
> cursor, and anchor are indexes in plain text. I think it is different with WebSize.
That's true. My feedback there was pointless.


> >> Source/WebKit/chromium/src/WebViewImpl.cpp:1512
> >> +bool WebViewImpl::surrounding(WebString& text, size_t& cursor, size_t& anchor)
> > 
> > It's sad to have this logic in WebKit layer.
> > If we have this in WebCore and make it available from LayoutTestcontroller or window.internals, It can be testable
> > and also would be helpful for other ports.
> 
> Could you give more detail information? So I could look into it.
> 
These code can be located somewhere under WebCore/, then it could be testable
using Webkit testing infrastructure. FrameSelection looks a good candidate.

[Peng Huang]

(In reply to comment #4)
> (From update of attachment 104692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104692&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1539
> > +    anchor = TextIterator::rangeLength(testRange.get());
> > +
> > +    ExceptionCode ec;
> > +    testRange->setEnd(selection->extent().containerNode(), selection->extent().offsetInContainerNode(), ec);
> > +    cursor = TextIterator::rangeLength(testRange.get());
> 
> You should call TextIterator::locationAndLengthFromRange instead.

In this case, cursor and anchor is not a range object, and the order of cursor and anchor is not same (depends on the direction of the selection). So I think using TextIterator::rangeLength() directly is simpler and efficient. What do you think?

[Ryosuke Niwa]

Comment on attachment 104692 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:1539
>>> +    cursor = TextIterator::rangeLength(testRange.get());
>> 
>> You should call TextIterator::locationAndLengthFromRange instead.
> 
> In this case, cursor and anchor is not a range object, and the order of cursor and anchor is not same (depends on the direction of the selection). So I think using TextIterator::rangeLength() directly is simpler and efficient. What do you think?

That sounds like an anti-pattern to me. Why do we need this API? What is it used for?

[Peng Huang]

(In reply to comment #13)
> > In this case, cursor and anchor is not a range object, and the order of cursor and anchor is not same (depends on the direction of the selection). So I think using TextIterator::rangeLength() directly is simpler and efficient. What do you think?
> 
> That sounds like an anti-pattern to me. Why do we need this API? What is it used for?

Some input method can generate different input depends on text before or after input cursor.

And some input method can also reconvert committed text or selected text to some others.

For example, the mozc input method can also reconvert committed text input Hiragana or Katakana, and etc.

This API is for getting the surrounding text and anchor, cursor position in surrounding text.

[Peng Huang]

Created attachment 105686 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 105686 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:1887
> +bool FrameSelection::getSurroundingText(String& text, size_t& focus, size_t& anchor) const

It seems like the function name should reflect the fact it only obtains text in the content editable area.  Also, I'd expect the return value of a function named get...Text to be a string.

> Source/WebCore/editing/FrameSelection.cpp:1893
> +    if (!isContentEditable())
> +        return false;
> +
> +    Element* element = rootEditableElement();
> +    if (!element)

This check is redundant. If there's no root editable element, then it's surely not editable.

> Source/WebCore/editing/FrameSelection.cpp:1900
> +    RefPtr<Range> testRange = Range::create(
> +        element->document(), element, 0,
> +        base().containerNode(), base().offsetInContainerNode());

You should call computeOffsetInContainerNode instead.  Otherwise this will hit an assertion.  r- because of this.

> Source/WebCore/editing/FrameSelection.cpp:1905
> +    testRange->setEnd(extent().containerNode(),
> +        extent().offsetInContainerNode(), ec);

Why don't you just do setEnd(extent(), ec) ?

> Source/WebCore/editing/FrameSelection.cpp:1908
> +    // FIXME truncate the text into reansonable size

Nit: need a colon after FIXME.

> Source/WebCore/editing/FrameSelection.h:246
> +    bool getSurroundingText(String&, size_t&, size_t&) const;

You probably need to say what these size_t's are.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1513
> +    String tmp;

Let's not use such an ambiguous shorthanded name.  We can rename text to webText or passedText.  Alternatively we can rename tmp to coreText.

[Alexey Proskuryakov]

As mentioned in comment 8, WebCore (and even WebKit/chromium) should already have all the necessary functionality. Why is this code being added?

[Peng Huang]

(In reply to comment #17)
> As mentioned in comment 8, WebCore (and even WebKit/chromium) should already have all the necessary functionality. Why is this code being added?

Sorry. I don't know which functions were you mentioned. Could you please give more detail? Thanks.

Just to clarify. We need API in chromium (all platforms). It can get the plain text surrounding the current input cursor, and the cursor index in the plain text. If selection exists, we want to get the index of the anchor as well.

[Ryosuke Niwa]

Mn... now that I think about it, you should be able to use locationAndLengthFromRange and then ask FrameSelection::baseIsFirst() to determine which one is anchor and focus.

[Alexey Proskuryakov]

What I'm saying is that this functionality exists in input methods on Mac. They work through NSTextInputClient protocol, which is implemented in WebKit. You can take a look at the implementation.

[Darin Fisher (:fishd, Google)]

Yes, indeed we do implement something like this on the Mac.  See WebKit::WebSubstringUtil::attributedSubstringInRange(...) in WebKit/chromium/public/mac/WebSubstringUtil.h.

It appears to be implemented as a forked copy of a similar function defined in WebKit/mac/Misc/WebNSAttributedStringExtras.mm.  Forking :-(

Obviously, that does not work on non-Mac platforms.  It is not so clear to me that it is exactly analogous to what we want in this case, but I haven't studied it very closely.

Thanks for the tip!

[Peng Huang]

(In reply to comment #20)
> What I'm saying is that this functionality exists in input methods on Mac. They work through NSTextInputClient protocol, which is implemented in WebKit. You can take a look at the implementation.

I checked related functions. They are for Mac platform only. And the interfaces are different with Linux and Windows. On Linux, IME does not specify a rang for getting the surrounding text. The application need report the cursor and selection with surrounding plain text. So we need implement the new API. I will make WebSubstringUtil::attributedSubstringInRange() as a reference.

Thanks.

[Peng Huang]

(In reply to comment #16)
> (From update of attachment 105686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105686&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:1887
> > +bool FrameSelection::getSurroundingText(String& text, size_t& focus, size_t& anchor) const
> 
> It seems like the function name should reflect the fact it only obtains text in the content editable area.  Also, I'd expect the return value of a function named get...Text to be a string.

How about getSurrounding() or getSurroundingTextAndSelection()? Or do you have some better suggestions?

[Ryosuke Niwa]

(In reply to comment #23)
> How about getSurrounding() or getSurroundingTextAndSelection()? Or do you have some better suggestions?

Neither of those names signify the fact this function only works in editable region.

[Peng Huang]

(In reply to comment #24)
> (In reply to comment #23)
> > How about getSurrounding() or getSurroundingTextAndSelection()? Or do you have some better suggestions?
> 
> Neither of those names signify the fact this function only works in editable region.

getSurroundingIfEditable(), getSurroundingAndSelectionIfEditable(), getInputSurrounding() or getInputSurroundingAndSelection(). 

? I can not get better names. BTW, I prefer without AndSelection suffix personally.

[Ryosuke Niwa]

(In reply to comment #25)
> getSurroundingIfEditable(), getSurroundingAndSelectionIfEditable(), getInputSurrounding() or getInputSurroundingAndSelection(). 
> 
> ? I can not get better names. BTW, I prefer without AndSelection suffix personally.

I'd call it textInEditableElementWithSelectionAnchorAndFocus or textInEditableElementWithSelection.

[Ryosuke Niwa]

(In reply to comment #26)
> (In reply to comment #25)
> > getSurroundingIfEditable(), getSurroundingAndSelectionIfEditable(), getInputSurrounding() or getInputSurroundingAndSelection(). 
> > 
> > ? I can not get better names. BTW, I prefer without AndSelection suffix personally.
> 
> I'd call it textInEditableElementWithSelectionAnchorAndFocus or textInEditableElementWithSelection.

I mean textInRootEditableElementWithSelectionAnchorAndFocus or textInRootEditableElementWithSelection

[Peng Huang]

(In reply to comment #27)
> I mean textInRootEditableElementWithSelectionAnchorAndFocus or textInRootEditableElementWithSelection

textInRootEditableElementWithSelection looks better. I will use it.

Thanks.

[Peng Huang]

(In reply to comment #19)
> Mn... now that I think about it, you should be able to use locationAndLengthFromRange and then ask FrameSelection::baseIsFirst() to determine which one is anchor and focus.

I think locationAndLengthFromRange() does not have a clear definition. What is the origin position of the location? It may be in the rootEditableElement of the range or in whole document. The scope is unclear.

[Ryosuke Niwa]

(In reply to comment #29)
> (In reply to comment #19)
> > Mn... now that I think about it, you should be able to use locationAndLengthFromRange and then ask FrameSelection::baseIsFirst() to determine which one is anchor and focus.
> 
> I think locationAndLengthFromRange() does not have a clear definition. What is the origin position of the location? It may be in the rootEditableElement of the range or in whole document. The scope is unclear.

Then we should address that issue rather than duplicating code.

[Alexey Proskuryakov]

> I checked related functions. They are for Mac platform only.

It's obvious that NSTextInputClient is a Mac specific interface. It's implementation is a thin wrapper around WebCore.

What I'm saying is that it's very unlikely that you genuinely need to make any WebCore changes.

[Ryosuke Niwa]

Peng, we can talk about how to proceed with this bug on IRC if you'd like. Alexey and I are known as ap and rniwa on #webkit respectively.

[Peng Huang]

After discussion on IRC, we decided to put code in WebKit layer and use locationAndLengthFromRange.

[Peng Huang]

Created attachment 106034 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 106034 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:4
> +        Add API for getting surrounding text and selection range
> +        from webkit in chromium.

Why is this line awkwardly wrapped in the middle of a sentence?

> Source/WebKit/chromium/ChangeLog:8
> +

You need to describe what kind of changes you're making.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1533
> +    if (selection->base().containerNode() == selection->extent().containerNode()) {
> +        // In most case focus and anchor are in the same element. We get the
> +        // text from this element.
> +        Node* node = selection->base().containerNode();
> +        if (!node || !node->isElementNode())
> +            return false;
> +        Element* element = static_cast<Element*>(selection->base().containerNode());
> +        text = element->innerText();
> +        focus = selection->base().computeOffsetInContainerNode();
> +        anchor = selection->extent().computeOffsetInContainerNode();
> +        return true;
> +    }

Let's not do this.  It's anti-pattern, and IME probably needs more context than what it fits in one element.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1538
> +    // If focus and anchor are in different nodes, we will get the text from
> +    // the root editable element.
> +    // FIXME: Find the smaller scope which can conver the focus and anchor
> +    // instead of the root editable element.

This is a completely tangential issue that's present throughout WebKit's code that came out of the discussion ap & I had.  I don't think you should include FIXME like this here.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1556
> +    Element* element = selection->rootEditableElement();

You should do this where you check whether selection is in a contenteditable region or not.

[Peng Huang]

Created attachment 106046 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 106046 [details]
Patch

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

r- due to various nits.

> Source/WebKit/chromium/ChangeLog:7
> +

Again, please add description here.

> Source/WebKit/chromium/public/WebWidget.h:157
> +    virtual bool surroundingTextWithSelection(
> +        WebString& text,
> +        size_t& focus,
> +        size_t& anchor) const { return false; }

I'd put everything in one line.  Also, "text" doesn't add any value here so you can omit.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1522
> +    const FrameSelection* selection = frame->selection();
> +    if (!selection->isContentEditable())
> +        return false;
> +
> +    Element* element = selection->rootEditableElement();

I would have done:
Element* element = selection->rootEditableElement();
if (!element)
    return false;
instead of checking isContentEditable then implicitly assuming that element is not null.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1532
> +    if (!range.get())
> +        return false;
> +
> +    if (!TextIterator::locationAndLengthFromRange(range.get(), location, length))
> +        return false;

You can combine these two statements:
if (!range || TextIterator::locationAndLengthFromRange(range.get(), location, length))
(notice there's no need to call get() on the nullity check).

> Source/WebKit/chromium/src/WebViewImpl.h:123
> +    virtual bool surroundingTextWithSelection(
> +        WebString& text,
> +        size_t& focus,
> +        size_t& anchor) const;

Ditto about putting it in one line and omitting text.

[Peng Huang]

Created attachment 106061 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 106061 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:8
> +        Add surroundingTextWithSelection() to Chromium's WebViewImpl

This doesn't explain anything about the function.  In fact, I can see that from the code change.  What I'd like to know instead is why we're adding this function and why it has this particular interface, etc...

> Source/WebKit/chromium/public/WebWidget.h:154
> +    virtual bool surroundingTextWithSelection(WebString&, size_t& focus, size_t& anchor) const { return false; }

I don't think "surroundingText" is a good name here because what it surrounds is ambiguous. Maybe getSelectionOffsetsAndTextInEditableElement?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1512
> +bool WebViewImpl::surroundingTextWithSelection(
> +    WebString& text, size_t& focus, size_t& anchor) const

Nit: I would put all in one line.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1523
> +    text = element->innerText();

It seems odd that we'll obtain text even when selection range is 0 or locationAndLengthFromRange returns false.
Since innerText is a pretty expensive operation, we should probably move this below that return statement.

[Ryosuke Niwa]

s/getSelectionOffsetsAndTextInEditableElement/getSelectionOffsetsAndTextInRootEditableElement/.

[Peng Huang]

(In reply to comment #39)
> 
> I don't think "surroundingText" is a good name here because what it surrounds is ambiguous. Maybe getSelectionOffsetsAndTextInEditableElement?

I think surroundingText is accurate even without WithSelection suffix (Without the cursor position, this text is useless. Or we need return two pieces of text. before cursor and after cursor). Maybe it is little brief, but it could be clear with a good document. At same time, the input methods just need a small piece of text surrounding the input cursor (The Surrounding Text concept is from qt and gtk), they do not need all content in the editable element. one paragraph is enough. Although this function returns all content in the root editable element currently, but we may optimize it to return a text in reasonable size in future. Especially in Chrome OS, those data will be transferred between several processes (webkit, chrome ui, input daemon and input engine process) frequently. Too large is not efficient.

> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1512
> > +bool WebViewImpl::surroundingTextWithSelection(
> > +    WebString& text, size_t& focus, size_t& anchor) const
> 
> Nit: I would put all in one line.
I could put all in one line, but it will be very long.

[Ryosuke Niwa]

(In reply to comment #41)
> I think surroundingText is accurate even without WithSelection suffix (Without the cursor position, this text is useless. Or we need return two pieces of text. before cursor and after cursor). Maybe it is little brief, but it could be clear with a good document.

In WebKit, we avoid documentation in the favor of self-evident code. We should avoid giving a function name that requires comment or documentation to convey its purpose.

> At same time, the input methods just need a small piece of text surrounding the input cursor (The Surrounding Text concept is from qt and gtk), they do not need all content in the editable element. one paragraph is enough. Although this function returns all content in the root editable element currently, but we may optimize it to return a text in reasonable size in future.

Fair enough. But we should still emphasize the fact this function only works within an editable content.

> > > Source/WebKit/chromium/src/WebViewImpl.cpp:1512
> > > +bool WebViewImpl::surroundingTextWithSelection(
> > > +    WebString& text, size_t& focus, size_t& anchor) const
> > 
> > Nit: I would put all in one line.
> I could put all in one line, but it will be very long.

I don't think 101 characters is considered long in WebKit's code base.

[Hironori Bono]

Greetings,

This is just an off-the-topic question: it seems this change does not check whether the focused element is a password field. I'm wondering how this change prevents IMEs from reading text in password fields as [WebHTMLView attributedSubStringFromRange] does. If I recall correctly, Chrome OS has an IME extension API so extension developers can implement IMEs and virtual keyboards. If WebKit allows IMEs to read text in password fields, it may allow extension developers to read text in password fields. (I'm not sure if it is good to allow such extension developers to read text in other input fields.)

By the way, to implement [attributedSubstringFromRange:nsRange], Mac Chrome adds the WebKit::WebSubstringUtil class. Maybe we use this class for consistency? (Even though this is a random thought, it seems Mac Chrome uses a round-trip IPC request to implement this method. It might be better for a renderer to send attributed substring to a browser when it is changed to avoid such round-trip IPCs as we send cursor position?)

Regards,

Hironori Bono

[Peng Huang]

(In reply to comment #43)
> Greetings,
> 
> This is just an off-the-topic question: it seems this change does not check whether the focused element is a password field. I'm wondering how this change prevents IMEs from reading text in password fields as [WebHTMLView attributedSubStringFromRange] does. If I recall correctly, Chrome OS has an IME extension API so extension developers can implement IMEs and virtual keyboards. If WebKit allows IMEs to read text in password fields, it may allow extension developers to read text in password fields. (I'm not sure if it is good to allow such extension developers to read text in other input fields.)
> 
> By the way, to implement [attributedSubstringFromRange:nsRange], Mac Chrome adds the WebKit::WebSubstringUtil class. Maybe we use this class for consistency? (Even though this is a random thought, it seems Mac Chrome uses a round-trip IPC request to implement this method. It might be better for a renderer to send attributed substring to a browser when it is changed to avoid such round-trip IPCs as we send cursor position?)

http://codereview.chromium.org/7824037/diff/1/content/renderer/render_widget.cc
Here is the related code in chromium. I plan check password field in chromium side. I think it is a little more efficient. Anyway, we could add check code in webkit as well.

At same time, it will notify input method any changes of surrounding text instead of sync round-trip IPCs.

[Peng Huang]

Created attachment 106175 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 106175 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:153
> +    virtual bool getTextSurroundEditCaret(WebString&, size_t& focus, size_t& anchor) const { return false; }

"EditCaret" is inaccurate name because non-collapsed selection isn't a caret. Also you have two verbs (get & surround) here. It sounds as if you're making the text surround "edit-caret".

[Peng Huang]

(In reply to comment #46)
> (From update of attachment 106175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106175&action=review
> 
> > Source/WebKit/chromium/public/WebWidget.h:153
> > +    virtual bool getTextSurroundEditCaret(WebString&, size_t& focus, size_t& anchor) const { return false; }
> 
> "EditCaret" is inaccurate name because non-collapsed selection isn't a caret. Also you have two verbs (get & surround) here. It sounds as if you're making the text surround "edit-caret".

Several candidates, please choice one.

textAroundEditCaret(), getTextAroundEditCaret(), surroundingTextInEditableElement(), getSurroundingTextInEditableElement().

[Ryosuke Niwa]

(In reply to comment #47)
> Several candidates, please choice one.
> 
> textAroundEditCaret(), getTextAroundEditCaret(), surroundingTextInEditableElement(), getSurroundingTextInEditableElement().

You probably need to include the word selection there because this API also obtains anchor and focus offsets.  Given that, how about getSelectionOffsetsAndTextInEditableContent?

[Peng Huang]

(In reply to comment #48)
> (In reply to comment #47)
> > Several candidates, please choice one.
> > 
> > textAroundEditCaret(), getTextAroundEditCaret(), surroundingTextInEditableElement(), getSurroundingTextInEditableElement().
> 
> You probably need to include the word selection there because this API also obtains anchor and focus offsets.  Given that, how about getSelectionOffsetsAndTextInEditableContent?

OK.

[Peng Huang]

Created attachment 106189 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 106189 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1518
> +    Element* element = selection->rootEditableElement();

I think selection could be null here.

[Peng Huang]

(In reply to comment #51)
> (From update of attachment 106189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106189&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1518
> > +    Element* element = selection->rootEditableElement();
> 
> I think selection could be null here.

Checked the code. It returns &m_selection; So it should not be null.

[WebKit Review Bot]

Comment on attachment 106189 [details]
Patch

Clearing flags on attachment: 106189

Committed r94459: <http://trac.webkit.org/changeset/94459>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Re-opened since this is blocked by 87164

[Ryosuke Niwa]

(In reply to comment #55)
> Re-opened since this is blocked by 87164

Why is this bug blocked by https://bugs.webkit.org/show_bug.cgi?id=87164 ? That one looks totally unrelated.

[Adam Barth]

Sorry.  I typoed 66687