Adding a WindowAndroidHelper class.

Adding a WindowAndroidHelper class.

Adding a WindowAndroidHelper class that will be used to access
application context and activity for text autofill.

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

[aurimas (slooooooooow), 2012-10-09 00:17:48]

Hello Avi,

I am creating a new helper class that will be used to access WindowAndroid. This
helper is using web_contents.

Could you take a look at this CL?

Aurimas

[aurimas (slooooooooow), 2012-10-09 20:32:38]

Hey Ted,

I'm adding a helper class to get WindowAndroid to be used in autofill move from
internal android chrome directory to chrome/. Could you take a look at this CL?

Aurimas

[Avi (use Gerrit), 2012-10-09 21:30:49]

https://codereview.chromium.org/11098008/diff/1/chrome/browser/ui/android/win...
File chrome/browser/ui/android/window_android_helper.h (right):

https://codereview.chromium.org/11098008/diff/1/chrome/browser/ui/android/win...
chrome/browser/ui/android/window_android_helper.h:8: #include
"chrome/browser/common/web_contents_user_data.h"
This just moved.

#include "content/public/browser/web_contents_user_data.h"

https://codereview.chromium.org/11098008/diff/1/chrome/browser/ui/android/win...
chrome/browser/ui/android/window_android_helper.h:11: class WindowAndroidHelper
: public WebContentsUserData<WindowAndroidHelper> {
content::WebContentsUserData

https://codereview.chromium.org/11098008/diff/1/chrome/browser/ui/android/win...
chrome/browser/ui/android/window_android_helper.h:20: friend class
WebContentsUserData<WindowAndroidHelper>;
content::WebContentsUserData

https://codereview.chromium.org/11098008/diff/1/chrome/browser/ui/tab_content...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

https://codereview.chromium.org/11098008/diff/1/chrome/browser/ui/tab_content...
chrome/browser/ui/tab_contents/tab_contents.cc:160:
WindowAndroidHelper::CreateForWebContents(contents);
No.

First, this is Android only, so why didn't you #ifdef?

Even so, this is the wrong place. The Android code creates TabContentses in
TabAndroid::GetOrCreateTabContents. WindowAndroidHelper is a tab helper that is
only useful for Android. Put it in TabAndroid.

Look at crbug.com/153587 . The people working on that bug are waiting for me to
finish detaching all the tab helpers so that they can make the Android port
create only the ones they need. This tab helper is Android-only. It should lead
the way.

https://codereview.chromium.org/11098008/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11098008/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:2010: 'browser/ui/android/window_android_helper.cc',
Note the lack of other "browser/ui/" files here. These entries belong in
chrome_browser_ui.gypi with their brethren.

[Ted C, 2012-10-09 22:47:54]

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
File chrome/browser/ui/android/window_android_helper.cc (right):

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.cc:10: class WindowAndroid;
why aren't you importing the header instead of forward declaring?

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.cc:22: }
blank line between these two.

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.cc:27:
WindowAndroidHelper::~WindowAndroidHelper() {
the destructor should go above the two methods up above to match the header

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
File chrome/browser/ui/android/window_android_helper.h (right):

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.h:15: void
setWindowAndroid(ui::WindowAndroid* windowAndroid);
c++ style should be SetWindowAndroid (capitalizing the first letter)...same for
GetWindowAndroid

that is unless you want to inline these and then it should be the hacker naming
scheme of:

set_window_android(ui::WindowAndroid* window_android) {
  window_android_ = window_android;
}

Also, just saw and it should be window_android for the variable name.

[aurimas (slooooooooow), 2012-10-10 00:22:13]

Fixed stuff based on avi's and tedchoc's comments.


Nilesh > Could you take a look at tab_android changes?

Aurimas

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
File chrome/browser/ui/android/window_android_helper.cc (right):

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.cc:10: class WindowAndroid;
On 2012/10/09 22:47:54, Ted C wrote:
> why aren't you importing the header instead of forward declaring?

Done.

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.cc:22: }
On 2012/10/09 22:47:54, Ted C wrote:
> blank line between these two.

Done.

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.cc:27:
WindowAndroidHelper::~WindowAndroidHelper() {
On 2012/10/09 22:47:54, Ted C wrote:
> the destructor should go above the two methods up above to match the header

Done.

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
File chrome/browser/ui/android/window_android_helper.h (right):

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.h:8: #include
"chrome/browser/common/web_contents_user_data.h"
On 2012/10/09 21:30:50, Avi wrote:
> This just moved.
> 
> #include "content/public/browser/web_contents_user_data.h"

Done.

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.h:11: class WindowAndroidHelper
: public WebContentsUserData<WindowAndroidHelper> {
On 2012/10/09 21:30:50, Avi wrote:
> content::WebContentsUserData

Done.

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.h:15: void
setWindowAndroid(ui::WindowAndroid* windowAndroid);
On 2012/10/09 22:47:54, Ted C wrote:
> c++ style should be SetWindowAndroid (capitalizing the first letter)...same
for
> GetWindowAndroid
> 
> that is unless you want to inline these and then it should be the hacker
naming
> scheme of:
> 
> set_window_android(ui::WindowAndroid* window_android) {
>   window_android_ = window_android;
> }
> 
> Also, just saw and it should be window_android for the variable name.

Done.

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/window_android_helper.h:20: friend class
WebContentsUserData<WindowAndroidHelper>;
On 2012/10/09 21:30:50, Avi wrote:
> content::WebContentsUserData

Done.

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/browser/ui/tab_...
chrome/browser/ui/tab_contents/tab_contents.cc:160:
WindowAndroidHelper::CreateForWebContents(contents);
On 2012/10/09 21:30:50, Avi wrote:
> No.
> 
> First, this is Android only, so why didn't you #ifdef?
> 
> Even so, this is the wrong place. The Android code creates TabContentses in
> TabAndroid::GetOrCreateTabContents. WindowAndroidHelper is a tab helper that
is
> only useful for Android. Put it in TabAndroid.
> 
> Look at crbug.com/153587 . The people working on that bug are waiting for me
to
> finish detaching all the tab helpers so that they can make the Android port
> create only the ones they need. This tab helper is Android-only. It should
lead
> the way.

Done.

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/11098008/diff/1/chrome/chrome_browser....
chrome/chrome_browser.gypi:2010: 'browser/ui/android/window_android_helper.cc',
On 2012/10/09 21:30:50, Avi wrote:
> Note the lack of other "browser/ui/" files here. These entries belong in
> chrome_browser_ui.gypi with their brethren.

Done.

[nilesh, 2012-10-10 00:28:20]

https://chromiumcodereview.appspot.com/11098008/diff/7001/chrome/browser/andr...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://chromiumcodereview.appspot.com/11098008/diff/7001/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:64:
setWindowAndroid(window_android);
rename this too?

https://chromiumcodereview.appspot.com/11098008/diff/7001/chrome/browser/ui/a...
File chrome/browser/ui/android/window_android_helper.h (right):

https://chromiumcodereview.appspot.com/11098008/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/window_android_helper.h:11: class WindowAndroid;
unindent..

[aurimas (slooooooooow), 2012-10-10 00:42:24]

Fixed nilesh's nits.

https://chromiumcodereview.appspot.com/11098008/diff/7001/chrome/browser/andr...
File chrome/browser/android/tab_base_android_impl.cc (right):

https://chromiumcodereview.appspot.com/11098008/diff/7001/chrome/browser/andr...
chrome/browser/android/tab_base_android_impl.cc:64:
setWindowAndroid(window_android);
On 2012/10/10 00:28:21, nileshagrawal1 wrote:
> rename this too?

Done.

https://chromiumcodereview.appspot.com/11098008/diff/7001/chrome/browser/ui/a...
File chrome/browser/ui/android/window_android_helper.h (right):

https://chromiumcodereview.appspot.com/11098008/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/android/window_android_helper.h:11: class WindowAndroid;
On 2012/10/10 00:28:21, nileshagrawal1 wrote:
> unindent..

Done.

[nilesh, 2012-10-10 00:47:50]

LGTM 

We are planning to use TabAndroid::InitTabHelpers to initialize all needed
helpers for Android.

[Avi (use Gerrit), 2012-10-10 03:58:11]

You don't need my LGTM but you get it anyway. I'm super happy to see the new
infrastructure for the tab helper initialization and look forward to our great
new future.

https://chromiumcodereview.appspot.com/11098008/diff/11001/chrome/browser/and...
File chrome/browser/android/tab_android.cc (right):

https://chromiumcodereview.appspot.com/11098008/diff/11001/chrome/browser/and...
chrome/browser/android/tab_android.cc:21: void
TabAndroid::InitTabHelpers(content::WebContents* web_contents) {
Whoo hoo!

Awesome.

[aurimas (slooooooooow), 2012-10-10 17:34:44]

Hello Yaron,

Could you take a look at chrome/browser/ui/android/ changes?

Aurimas

[Ted C, 2012-10-10 17:41:20]

lgtm w/ nit

https://chromiumcodereview.appspot.com/11098008/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/android/window_android_helper.h (right):

https://chromiumcodereview.appspot.com/11098008/diff/11001/chrome/browser/ui/...
chrome/browser/ui/android/window_android_helper.h:15: : public
content::WebContentsUserData<WindowAndroidHelper> {
+2 indent

[aurimas (slooooooooow), 2012-10-10 18:07:47]

https://chromiumcodereview.appspot.com/11098008/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/android/window_android_helper.h (right):

https://chromiumcodereview.appspot.com/11098008/diff/11001/chrome/browser/ui/...
chrome/browser/ui/android/window_android_helper.h:15: : public
content::WebContentsUserData<WindowAndroidHelper> {
On 2012/10/10 17:41:21, Ted C wrote:
> +2 indent

Done.

[Yaron, 2012-10-10 18:16:49]

lgtm % nit

https://codereview.chromium.org/11098008/diff/9004/chrome/browser/ui/android/...
File chrome/browser/ui/android/window_android_helper.h (right):

https://codereview.chromium.org/11098008/diff/9004/chrome/browser/ui/android/...
chrome/browser/ui/android/window_android_helper.h:14: class WindowAndroidHelper
Class-level comment.

[aurimas (slooooooooow), 2012-10-10 18:54:50]

Hello Nico,

Could you take a look at this CL? I got LGTMs from all the subdirectories
already.

Thanks,
Aurimas

https://codereview.chromium.org/11098008/diff/9004/chrome/browser/ui/android/...
File chrome/browser/ui/android/window_android_helper.h (right):

https://codereview.chromium.org/11098008/diff/9004/chrome/browser/ui/android/...
chrome/browser/ui/android/window_android_helper.h:14: class WindowAndroidHelper
On 2012/10/10 18:16:49, Yaron wrote:
> Class-level comment.

Done.

[aurimas (slooooooooow), 2012-10-10 19:57:38]

Hello Brett,

Could you take a look at my CL? I got LGTMs from all the subdirectories and
thakis is on holiday.

Thanks,
Aurimas

[brettw, 2012-10-10 22:00:47]

chrome owners lgtm, I didn't check the details.

[commit-bot: I haz the power, 2012-10-10 23:48:05]

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

[commit-bot: I haz the power, 2012-10-11 02:01:40]

Change committed as 161263