Straighten up life cycle of native InfoBar pointers

Straighten up life cycle of native InfoBar pointers

Currently, there are two issues for potential crashes.

1. There is a chance that Java InfoBars can access native counterparts
even after they are destroyed.

mNativeInfoBarPtr is never reset even after native InfoBar is destroyed.
We do not accept touch inputs after closing infobars, but there may be
corner cases where mNativeInfoBarPtr is accessed afterwards.

2. Infobars aren't setting "base" native pointers correctly.

InfoBar_jni.h has the following code:

InfoBarAndroid* native =
    reinterpret_cast<InfoBarAndroid*>(nativeInfoBarAndroid);

Here, nativeInfoBarAndroid refers to the subclass, so we implicitly upcast
the pointer using reinterpret_cast<>, which may result in pointing to
an incorrect pointer depending on the architecture.

In order to prevent any such occurrence in the future,
this also prevents InfoBar Java subclasses from accessing
the native base pointer and functions.

BUG=492777, 481758
Committed: https://crrev.com/6e63752fcab8553a3b84c5ecf6a356240dacb6e4
Cr-Commit-Position: refs/heads/master@{#333015}

[Changwan Ryu, 2015-06-01 03:10:05]

changwan@chromium.org changed reviewers:
+ cjhopman@chromium.org, dtrainor@chromium.org, miguelg@chromium.org,
tedchoc@chromium.org

[Peter Kasting, 2015-06-01 06:43:16]

Based on the CL description, why is the code using reinterpret_cast<> instead of
static_cast<>, which should adjust the pointer base correctly?

[Changwan Ryu, 2015-06-01 07:06:34]

On 2015/06/01 06:43:16, Peter Kasting wrote:
> Based on the CL description, why is the code using reinterpret_cast<> instead
of
> static_cast<>, which should adjust the pointer base correctly?

Since Java keeps track of a pointer value as long, we still reinterpret_cast in
two places:
1) Before sending pointer value to Java
2) Before calling native call from Java

The latter happens inside automatically generated code (*_jni.h) which just
blindly reinterpret
long value to a matching class pointer. So the generated code does not have full
context
of the original class and how it should apply cast into another class.

Maybe we could create a wrapper for the long type
to keep track of the class type of the native pointer, and apply static_cast<>
automatically,
but I don't have a concrete idea for that.

[Peter Kasting, 2015-06-01 07:18:24]

On 2015/06/01 07:06:34, Changwan Ryu wrote:
> On 2015/06/01 06:43:16, Peter Kasting wrote:
> > Based on the CL description, why is the code using reinterpret_cast<>
instead
> of
> > static_cast<>, which should adjust the pointer base correctly?
> 
> Since Java keeps track of a pointer value as long, we still reinterpret_cast
in
> two places:
> 1) Before sending pointer value to Java
> 2) Before calling native call from Java
> 
> The latter happens inside automatically generated code (*_jni.h) which just
> blindly reinterpret
> long value to a matching class pointer. So the generated code does not have
full
> context
> of the original class and how it should apply cast into another class.
> 
> Maybe we could create a wrapper for the long type
> to keep track of the class type of the native pointer, and apply static_cast<>
> automatically,
> but I don't have a concrete idea for that.

Well, I can't tell you specifically what to do without deeply understanding your
system, but it sounds like perhaps you should only store base-class-type
pointers in these auto-converted fields to begin with.  In other words, on the
C++ side, cast to InfoBarAndroid* before stuffing the pointer value into this
field.  Then you know your autogenerated reinterpret_cast<>s will never be
converting across inheritance levels.

[Changwan Ryu, 2015-06-01 08:32:47]

On 2015/06/01 07:18:24, Peter Kasting wrote:
> On 2015/06/01 07:06:34, Changwan Ryu wrote:
> > On 2015/06/01 06:43:16, Peter Kasting wrote:
> > > Based on the CL description, why is the code using reinterpret_cast<>
> instead
> > of
> > > static_cast<>, which should adjust the pointer base correctly?
> > 
> > Since Java keeps track of a pointer value as long, we still reinterpret_cast
> in
> > two places:
> > 1) Before sending pointer value to Java
> > 2) Before calling native call from Java
> > 
> > The latter happens inside automatically generated code (*_jni.h) which just
> > blindly reinterpret
> > long value to a matching class pointer. So the generated code does not have
> full
> > context
> > of the original class and how it should apply cast into another class.
> > 
> > Maybe we could create a wrapper for the long type
> > to keep track of the class type of the native pointer, and apply
static_cast<>
> > automatically,
> > but I don't have a concrete idea for that.
> 
> Well, I can't tell you specifically what to do without deeply understanding
your
> system, but it sounds like perhaps you should only store base-class-type
> pointers in these auto-converted fields to begin with.  In other words, on the
> C++ side, cast to InfoBarAndroid* before stuffing the pointer value into this
> field.  Then you know your autogenerated reinterpret_cast<>s will never be
> converting across inheritance levels.

I believe that's close to what this CL is about. "this" has been replaced by
"infobar_android()" in many places when making calls to the Java counterpart.
We still need the subclass pointers in other places, though.

[David Trainor- moved to gerrit, 2015-06-01 17:29:58]

On 2015/06/01 08:32:47, Changwan Ryu wrote:
> On 2015/06/01 07:18:24, Peter Kasting wrote:
> > On 2015/06/01 07:06:34, Changwan Ryu wrote:
> > > On 2015/06/01 06:43:16, Peter Kasting wrote:
> > > > Based on the CL description, why is the code using reinterpret_cast<>
> > instead
> > > of
> > > > static_cast<>, which should adjust the pointer base correctly?
> > > 
> > > Since Java keeps track of a pointer value as long, we still
reinterpret_cast
> > in
> > > two places:
> > > 1) Before sending pointer value to Java
> > > 2) Before calling native call from Java
> > > 
> > > The latter happens inside automatically generated code (*_jni.h) which
just
> > > blindly reinterpret
> > > long value to a matching class pointer. So the generated code does not
have
> > full
> > > context
> > > of the original class and how it should apply cast into another class.
> > > 
> > > Maybe we could create a wrapper for the long type
> > > to keep track of the class type of the native pointer, and apply
> static_cast<>
> > > automatically,
> > > but I don't have a concrete idea for that.
> > 
> > Well, I can't tell you specifically what to do without deeply understanding
> your
> > system, but it sounds like perhaps you should only store base-class-type
> > pointers in these auto-converted fields to begin with.  In other words, on
the
> > C++ side, cast to InfoBarAndroid* before stuffing the pointer value into
this
> > field.  Then you know your autogenerated reinterpret_cast<>s will never be
> > converting across inheritance levels.
> 
> I believe that's close to what this CL is about. "this" has been replaced by
> "infobar_android()" in many places when making calls to the Java counterpart.
> We still need the subclass pointers in other places, though.

I'm still a little confused on the overall design of the Java <-> Native
interaction.  I think it would be a lot cleaner to hide the base class native
pointer from the child classes completely to prevent this from happening.  A few
suggestions on the design that might help:

1. Hide the setting/clearing of the native base pointer from the child class
completely (see Tab.java and ChromeTab.java).  Can the InfoBarAndroid native
base class have some kind of setup method that calls the Java private method
setNativePtr() on the base Java class?  That way the base class InfoBarAndroid
is responsible for setting up the base class InfoBar in Java.  Child
InfoBarAndroid classes could override it and do additional setup as well.

2. It looks like we have some hybrid approach to creating the Java infobar
classes (either through some delegate method or through some static method on
the Infobar subclass).  Could we be consistent about this?  I think it would
help us come up with a design that minimizes exposing so many native pointers. 
Additionally, we also have a hybrid approach for handling interaction with the
delegate.  Sometimes we build this Java version of a delegate class and
sometimes we just hit the InfoBarAndroid class directly which pokes at the
delegate anyway.  We should be consistent for all of our cross-boundary designs
for this feature IMO.

wdyt?

[Changwan Ryu, 2015-06-02 08:52:59]

On 2015/06/01 17:29:58, David Trainor wrote:
> On 2015/06/01 08:32:47, Changwan Ryu wrote:
> > On 2015/06/01 07:18:24, Peter Kasting wrote:
> > > On 2015/06/01 07:06:34, Changwan Ryu wrote:
> > > > On 2015/06/01 06:43:16, Peter Kasting wrote:
> > > > > Based on the CL description, why is the code using reinterpret_cast<>
> > > instead
> > > > of
> > > > > static_cast<>, which should adjust the pointer base correctly?
> > > > 
> > > > Since Java keeps track of a pointer value as long, we still
> reinterpret_cast
> > > in
> > > > two places:
> > > > 1) Before sending pointer value to Java
> > > > 2) Before calling native call from Java
> > > > 
> > > > The latter happens inside automatically generated code (*_jni.h) which
> just
> > > > blindly reinterpret
> > > > long value to a matching class pointer. So the generated code does not
> have
> > > full
> > > > context
> > > > of the original class and how it should apply cast into another class.
> > > > 
> > > > Maybe we could create a wrapper for the long type
> > > > to keep track of the class type of the native pointer, and apply
> > static_cast<>
> > > > automatically,
> > > > but I don't have a concrete idea for that.
> > > 
> > > Well, I can't tell you specifically what to do without deeply
understanding
> > your
> > > system, but it sounds like perhaps you should only store base-class-type
> > > pointers in these auto-converted fields to begin with.  In other words, on
> the
> > > C++ side, cast to InfoBarAndroid* before stuffing the pointer value into
> this
> > > field.  Then you know your autogenerated reinterpret_cast<>s will never be
> > > converting across inheritance levels.
> > 
> > I believe that's close to what this CL is about. "this" has been replaced by
> > "infobar_android()" in many places when making calls to the Java
counterpart.
> > We still need the subclass pointers in other places, though.
> 
> I'm still a little confused on the overall design of the Java <-> Native
> interaction.  I think it would be a lot cleaner to hide the base class native
> pointer from the child classes completely to prevent this from happening.  A
few
> suggestions on the design that might help:
> 
> 1. Hide the setting/clearing of the native base pointer from the child class
> completely (see Tab.java and ChromeTab.java).  Can the InfoBarAndroid native
> base class have some kind of setup method that calls the Java private method
> setNativePtr() on the base Java class?  That way the base class InfoBarAndroid
> is responsible for setting up the base class InfoBar in Java.  Child
> InfoBarAndroid classes could override it and do additional setup as well.
> 
> 2. It looks like we have some hybrid approach to creating the Java infobar
> classes (either through some delegate method or through some static method on
> the Infobar subclass).  Could we be consistent about this?  I think it would
> help us come up with a design that minimizes exposing so many native pointers.

> Additionally, we also have a hybrid approach for handling interaction with the
> delegate.  Sometimes we build this Java version of a delegate class and
> sometimes we just hit the InfoBarAndroid class directly which pokes at the
> delegate anyway.  We should be consistent for all of our cross-boundary
designs
> for this feature IMO.
> 
> wdyt?

Good points! I've addressed point 1 by preventing child classes from calling
setNativeInfoBarPtr().
Point 2 is definitely something we'd want to do, but I feel that it's out of
this CL's scope,
so I'd like to handle it separately. (At least we don't pass around base native
pointer any more.)

[David Trainor- moved to gerrit, 2015-06-02 17:51:56]

https://codereview.chromium.org/1150193004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java
(right):

https://codereview.chromium.org/1150193004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:50:
super.onButtonClicked(action, "");
Don't need super?  Same with all others where it makes sense to remove it.

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java
(right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:49:
super.onButtonClicked(action, "");
Do we need super.?  Same with some other places.

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
(right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:57:
private long mNativeInfoBarPtr;
Where do we ever clear this pointer?  I don't see the place, but we should.  The
Android view system can post Runnables/do whatever it wants while we're showing.
 We should clean up the reference when the native object dies (protected called
by native onNativeDestroyed(), that way subclasses can drop their references
too).  wdyt?

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:99:
protected void replaceNativePointer(long newInfoBarPtr) {
Looks like you fixed this to not break.  Can we still just rely on private
methods to do this?  ReassignJavaInfoBar should set the pointer already.  You
just need to fix setNativeInfoBar so it can be called more than once with a
valid pointer.

Then TransferOwnership just needs to change the TranslateStep (new_type?) on the
child class instead of worrying about the pointers.

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java
(right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:232:
mTranslateDelegate.applyTranslateOptions(mNativeTranslateInfoBarPtr,
This one is particularly confusing.  Why isn't this method just a part of this
class?  I could see mocking this out being useful for unit tests, but I don't
see anyone else that calls/implements it.  It's just a straight pass through to
a TranslateInfoBar method anyway.

That whole delegate should probably go IMO.  That can be a follow up patch, but
you might just need to do it here to fix the replaceNativePointer
(changeInfoBarTypeAndNativePointer) bugs anyway.

https://codereview.chromium.org/1150193004/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/infobar_android.cc (right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/infobar_android.cc:23:
InfoBarAndroid::~InfoBarAndroid() {
This should probably tell Java to drop the reference.

https://codereview.chromium.org/1150193004/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/translate_infobar.cc (right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/translate_infobar.cc:107: if (destination !=
nullptr &&
If destination is null, should we just drop ownership anyway (not sure if this
class expects to still own it if this fails).  Should we add a DCHECK and leave
this check in here anyway?

[Changwan Ryu, 2015-06-03 04:55:41]

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java
(right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:49:
super.onButtonClicked(action, "");
On 2015/06/02 17:51:55, David Trainor wrote:
> Do we need super.?  Same with some other places.

Removed.

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
(right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:57:
private long mNativeInfoBarPtr;
On 2015/06/02 17:51:55, David Trainor wrote:
> Where do we ever clear this pointer?  I don't see the place, but we should. 
The
> Android view system can post Runnables/do whatever it wants while we're
showing.
>  We should clean up the reference when the native object dies (protected
called
> by native onNativeDestroyed(), that way subclasses can drop their references
> too).  wdyt?

I was addressing this in a separate CL but merged here.
Almost same but I implemented it slightly differently. Please take another look.

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:99:
protected void replaceNativePointer(long newInfoBarPtr) {
On 2015/06/02 17:51:55, David Trainor wrote:
> Looks like you fixed this to not break.  Can we still just rely on private
> methods to do this?  ReassignJavaInfoBar should set the pointer already.  You
> just need to fix setNativeInfoBar so it can be called more than once with a
> valid pointer.
> 
> Then TransferOwnership just needs to change the TranslateStep (new_type?) on
the
> child class instead of worrying about the pointers.

Good catch that ReassignJavaInfoBar already sets the pointer. This is removed
now. Could you take another look?

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java
(right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:232:
mTranslateDelegate.applyTranslateOptions(mNativeTranslateInfoBarPtr,
On 2015/06/02 17:51:55, David Trainor wrote:
> This one is particularly confusing.  Why isn't this method just a part of this
> class?  I could see mocking this out being useful for unit tests, but I don't
> see anyone else that calls/implements it.  It's just a straight pass through
to
> a TranslateInfoBar method anyway.
> 
> That whole delegate should probably go IMO.  That can be a follow up patch,
but
> you might just need to do it here to fix the replaceNativePointer
> (changeInfoBarTypeAndNativePointer) bugs anyway.

Removed TranslateInfoBarDelegate.

https://codereview.chromium.org/1150193004/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/infobar_android.cc (right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/infobar_android.cc:23:
InfoBarAndroid::~InfoBarAndroid() {
On 2015/06/02 17:51:55, David Trainor wrote:
> This should probably tell Java to drop the reference.

Done.

https://codereview.chromium.org/1150193004/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/translate_infobar.cc (right):

https://codereview.chromium.org/1150193004/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/translate_infobar.cc:107: if (destination !=
nullptr &&
On 2015/06/02 17:51:55, David Trainor wrote:
> If destination is null, should we just drop ownership anyway (not sure if this
> class expects to still own it if this fails).  Should we add a DCHECK and
leave
> this check in here anyway?

Removed the check as it cannot be null.

[David Trainor- moved to gerrit, 2015-06-03 19:38:46]

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:158:
protected void finalize() throws Throwable {
Overriding finalize causes the GC to require multiple GC requests before
actually freeing the object IIRC.  I don't think we necessarily need this since
native owns Java and not the other way around.

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:97:
private void onNativeDestroyed() {
You should make this protected and just override it to clear child class
pointers.  Each class in the inheritence hierarchy doesn't have to notify right?
 It's the same dying object on the native side.

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:53:
public TranslateInfoBar(int infoBarType, int sourceLanguageIndex, int
targetLanguageIndex,
Javadoc.  Or make private because it's built from the static anyway.

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:342:
private void onNativeDestroyed() {
Same comment as before

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:357:
protected void finalize() throws Throwable {
Same comment as before

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
chrome/browser/ui/android/infobars/account_chooser_infobar.cc:99: if
(java_info_bar()) {
Just override onNativeDestroyed() in Java?

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
File chrome/browser/ui/android/infobars/infobar_android.h (right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
chrome/browser/ui/android/infobars/infobar_android.h:49: jobject
java_info_bar();
Probably don't need this.  If you end up needing it either inline the call for
java_info_bar() or make it GetJavaInfoBar().

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
File chrome/browser/ui/android/infobars/translate_infobar.cc (right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
chrome/browser/ui/android/infobars/translate_infobar.cc:31: if (java_info_bar())
{
Just override onNativeDestroyed() in java?

[Changwan Ryu, 2015-06-04 01:42:27]

PTAL

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:158:
protected void finalize() throws Throwable {
On 2015/06/03 19:38:45, David Trainor wrote:
> Overriding finalize causes the GC to require multiple GC requests before
> actually freeing the object IIRC.  I don't think we necessarily need this
since
> native owns Java and not the other way around.

Agreed. Removed them after running git try.

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:97:
private void onNativeDestroyed() {
On 2015/06/03 19:38:45, David Trainor wrote:
> You should make this protected and just override it to clear child class
> pointers.  Each class in the inheritence hierarchy doesn't have to notify
right?
>  It's the same dying object on the native side.

Done.

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:53:
public TranslateInfoBar(int infoBarType, int sourceLanguageIndex, int
targetLanguageIndex,
On 2015/06/03 19:38:45, David Trainor wrote:
> Javadoc.  Or make private because it's built from the static anyway.

Changed to private.

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:342:
private void onNativeDestroyed() {
On 2015/06/03 19:38:45, David Trainor wrote:
> Same comment as before

Done.

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:357:
protected void finalize() throws Throwable {
On 2015/06/03 19:38:45, David Trainor wrote:
> Same comment as before

Done.

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
chrome/browser/ui/android/infobars/account_chooser_infobar.cc:99: if
(java_info_bar()) {
On 2015/06/03 19:38:46, David Trainor wrote:
> Just override onNativeDestroyed() in Java?

Done.

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
File chrome/browser/ui/android/infobars/infobar_android.h (right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
chrome/browser/ui/android/infobars/infobar_android.h:49: jobject
java_info_bar();
On 2015/06/03 19:38:46, David Trainor wrote:
> Probably don't need this.  If you end up needing it either inline the call for
> java_info_bar() or make it GetJavaInfoBar().

Changed it to GetJavaInfoBar().

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
File chrome/browser/ui/android/infobars/translate_infobar.cc (right):

https://chromiumcodereview.appspot.com/1150193004/diff/60001/chrome/browser/u...
chrome/browser/ui/android/infobars/translate_infobar.cc:31: if (java_info_bar())
{
On 2015/06/03 19:38:46, David Trainor wrote:
> Just override onNativeDestroyed() in java?

Done.

[David Trainor- moved to gerrit, 2015-06-04 17:55:49]

lgtm

[cjhopman, 2015-06-04 20:47:51]

lgtm

[Changwan Ryu, 2015-06-04 23:28:30]

On 2015/06/04 20:47:51, cjhopman wrote:
> lgtm

Ted / Miguel, could you take a look as well?

[Ted C, 2015-06-05 00:39:08]

lgtm

https://chromiumcodereview.appspot.com/1150193004/diff/80001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:226:
protected void onButtonClicked(int action, String actionValue) {
javadoc

https://chromiumcodereview.appspot.com/1150193004/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:215:
// We need to re-check if the pointer is null now because applying options (like
never
this comment no longer applies since it is done in the super class.  Or it needs
to be changed to reflect that the parent class will ensure a valid native
counterpart before issuing the button action.

Or you just check mNativeTranslateInfoBarPtr here to keep the old one.

[Changwan Ryu, 2015-06-05 01:04:05]

https://chromiumcodereview.appspot.com/1150193004/diff/80001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:226:
protected void onButtonClicked(int action, String actionValue) {
On 2015/06/05 00:39:08, Ted C wrote:
> javadoc

Done.

https://chromiumcodereview.appspot.com/1150193004/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java
(right):

https://chromiumcodereview.appspot.com/1150193004/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:215:
// We need to re-check if the pointer is null now because applying options (like
never
On 2015/06/05 00:39:08, Ted C wrote:
> this comment no longer applies since it is done in the super class.  Or it
needs
> to be changed to reflect that the parent class will ensure a valid native
> counterpart before issuing the button action.
> 
> Or you just check mNativeTranslateInfoBarPtr here to keep the old one.

Removed.

[Changwan Ryu, 2015-06-05 01:21:23]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2015-06-05 01:21:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dtrainor@chromium.org, cjhopman@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1150193004/#ps190001 (title: "removed
unrelated file")

[commit-bot: I haz the power, 2015-06-05 01:22:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150193004/190001

[commit-bot: I haz the power, 2015-06-05 03:33:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 03:33:51]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Changwan Ryu, 2015-06-05 05:20:16]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2015-06-05 05:20:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150193004/190001

[commit-bot: I haz the power, 2015-06-05 06:07:12]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2015-06-05 06:09:15]

Patchset 11 (id:??) landed as
https://crrev.com/6e63752fcab8553a3b84c5ecf6a356240dacb6e4
Cr-Commit-Position: refs/heads/master@{#333015}