Upstreaming Select Action

Upstreaming Select Action

Upstreaming the select action functionality for Android.

BUG=139111
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149329

[aurimas (slooooooooow), 2012-07-31 16:50:35]

Hello Yaron,

I am upstreaming Select Action functionality. Could you take a look?

Thanks,
Aurimas

[Yaron, 2012-07-31 17:09:42]

I'm not the best person to look at the IME stuff. Adding Oli/Min

[olilan, 2012-07-31 18:24:08]

lgtm with a couple of nits

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.h (right):

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.h:24: // For more information,
see http://b/issue?id=3248163
For what you've added in this CL this isn't really true. Maybe better to not
include this comment for now. Also better to not include references to http://b/

https://chromiumcodereview.appspot.com/10828098/diff/1/content/common/view_me...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10828098/diff/1/content/common/view_me...
content/common/view_messages.h:977: #if defined(OS_ANDROID)
I don't think this needs to be OS_ANDROID only.

[qinmin, 2012-07-31 19:22:48]

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:23:
content::RenderWidgetHostViewAndroid* rwhva)
ditto

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:55:
content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From(
ditto

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:73:
content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From(
ditto

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.h (right):

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.h:25: class ImeAdapterAndroid
{
add content namespace here

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.h:27: explicit
ImeAdapterAndroid(content::RenderWidgetHostViewAndroid* rwhva);
no content:: if ImeAdapterAndroid is in content namespace

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.h:40:
content::RenderWidgetHostViewAndroid* rwhva_;
ditto

[aurimas (slooooooooow), 2012-07-31 21:12:14]

Hey guys,

I fixed Oli's and Min's nits.

Aurimas

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:23:
content::RenderWidgetHostViewAndroid* rwhva)
On 2012/07/31 19:22:48, qinmin wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:55:
content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From(
On 2012/07/31 19:22:48, qinmin wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:73:
content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From(
On 2012/07/31 19:22:48, qinmin wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.h (right):

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.h:24: // For more information,
see http://b/issue?id=3248163
On 2012/07/31 18:24:08, olilan wrote:
> For what you've added in this CL this isn't really true. Maybe better to not
> include this comment for now. Also better to not include references to
http://b/

Done.

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.h:25: class ImeAdapterAndroid
{
On 2012/07/31 19:22:48, qinmin wrote:
> add content namespace here

Done.

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.h:27: explicit
ImeAdapterAndroid(content::RenderWidgetHostViewAndroid* rwhva);
On 2012/07/31 19:22:48, qinmin wrote:
> no content:: if ImeAdapterAndroid is in content namespace

Done.

https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
content/browser/renderer_host/ime_adapter_android.h:40:
content::RenderWidgetHostViewAndroid* rwhva_;
On 2012/07/31 19:22:48, qinmin wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10828098/diff/1/content/common/view_me...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10828098/diff/1/content/common/view_me...
content/common/view_messages.h:977: #if defined(OS_ANDROID)
On 2012/07/31 18:24:08, olilan wrote:
> I don't think this needs to be OS_ANDROID only.

Done.

[qinmin, 2012-07-31 21:16:10]

lgtm, you need approval from content OWNERs

On 2012/07/31 21:12:14, aurimas1 wrote:
> Hey guys,
> 
> I fixed Oli's and Min's nits.
> 
> Aurimas
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
> File content/browser/renderer_host/ime_adapter_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
> content/browser/renderer_host/ime_adapter_android.cc:23:
> content::RenderWidgetHostViewAndroid* rwhva)
> On 2012/07/31 19:22:48, qinmin wrote:
> > ditto
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
> content/browser/renderer_host/ime_adapter_android.cc:55:
> content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From(
> On 2012/07/31 19:22:48, qinmin wrote:
> > ditto
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
> content/browser/renderer_host/ime_adapter_android.cc:73:
> content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From(
> On 2012/07/31 19:22:48, qinmin wrote:
> > ditto
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
> File content/browser/renderer_host/ime_adapter_android.h (right):
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
> content/browser/renderer_host/ime_adapter_android.h:24: // For more
information,
> see http://b/issue?id=3248163
> On 2012/07/31 18:24:08, olilan wrote:
> > For what you've added in this CL this isn't really true. Maybe better to not
> > include this comment for now. Also better to not include references to
> http://b/
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
> content/browser/renderer_host/ime_adapter_android.h:25: class
ImeAdapterAndroid
> {
> On 2012/07/31 19:22:48, qinmin wrote:
> > add content namespace here
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
> content/browser/renderer_host/ime_adapter_android.h:27: explicit
> ImeAdapterAndroid(content::RenderWidgetHostViewAndroid* rwhva);
> On 2012/07/31 19:22:48, qinmin wrote:
> > no content:: if ImeAdapterAndroid is in content namespace
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/browser/render...
> content/browser/renderer_host/ime_adapter_android.h:40:
> content::RenderWidgetHostViewAndroid* rwhva_;
> On 2012/07/31 19:22:48, qinmin wrote:
> > ditto
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/common/view_me...
> File content/common/view_messages.h (right):
> 
>
https://chromiumcodereview.appspot.com/10828098/diff/1/content/common/view_me...
> content/common/view_messages.h:977: #if defined(OS_ANDROID)
> On 2012/07/31 18:24:08, olilan wrote:
> > I don't think this needs to be OS_ANDROID only.
> 
> Done.

[aurimas (slooooooooow), 2012-07-31 21:20:39]

Hello Brett,

I got LGTMs from two reviewers so far. Would you mind looking this CL over so I
could submit it?

Thanks,
Aurimas

[brettw, 2012-07-31 21:41:53]

LGTM, just some style nits. I didn't look at the java code.

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.cc:17: if
(!RegisterNativesImpl(env)) {
Be consistent about {} usage for single-line conditionals (you generally don't
use them below).

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.cc:82: } // namespace content
Two spaces before //

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
File content/browser/renderer_host/ime_adapter_android.h (right):

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.h:10: namespace content {
Blank line after this.

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.h:36: bool
RegisterImeAdapter(JNIEnv* env);
Why not have this as a static function in the class?

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.h:38: } // namespace content
Two spaces before end-of-line comment.

[aurimas (slooooooooow), 2012-07-31 21:58:38]

Hey Brett,

I fixed up the nits.

Aurimas

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.cc:17: if
(!RegisterNativesImpl(env)) {
On 2012/07/31 21:41:53, brettw wrote:
> Be consistent about {} usage for single-line conditionals (you generally don't
> use them below).

Done.

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.cc:82: } // namespace content
On 2012/07/31 21:41:53, brettw wrote:
> Two spaces before //

Done.

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
File content/browser/renderer_host/ime_adapter_android.h (right):

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.h:10: namespace content {
On 2012/07/31 21:41:53, brettw wrote:
> Blank line after this.

Done.

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.h:36: bool
RegisterImeAdapter(JNIEnv* env);
On 2012/07/31 21:41:53, brettw wrote:
> Why not have this as a static function in the class?

Done.

https://chromiumcodereview.appspot.com/10828098/diff/8004/content/browser/ren...
content/browser/renderer_host/ime_adapter_android.h:38: } // namespace content
On 2012/07/31 21:41:53, brettw wrote:
> Two spaces before end-of-line comment.

Done.

[commit-bot: I haz the power, 2012-07-31 23:04:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/10828098/6004

[commit-bot: I haz the power, 2012-08-01 00:24:14]

Change committed as 149329