Move TextInputController from CppBoundClass to gin::Wrappable.

Move TextInputController from CppBoundClass to gin::Wrappable.

Also, change the class to use chromium coding style.

BUG=297480, 331301
R=jochen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251692

[tfarina, 2014-02-08 02:28:35]

Jochen, from here I need some help.

[jochen (gone - plz use gerrit), 2014-02-10 12:32:04]

https://codereview.chromium.org/144013010/diff/170002/content/shell/renderer/...
File content/shell/renderer/test_runner/TestInterfaces.h (left):

https://codereview.chromium.org/144013010/diff/170002/content/shell/renderer/...
content/shell/renderer/test_runner/TestInterfaces.h:65:
scoped_ptr<TextInputController> m_textInputController;
i've updated my gamepad controller to use a less intrusive scheme, could you do
the same?

https://codereview.chromium.org/144013010/diff/170002/content/shell/renderer/...
File content/shell/renderer/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/144013010/diff/170002/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:61: void
TextInputController::InsertText(gin::Arguments* args) {
if you declare InsertText(const std::string& text) {
 ...
}

gin will automatically convert the v8 args to string

https://codereview.chromium.org/144013010/diff/170002/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:139: void
TextInputController::FirstRectForCharacterRange(gin::Arguments* args) {
same for the return value. If you return a vector<int>, it'll automatically be
translated to a v8 array

https://codereview.chromium.org/144013010/diff/170002/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:172:
frame_->view()->handleInputEvent(key_down);
you can't know for sure that frame exists at this point. If you restructure the
CL to match my latest cl, you would forward this to a central object that always
has the right frame.

https://codereview.chromium.org/144013010/diff/170002/content/shell/renderer/...
File content/shell/renderer/test_runner/text_input_controller.h (right):

https://codereview.chromium.org/144013010/diff/170002/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.h:19: namespace
WebTestRunner {
while at it, you could update the namespace to content

[tfarina, 2014-02-11 21:43:02]

I think I addressed most of your comments, but not all.

Could you do another pass?

[tfarina, 2014-02-11 21:43:02]

I think I addressed most of your comments, but not all.

Could you do another pass?

[jochen (gone - plz use gerrit), 2014-02-12 09:45:26]

On 2014/02/11 21:43:02, tfarina wrote:
> I think I addressed most of your comments, but not all.
> 
> Could you do another pass?

The comments from last time still apply :)

Essentially, you should have a TextInputController class that does all the work,
and the lifetime of that class is still managed by TestInterfaces.

And a TextInputControllerBindings class, that derives from gin::Wrappable and
just forwards to the TextInputController (to which it holds a weak pointer).

You shouldn't need any method here that takes gin::Arguments* as args, but you
can always specify the concrete arguments you expect, same for the return value.

[tfarina, 2014-02-13 13:05:01]

Hey Jochen,

I made the requested changes. Still have some things commented out and does not
compile yet.

The error is:

In file included from
../../content/shell/renderer/test_runner/text_input_controller.cc:7:
In file included from ../../gin/arguments.h:9:
../../gin/converter.h:181:24: error: no member named 'ToV8' in
'gin::Converter<NPObject *, void>'
  return Converter<T>::ToV8(isolate, input);
         ~~~~~~~~~~~~~~^
../../gin/arguments.h:61:33: note: in instantiation of function template
specialization 'gin::ConvertToV8<NPObject *>' requested here
    info_->GetReturnValue().Set(ConvertToV8(isolate_, val));
                                ^
../../gin/function_template.h:226:11: note: in instantiation of function
template specialization 'gin::Arguments::Return<NPObject *>' requested here
    args->Return(callback.Run(a1));
          ^
../../gin/function_template.h:330:21: note: in instantiation of member function
'gin::internal::Invoker<NPObject *, content::TextInputControllerBindings *,
void, void, void, void, void>::Go' requested here
    Invoker<R, P1>::Go(&args, holder->callback, a1);
                    ^
../../gin/function_template.h:499:35: note: in instantiation of member function
'gin::internal::Dispatcher<NPObject *(content::TextInputControllerBindings
*)>::DispatchToCallback' requested here
      &internal::Dispatcher<Sig>::DispatchToCallback,
                                  ^
../../gin/object_template_builder.h:50:12: note: in instantiation of function
template specialization 'gin::CreateFunctionTemplate<NPObject
*(content::TextInputControllerBindings *)>' requested here
    return CreateFunctionTemplate(isolate, base::Bind(callback),
           ^
../../gin/object_template_builder.h:90:45: note: in instantiation of member
function 'gin::<anonymous namespace>::CallbackTraits<NPObject
*(content::TextInputControllerBindings::*)(), void>::CreateTemplate' requested
here
    return SetImpl(name, CallbackTraits<T>::CreateTemplate(isolate_, callback));
                                            ^
../../content/shell/renderer/test_runner/text_input_controller.cc:91:8: note: in
instantiation of function template specialization
'gin::ObjectTemplateBuilder::SetMethod<NPObject
*(content::TextInputControllerBindings::*)()>' requested here
      .SetMethod("markedRange", &TextInputControllerBindings::MarkedRange)
       ^
1 error generated.
ninja: build stopped: cannot make progress due to previous errors.

[jochen (gone - plz use gerrit), 2014-02-13 13:08:23]

https://codereview.chromium.org/144013010/diff/320001/content/shell/renderer/...
File content/shell/renderer/test_runner/text_input_controller.h (right):

https://codereview.chromium.org/144013010/diff/320001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.h:12: #include
"third_party/WebKit/public/web/WebBindings.h"
don't include this

https://codereview.chromium.org/144013010/diff/320001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.h:39: NPObject*
SelectedRange();
don't return an NPObject here. Return std::vector<int>

[tfarina, 2014-02-13 13:14:06]

https://codereview.chromium.org/144013010/diff/320001/content/shell/renderer/...
File content/shell/renderer/test_runner/text_input_controller.h (right):

https://codereview.chromium.org/144013010/diff/320001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.h:12: #include
"third_party/WebKit/public/web/WebBindings.h"
On 2014/02/13 13:08:24, jochen wrote:
> don't include this

Done.

https://codereview.chromium.org/144013010/diff/320001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.h:39: NPObject*
SelectedRange();
On 2014/02/13 13:08:24, jochen wrote:
> don't return an NPObject here. Return std::vector<int>

Done.

[jochen (gone - plz use gerrit), 2014-02-14 10:01:17]

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
File content/shell/renderer/test_runner/TestInterfaces.cpp (left):

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/TestInterfaces.cpp:48:
m_textInputController->setWebView(0);
please keep this logic to explicitly set the webview instead of storing the
frame_ via Install()

Otherwise, we don't know whether frame_ is still alive

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/TestInterfaces.cpp:54: //
m_textInputController doesn't depend on WebTestDelegate.
please keep those comments (same below)

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
File content/shell/renderer/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:53: // static
this comment is only needed for methods

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:55:
gin::kEmbedderNativeGin
}; should be on this line.

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:84: return
gin::Wrappable<TextInputControllerBindings>::GetObjectTemplateBuilder(isolate)
80c

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:167: /*WebString
web_text(WebString::fromUTF8(text));
why did you comment this out?

[tfarina, 2014-02-14 12:46:12]

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
File content/shell/renderer/test_runner/TestInterfaces.cpp (left):

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/TestInterfaces.cpp:48:
m_textInputController->setWebView(0);
On 2014/02/14 10:01:17, jochen wrote:
> please keep this logic to explicitly set the webview instead of storing the
> frame_ via Install()
> 
> Otherwise, we don't know whether frame_ is still alive

Done.

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/TestInterfaces.cpp:54: //
m_textInputController doesn't depend on WebTestDelegate.
On 2014/02/14 10:01:17, jochen wrote:
> please keep those comments (same below)

Done.

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
File content/shell/renderer/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:53: // static
On 2014/02/14 10:01:17, jochen wrote:
> this comment is only needed for methods

Done.

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:55:
gin::kEmbedderNativeGin
On 2014/02/14 10:01:17, jochen wrote:
> }; should be on this line.

Done.

https://codereview.chromium.org/144013010/diff/420001/content/shell/renderer/...
content/shell/renderer/test_runner/text_input_controller.cc:84: return
gin::Wrappable<TextInputControllerBindings>::GetObjectTemplateBuilder(isolate)
On 2014/02/14 10:01:17, jochen wrote:
> 80c

Done.

[jochen (gone - plz use gerrit), 2014-02-17 14:34:18]

lgtm, thanks

[tfarina, 2014-02-17 16:18:17]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-02-17 16:18:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/144013010/470001

[commit-bot: I haz the power, 2014-02-17 17:51:33]

Change committed as 251692

[tkent, 2014-02-18 00:41:57]

A revert of this CL has been created in
https://codereview.chromium.org/169813004/ by tkent@chromium.org.

The reason for reverting is: Broke
LayoutTests/editing/selection/move-left-right.html, which uses
TextInputController.
.

[tkent, 2014-02-18 00:54:08]

See
http://chromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linux%203...