Bug 38100 – [chromium] Implement TextInputController::firstRectForCharacterRange

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Yuzo Fujishima]

The method is unimplemented as of r 58231.

As a result,
editing/inserting/caret-position.html
fails.

[Yuzo Fujishima]

The test failure above started at r58191.

[Yuzo Fujishima]

Committed r58232: <http://trac.webkit.org/changeset/58232>

[Yuzo Fujishima]

Just changed the test expectation.
The issue remains.

[Yuzo Fujishima]

Kent-san, can you take a look?

[Ryosuke Niwa]

I've been able to implement the feature but I haven't been able to figure out how to create an array of ints in DRT / test shell to pass it back to V8.  Does anyone know how to create an array of ints to CppVariant?

Index: src/WebFrameImpl.cpp
===================================================================
--- src/WebFrameImpl.cpp        (revision 67748)
+++ src/WebFrameImpl.cpp        (working copy)
@@ -1071,6 +1071,20 @@
     return frame()->editor()->compositionRange();
 }

+WebRect WebFrameImpl::firstRectForCharacterRange(unsigned location, unsigned length) const
+{
+    if ((location + length < location) && (location + length))
+        length = 0;
+
+    Element* selectionRoot = frame()->selection()->rootEditableElement();
+    Element* scope = selectionRoot ? selectionRoot : frame()->document()->documentElement();
+    RefPtr<Range> range = TextIterator::rangeFromLocationAndLength(scope, location, length);
+    if (!range)
+        return WebRect();
+    IntRect rect = frame()->editor()->firstRectForRange(range.get());
+    return WebRect(rect.x(), rect.y(), rect.width(), rect.height());
+}
+
 bool WebFrameImpl::executeCommand(const WebString& name)
 {
     ASSERT(frame());
Index: src/WebFrameImpl.h
===================================================================
--- src/WebFrameImpl.h  (revision 67748)
+++ src/WebFrameImpl.h  (working copy)
@@ -135,6 +135,7 @@
     virtual void unmarkText();
     virtual bool hasMarkedText() const;
     virtual WebRange markedRange() const;
+    virtual WebRect firstRectForCharacterRange(unsigned location, unsigned length) const;
     virtual bool executeCommand(const WebString&);
     virtual bool executeCommand(const WebString&, const WebString& value);
     virtual bool isCommandEnabled(const WebString&) const;
Index: public/WebFrame.h
===================================================================
--- public/WebFrame.h   (revision 67748)
+++ public/WebFrame.h   (working copy)
@@ -352,6 +352,8 @@

     virtual WebRange markedRange() const = 0;

+    virtual WebRect firstRectForCharacterRange(unsigned location, unsigned length) const = 0;
+
     // Supports commands like Undo, Redo, Cut, Copy, Paste, SelectAll,
     // Unselect, etc. See EditorCommand.cpp for the full list of supported
     // commands.


Index: chromium/TextInputController.cpp
===================================================================
--- chromium/TextInputController.cpp    (revision 67748)
+++ chromium/TextInputController.cpp    (working copy)
@@ -186,10 +186,21 @@
     result->set(string(buffer));
 }

-void TextInputController::firstRectForCharacterRange(const CppArgumentList&, CppVariant* result)
+void TextInputController::firstRectForCharacterRange(const CppArgumentList& arguments, CppVariant* result)
 {
-    // FIXME: Implement this.
     result->setNull();
+
+    if (arguments.size() < 2 || !arguments[0].isNumber() || !arguments[1].isNumber())
+        return;
+
+    WebFrame* mainFrame = getMainFrame();
+    WebRect rect = mainFrame->firstRectForCharacterRange(arguments[0].toInt32(), arguments[1].toInt32());
+    if (rect.x == 0 && rect.y == 0 && rect.width ==0 && rect.height == 0)
+        return;
+
+    std::string rect_str;
+    SStringPrintf(&rect_str, "%d,%d,%d,%d", rect.x, rect.y, rect.width, rect.height);
+    result->Set(rect_str);
 }

[Kent Tamura]

(In reply to comment #5)

Niwa-san, thank you for taking look at this.

We need to construct an object via NPAPI.  WebKitTools/DumpRenderTree/chromium/CppVariant.{cpp,h} and WebKit/chromium/src/WebBindings.cpp are helpful.

[Ryosuke Niwa]

Created attachment 68291 [details]
work in progress

Here's my work in progress.  I ended up adding makeIntArray to WebBindings where I instantiate v8::array and create a NP object for it.

[Ryosuke Niwa]

Created attachment 68600 [details]
Patch

[Ryosuke Niwa]

Hi Kent, I'm taking this bug from you.   I hope you don't mind.

[Tony Chang]

Comment on attachment 68600 [details]
Patch

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

> WebKit/chromium/ChangeLog:11
> +        Because the function needs to return an array of integers, added makeIntArray to WebBindings.
> +        makeIntArray does not take Vector because WebBindings.h is included in plugin glue code
> +        where it cannot find wtf/Vector.h.

Can you use WebVector.h instead?

[Ryosuke Niwa]

(In reply to comment #10)
> (From update of attachment 68600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68600&action=review
> 
> > WebKit/chromium/ChangeLog:11
> > +        Because the function needs to return an array of integers, added makeIntArray to WebBindings.
> > +        makeIntArray does not take Vector because WebBindings.h is included in plugin glue code
> > +        where it cannot find wtf/Vector.h.
> 
> Can you use WebVector.h instead?

Ah! I wasn't aware of this class. Yes, I can make use of this class.  Thanks for the info.  I'm uploading a new patch now.

[Ryosuke Niwa]

Created attachment 68607 [details]
Patch

[Tony Chang]

Comment on attachment 68607 [details]
Patch

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

> WebKit/chromium/src/WebBindings.cpp:319
> +    for (size_t i = data.size(); i > 0; i--)
> +        result->Set(v8::Number::New(i-1), v8::Number::New(data[i-1]));

Nit: Spaces around the minus sign (e.g., New(i - 1)).  Why not just have the loop go from 0 to < size() so you don't have to do the minus 1?

> LayoutTests/platform/chromium/test_expectations.txt:968
> +BUGWK38100 WIN LINUX MAC : editing/inserting/caret-position.html = TEXT PASS

Nit: Just remove the platform names here.  No platforms means it applies to all platforms.

[anton muhin]

Comment on attachment 68607 [details]
Patch

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

Drive by notes

> WebKit/chromium/src/WebBindings.cpp:316
> +{

As you keep a handle to v8 object, it might be a good idea to create a handle scope here.  Do you really need it or not depends on if callers create those scopes themselves.

>> WebKit/chromium/src/WebBindings.cpp:319
>> +        result->Set(v8::Number::New(i-1), v8::Number::New(data[i-1]));
> 
> Nit: Spaces around the minus sign (e.g., New(i - 1)).  Why not just have the loop go from 0 to < size() so you don't have to do the minus 1?

You might prefer Object::Set(uint32_t, Handle<Value>) overload which would allow you to write it shorter

> WebKit/chromium/src/WebBindings.cpp:322
> +    return npCreateV8ScriptObject(0, v8::Handle<v8::Object>::Cast(result), window);

Do you really need to cast result which is already v8::Handle<v8::Array> to v8::Handle<v8::Object>?

[Ryosuke Niwa]

(In reply to comment #14)
> (From update of attachment 68607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68607&action=review
> 
> Drive by notes
> 
> > WebKit/chromium/src/WebBindings.cpp:316
> > +{
> 
> As you keep a handle to v8 object, it might be a good idea to create a handle scope here.  Do you really need it or not depends on if callers create those scopes themselves.

But I'm returning this object as a return value to TextInputController::firstRectForCharacterRange.  Do I still want to set the scope then?

> >> WebKit/chromium/src/WebBindings.cpp:319
> >> +        result->Set(v8::Number::New(i-1), v8::Number::New(data[i-1]));
> > 
> > Nit: Spaces around the minus sign (e.g., New(i - 1)).  Why not just have the loop go from 0 to < size() so you don't have to do the minus 1?
> 
> You might prefer Object::Set(uint32_t, Handle<Value>) overload which would allow you to write it shorter

Will do.

> > WebKit/chromium/src/WebBindings.cpp:322
> > +    return npCreateV8ScriptObject(0, v8::Handle<v8::Object>::Cast(result), window);
> 
> Do you really need to cast result which is already v8::Handle<v8::Array> to v8::Handle<v8::Object>?

Huh, I guess that's not needed.  I think I copy&pasted the code and forgot to remove that.  Thanks for pointing that out.

[Ryosuke Niwa]

Created attachment 68748 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 68748 [details]
Patch for landing

Clearing flags on attachment: 68748

Committed r68312: <http://trac.webkit.org/changeset/68312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.