Upstream TabAndroid abstract class.

Upstream TabAndroid abstract class.

Together with TabAndroid I am also upstreaming part of the
chrome_web_contents_view_delegate_android.cc so that we can include
tab_android.h and compile it.
I am using the target "browser" to check that it compiles.

BUG=136075, 137008
TEST=compiled with "make browser"


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

[Lei Zhang, 2012-07-18 18:07:43]

Do you need to add tab_android* to the .gypi file?

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
File chrome/browser/android/tab_android.h (right):

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:13: #include "base/memory/ref_counted.h"
nit: Remove unneeded includes, such as both base/memory headers and
content/public/browser/notification_registrar.h.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:19: #include "googleurl/src/gurl.h"
nit: forward declare instead.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:20: #include
"third_party/skia/include/core/SkBitmap.h"
nit: Probably not needed since you forward decl'd SkBitmap below.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:22: class AutofillExternalDelegateAndroid;
nit: Please remove unneeded forward decls. Ditto for Profile, TabContents, etc.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:41: TabAndroid() : tab_id_(-1) {}
Please move ctor/dtor impl into the .cc file.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/ui/a...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:13:
class ContextMenuParams;
nit: no space in front of "class"

[felipeg, 2012-07-19 10:15:36]

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
File chrome/browser/android/tab_android.h (right):

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:13: #include "base/memory/ref_counted.h"
On 2012/07/18 18:07:43, Lei Zhang wrote:
> nit: Remove unneeded includes, such as both base/memory headers and
> content/public/browser/notification_registrar.h.

Done.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:19: #include "googleurl/src/gurl.h"
On 2012/07/18 18:07:43, Lei Zhang wrote:
> nit: forward declare instead.

Done.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:20: #include
"third_party/skia/include/core/SkBitmap.h"
On 2012/07/18 18:07:43, Lei Zhang wrote:
> nit: Probably not needed since you forward decl'd SkBitmap below.

Done.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:22: class AutofillExternalDelegateAndroid;
On 2012/07/18 18:07:43, Lei Zhang wrote:
> nit: Please remove unneeded forward decls. Ditto for Profile, TabContents,
etc.

Done.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/andr...
chrome/browser/android/tab_android.h:41: TabAndroid() : tab_id_(-1) {}
On 2012/07/18 18:07:43, Lei Zhang wrote:
> Please move ctor/dtor impl into the .cc file.

Done.

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/ui/a...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/10803010/diff/6001/chrome/browser/ui/a...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:13:
class ContextMenuParams;
On 2012/07/18 18:07:43, Lei Zhang wrote:
> nit: no space in front of "class"

Done.

[Lei Zhang, 2012-07-19 18:45:03]

LGTM, with a nit below, and with a LG from someone from the Chrome Android team.

https://chromiumcodereview.appspot.com/10803010/diff/6003/chrome/browser/ui/a...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/10803010/diff/6003/chrome/browser/ui/a...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:12:
namespace content {
This forward decl is probably not needed.

[felipeg, 2012-07-19 18:54:06]

Hey Yaron or Jay, could you take a look at this change ? thanks

https://chromiumcodereview.appspot.com/10803010/diff/6003/chrome/browser/ui/a...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/10803010/diff/6003/chrome/browser/ui/a...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:12:
namespace content {
On 2012/07/19 18:45:03, Lei Zhang wrote:
> This forward decl is probably not needed.

Done.

[Yaron, 2012-07-19 22:21:28]

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

https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
chrome/browser/android/tab_android_stubs.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: rename tab_android_test_stubs.cc.
It took me a while to see why we need this. Also, you need to add it to the
chrome_tests.gypi

[Yaron, 2012-07-19 22:24:56]

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

https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
chrome/browser/android/tab_android.h:28: class TabAndroid : public
content::NotificationObserver {
This doesn't actually implement Observe. Can you move to the impl class?

https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
chrome/browser/android/tab_android.h:65: virtual void PromoSendEmail(const
string16& data_email,
Also, we shouldn't really have this. I think Ted file a bug about removing it.
Please discuss with him.

[felipeg, 2012-07-20 09:39:09]

Thanks Yaron,
I am adding Ted to the review too.
Ted, can you take a look at Yaron comment's and my reply below, about the
PromoSendEmail ?

Cheers,
Felipeg

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

https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
chrome/browser/android/tab_android.h:28: class TabAndroid : public
content::NotificationObserver {
On 2012/07/19 22:24:56, Yaron wrote:
> This doesn't actually implement Observe. Can you move to the impl class?

Done.

https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
chrome/browser/android/tab_android.h:65: virtual void PromoSendEmail(const
string16& data_email,
On 2012/07/19 22:24:56, Yaron wrote:
> Also, we shouldn't really have this. I think Ted file a bug about removing it.
> Please discuss with him.

Hi Yaron,
Ted is not online now, I am adding him to the CL.
The only reference I found in crbug or buganizer was:
http://crbug.com/136789
where Ted is cc'ed

Therefore I created a new bug: http://crbug.com/138216
And added a comment here.

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

https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
chrome/browser/android/tab_android_stubs.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/07/19 22:21:28, Yaron wrote:
> nit: rename tab_android_test_stubs.cc.
> It took me a while to see why we need this. Also, you need to add it to the
> chrome_tests.gypi

Done.

[Yaron, 2012-07-20 16:49:27]

lgtm mod nit. I'm fine with the TODO you added

https://chromiumcodereview.appspot.com/10803010/diff/14001/chrome/browser/and...
File chrome/browser/android/tab_android.h (right):

https://chromiumcodereview.appspot.com/10803010/diff/14001/chrome/browser/and...
chrome/browser/android/tab_android.h:37: virtual
browser_sync::SyncedTabDelegate* synced_tab_delegate() = 0;
virtual shouldn't use hacker_style. Rename GetSyncedTabDelegate

[Ted C, 2012-07-20 17:22:24]

On 2012/07/20 09:39:09, felipeg1 wrote:
> Thanks Yaron,
> I am adding Ted to the review too.
> Ted, can you take a look at Yaron comment's and my reply below, about the
> PromoSendEmail ?
> 
> Cheers,
> Felipeg
> 
>
https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
> File chrome/browser/android/tab_android.h (right):
> 
>
https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
> chrome/browser/android/tab_android.h:28: class TabAndroid : public
> content::NotificationObserver {
> On 2012/07/19 22:24:56, Yaron wrote:
> > This doesn't actually implement Observe. Can you move to the impl class?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
> chrome/browser/android/tab_android.h:65: virtual void PromoSendEmail(const
> string16& data_email,
> On 2012/07/19 22:24:56, Yaron wrote:
> > Also, we shouldn't really have this. I think Ted file a bug about removing
it.
> > Please discuss with him.
> 
> Hi Yaron,
> Ted is not online now, I am adding him to the CL.
> The only reference I found in crbug or buganizer was:
> http://crbug.com/136789
> where Ted is cc'ed
> 
> Therefore I created a new bug: http://crbug.com/138216
> And added a comment here.
> 

Cool thanks, I'll try and get rid of this as soon as this CL lands.

>
https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
> File chrome/browser/android/tab_android_stubs.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10803010/diff/11001/chrome/browser/and...
> chrome/browser/android/tab_android_stubs.cc:1: // Copyright (c) 2012 The
> Chromium Authors. All rights reserved.
> On 2012/07/19 22:21:28, Yaron wrote:
> > nit: rename tab_android_test_stubs.cc.
> > It took me a while to see why we need this. Also, you need to add it to the
> > chrome_tests.gypi
> 
> Done.

[Ted C, 2012-07-20 17:23:11]

lgtm

[felipeg, 2012-07-23 12:19:59]

Thanks guys,
Landing now.

https://chromiumcodereview.appspot.com/10803010/diff/14001/chrome/browser/and...
File chrome/browser/android/tab_android.h (right):

https://chromiumcodereview.appspot.com/10803010/diff/14001/chrome/browser/and...
chrome/browser/android/tab_android.h:37: virtual
browser_sync::SyncedTabDelegate* synced_tab_delegate() = 0;
On 2012/07/20 16:49:28, Yaron wrote:
> virtual shouldn't use hacker_style. Rename GetSyncedTabDelegate

Done.

[commit-bot: I haz the power, 2012-07-23 12:20:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felipeg@chromium.org/10803010/16001

[commit-bot: I haz the power, 2012-07-23 13:34:43]

Change committed as 147849