Hide translate infobar on programmatic navigation.

Hide translate infobar on programmatic navigation.

In Google news, you can change the UI language through a popup menu [which does so by submitting a form programmatically].  The translation infobar wasn't being refreshed in this situation because the page navigation being performed is programmatic.

Infobar hiding on navigation, goes through TabContents::ExpireInfoBars() which has an early return if the navigation just performed wasn't initiated by the user.  After this, the code loops through the list of infobars and calls ShouldExpire() on each one to determine if it can be closed.

This CL removes the early return and moves the code to determine dismissal to the ShouldExpire() method of the infobar delegates, thus allowing the translate infobar to indicate that it can be dismissed on page navigation.

BUG=70261
TEST=See bug.

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

[jeremy, 2011-05-01 13:50:44]

brettw/ben: Only need one of you to look at this.

[brettw, 2011-05-02 16:35:25]

http://codereview.chromium.org/6883299/diff/1/chrome/browser/translate/transl...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

http://codereview.chromium.org/6883299/diff/1/chrome/browser/translate/transl...
chrome/browser/translate/translate_infobar_delegate.cc:357: return
(contents_unique_id_ != details.entry->unique_id()) ||
Might we still want to check that it's the main frame? Actually, maybe if it's
user initiated OR the main frame might be even better. We don't really want to
trigger for random subframes that may be loaded (ads and such) but some sites
may use a frame for their "main content."

[MAD, 2011-05-02 19:18:02]

An alternate proposal...

http://codereview.chromium.org/6883299/diff/1/chrome/browser/tab_contents/inf...
File chrome/browser/tab_contents/infobar_delegate.cc (right):

http://codereview.chromium.org/6883299/diff/1/chrome/browser/tab_contents/inf...
chrome/browser/tab_contents/infobar_delegate.cc:27: if
(!details.is_user_initiated_main_frame_load())
Why not do this as part of another method, that only the translate bar
overloads?

This way, the translate bar wouldn't have to duplicate the code below, and keep
it in sync... Unless we prefer to put what's below in a shared method...

[jeremy, 2011-05-11 08:24:02]

mad: I made a less invasive change per-your-suggestions by adding a
DoNotCloseOnProgrammaticNavigation() virtual method.

I tend to prefer the previous approach since it's more explicit and easier to
follow but i'm fine either way.  I also, don't like the method name much, but
this is the best idea I had.

http://codereview.chromium.org/6883299/diff/1/chrome/browser/translate/transl...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

http://codereview.chromium.org/6883299/diff/1/chrome/browser/translate/transl...
chrome/browser/translate/translate_infobar_delegate.cc:357: return
(contents_unique_id_ != details.entry->unique_id()) ||
On 2011/05/02 16:35:25, brettw wrote:
> Might we still want to check that it's the main frame? Actually, maybe if it's
> user initiated OR the main frame might be even better. We don't really want to
> trigger for random subframes that may be loaded (ads and such) but some sites
> may use a frame for their "main content."

Done.

[MAD, 2011-05-13 12:41:56]

Excellent. Thanks!

LGTM with two coding style change requests...

And after spending some time triaging translate bugs recently, I think this will
fix many of them... Cool!!!

BYE
MAD

http://codereview.chromium.org/6883299/diff/5001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6883299/diff/5001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1219: if (non_user_initiated_load
&&
I think the coding style asks for {} when the condition spans over more than one
line.

http://codereview.chromium.org/6883299/diff/5001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1221: continue;
Too much indent, should be brought back two spaces...

[brettw, 2011-05-13 17:04:45]

http://codereview.chromium.org/6883299/diff/5001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/infobar_delegate.cc (right):

http://codereview.chromium.org/6883299/diff/5001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/infobar_delegate.cc:30: return true;
Indentation messed up.

http://codereview.chromium.org/6883299/diff/5001/chrome/browser/translate/tra...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

http://codereview.chromium.org/6883299/diff/5001/chrome/browser/translate/tra...
chrome/browser/translate/translate_infobar_delegate.cc:364: return false;
Indentation.

http://codereview.chromium.org/6883299/diff/5001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6883299/diff/5001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1220:
delegate->DoNotCloseOnProgrammaticNavigation())
It's not clear why we need this extra function. Can't everything be implemented
in ShouldExpire? (which you had to provide an implementation of anyway, it looks
like).

[jeremy, 2011-05-13 17:17:15]

That's what I did in my first patch but changed due to mad's review comment.

I'll change it back to the way it was in the first patch + the changes to
TranslateInfobarDelegate::ShouldExpire() in the second patch to dismiss on
user navigation || a load in the main frame.

Can you LGTM based on that so I don't have to wait another day of roundtrip
before landing? If the above explanation of what I want to do isn't clear
and you want to see a final patch, np.

On Fri, May 13, 2011 at 8:04 PM, <brettw@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6883299/diff/5001/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/infobar_delegate.cc (right):
>
>
>
http://codereview.chromium.org/6883299/diff/5001/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/infobar_delegate.cc:30: return true;
> Indentation messed up.
>
>
>
http://codereview.chromium.org/6883299/diff/5001/chrome/browser/translate/tra...
> File chrome/browser/translate/translate_infobar_delegate.cc (right):
>
>
>
http://codereview.chromium.org/6883299/diff/5001/chrome/browser/translate/tra...
> chrome/browser/translate/translate_infobar_delegate.cc:364: return
> false;
> Indentation.
>
>
>
>
http://codereview.chromium.org/6883299/diff/5001/content/browser/tab_contents...
> File content/browser/tab_contents/tab_contents.cc (right):
>
>
>
http://codereview.chromium.org/6883299/diff/5001/content/browser/tab_contents...
> content/browser/tab_contents/tab_contents.cc:1220:
> delegate->DoNotCloseOnProgrammaticNavigation())
> It's not clear why we need this extra function. Can't everything be
> implemented in ShouldExpire? (which you had to provide an implementation
> of anyway, it looks like).
>
>
> http://codereview.chromium.org/6883299/
>

[MAD, 2011-05-13 18:07:32]

Ho, sorry, I looked too quickly, so I revoke my LGTM...

The goal of my proposal was to prevent the code duplication that is still
there... Really sorry that I missed that this morning...

So one way to avoid the code duplication would be to call the base class
implementation instead of copying the code in the
TranslateInfoBarDelegate::ShouldExpire to do exactly the same thing (the
part where it compares the unique_id)... What TranslateInfoBarDelegate is
trying to do is to add conditions to the base class ShouldExpire , right? So
we shouldn't need to copy the code over, just call the base class...

Makes sense?

Again, sorry for the bad LGTM this morning, I should have waited after my
first coffee... :-)

On Fri, May 13, 2011 at 1:16 PM, Jeremy Moskovich <jeremy@chromium.org>wrote:

> That's what I did in my first patch but changed due to mad's review
> comment.
>
> I'll change it back to the way it was in the first patch + the changes to
> TranslateInfobarDelegate::ShouldExpire() in the second patch to dismiss on
> user navigation || a load in the main frame.
>
> Can you LGTM based on that so I don't have to wait another day of roundtrip
> before landing? If the above explanation of what I want to do isn't clear
> and you want to see a final patch, np.
>
> On Fri, May 13, 2011 at 8:04 PM, <brettw@chromium.org> wrote:
>
>>
>>
>>
http://codereview.chromium.org/6883299/diff/5001/chrome/browser/tab_contents/...
>> File chrome/browser/tab_contents/infobar_delegate.cc (right):
>>
>>
>>
http://codereview.chromium.org/6883299/diff/5001/chrome/browser/tab_contents/...
>> chrome/browser/tab_contents/infobar_delegate.cc:30: return true;
>> Indentation messed up.
>>
>>
>>
http://codereview.chromium.org/6883299/diff/5001/chrome/browser/translate/tra...
>> File chrome/browser/translate/translate_infobar_delegate.cc (right):
>>
>>
>>
http://codereview.chromium.org/6883299/diff/5001/chrome/browser/translate/tra...
>> chrome/browser/translate/translate_infobar_delegate.cc:364: return
>> false;
>> Indentation.
>>
>>
>>
>>
http://codereview.chromium.org/6883299/diff/5001/content/browser/tab_contents...
>> File content/browser/tab_contents/tab_contents.cc (right):
>>
>>
>>
http://codereview.chromium.org/6883299/diff/5001/content/browser/tab_contents...
>> content/browser/tab_contents/tab_contents.cc:1220:
>> delegate->DoNotCloseOnProgrammaticNavigation())
>> It's not clear why we need this extra function. Can't everything be
>> implemented in ShouldExpire? (which you had to provide an implementation
>> of anyway, it looks like).
>>
>>
>> http://codereview.chromium.org/6883299/
>>
>
>

[brettw, 2011-05-13 18:10:36]

LGTM from a high level, I'll assume you'll work with MAD to reduce duplication
and avoid extra weird functions.

[jeremy, 2011-05-17 10:46:23]

New version uploaded - ready for another look.

[jeremy, 2011-05-17 14:52:14]

http://codereview.chromium.org/6883299/diff/20001/chrome/browser/translate/tr...
File chrome/browser/translate/translate_manager_browsertest.cc (left):

http://codereview.chromium.org/6883299/diff/20001/chrome/browser/translate/tr...
chrome/browser/translate/translate_manager_browsertest.cc:724: // Navigate in
page, the same infobar should still be shown.
I've removed this part of the test since the navigation here is programmatic
which dismisses the infobar. Before this CL the infobar would stick around, not
because we were navigating in the page but because of the programatic
navigation.

[MAD, 2011-05-17 16:25:55]

Cool Thanks!

LGTM with two style nits...

BYE
MAD

http://codereview.chromium.org/6883299/diff/20001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/infobar_delegate.cc (right):

http://codereview.chromium.org/6883299/diff/20001/chrome/browser/tab_contents...
chrome/browser/tab_contents/infobar_delegate.cc:28: return false;
Too much indent...

http://codereview.chromium.org/6883299/diff/20001/chrome/browser/translate/tr...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

http://codereview.chromium.org/6883299/diff/20001/chrome/browser/translate/tr...
chrome/browser/translate/translate_infobar_delegate.cc:356: // was programmatic
and not initiated by the user - crbug.com/70261 .
Remove extra space before '.'