fix "Always do this" checkbox

Split to use two different ID for the two different checkbox in two different subview.
Currently the code always listen to the checkbox created in the advanced view. so even if user click to check/uncheck the box in the before view, the code listen to the value of the checkbox inside the advanced view which was not shown to the user.

BUG=620921
Committed: https://crrev.com/bf4dd975949936a88973a42df91639cec18d0d2e
Cr-Commit-Position: refs/heads/master@{#400797}

[ftang, 2016-06-17 20:41:35]

Description was changed from

==========
fix UMA ALWAYS_TRANSLATE_{CHECKED,UNCHECKED}

BUG=620921
==========

to

==========
fix UMA ALWAYS_TRANSLATE_{CHECKED,UNCHECKED}

BUG=620921
==========

[ftang, 2016-06-17 20:44:36]

Description was changed from

==========
fix UMA ALWAYS_TRANSLATE_{CHECKED,UNCHECKED}

BUG=620921
==========

to

==========
Split to use two different ID for the two different checkbox in two different
subview.
Currently the code always listen to the checkbox created in the advanced view.
so even if user click to check/uncheck the box in the before view, the code
listen to the value of the checkbox inside the advanced view which was not shown
to the user.

BUG=620921
==========

[ftang, 2016-06-17 20:45:24]

ftang@chromium.org changed reviewers:
+ groby@chromium.org

[ftang, 2016-06-17 20:45:24]

Please review this. This is a bad bug and I think we should also cherry pick
into M52

[groby-ooo-7-16, 2016-06-17 21:08:47]

A few thoughts:

For this CL
1) This doesn't seem to keep the two checkboxes in sync if you switch between
advanced/before view?
2) I don't think you need separate IDs for the buttons. Just give them the same
ID. That should automatically keep them in sync, too :)
3) Please name them similarly, e.g. before_always_translate_checkbox_ and
advanced_always_translate_checkbox_

Bigger scope (i.e. not this CL)
1) Having the same control on multiple pages of a multi-page surface is not the
greatest UI. We might want to reconsider that.
2) We probably should break the subviews into their own classes by now - having
it all in a mega-view is what made this happen in the first place.
3) And of course, a nicer way to handle all this would be to have a temporary
model that gets updated on every single change, and then committed on Done.

[ftang, 2016-06-17 22:38:37]

sync the two checkbox and rename them

[ftang, 2016-06-17 22:42:12]

The CQ bit was checked by ftang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-17 22:42:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2074983003/20001

[ftang, 2016-06-17 22:43:03]

address the sync issue and rename as you ask.
Not sure how can I use one single ID for both checkbox.

[commit-bot: I haz the power, 2016-06-17 23:18:31]

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

[commit-bot: I haz the power, 2016-06-17 23:18:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[groby-ooo-7-16, 2016-06-18 00:00:05]

On 2016/06/17 22:43:03, ftang wrote:
> address the sync issue and rename as you ask.
> Not sure how can I use one single ID for both checkbox.

Here's how I'd handle the syncing:

Add a variable to keep track of the desired state for always:
    bool should_always_translate_;

In Init(), initialize with the desired state (that kills the duplicate
initialization logic we have right now)
    should_always_translate_ = model_->ShouldAlwaysTranslate();
    if (Use2016Q2UI() && !is_in_incognito_window_) {
        should_always_translate_ =
            model_->ShouldAlwaysTranslateBeCheckedByDefault();
    }

Add a helper to get the current checkbox:
    views::Checkbox* GetAlwaysTranslateCheckbox() {
        if (model_->GetViewState() == TranslateBubbleModel::VIEW_STATE_ADVANCED)
           return advanced_should_always_translate_checkbox_;
        else if (model_->... VIEW_STATE_BEFOREl
            return before_should_always_translate_checkbox_;
        else
             return nullptr;   // This is technically NOTREACHED
    } 
In the button handler: (This now gets data from whatever checkbox is active at
the moment)
    case BUTTON_ID_ALWAYS_TRANSLATE:
        views::Checkbox* always_checkbox = GetAlwaysTranslateCheckbox();
        if (always_checkbox) {
            should_always_translate_ = always_checkbox->checked();
            translate::ReportUiAction(should_always_translate_
                                                       ?
translate::ALWAYS_TRANSLATE_CHECKED
                                                       :
translate::ALWAYS_TRANSLATE_UNCHECKED);

And transfer state whenever visibility might change, in UpdateChildVisibilities:
(That way, you don't need to worry if there are other places where you need to
do that in the future - a view change will always synchronize data)

    if (advanced_should_always_translate_checkbox_) 
       
advanced_should_always_translate_checkbox_->SetChecked(should_always_translate_);
    if (before_should_always_translate_checkbox_) 
       
before_should_always_translate_checkbox_->SetChecked(should_always_translate_);

And as a last step, update the model from the variable for both
BUTTON_ID_TRANSLATE and BUTTON_ID_DONE

[ftang, 2016-06-18 00:02:13]

check poniter

[ftang, 2016-06-18 15:02:41]

The CQ bit was checked by ftang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-18 15:02:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2074983003/40001

[commit-bot: I haz the power, 2016-06-18 15:36:21]

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

[commit-bot: I haz the power, 2016-06-18 15:36:22]

Dry run: This issue passed the CQ dry run.

[ftang, 2016-06-20 18:28:49]

change the logic based on review comment

[ftang, 2016-06-20 18:29:10]

The CQ bit was checked by ftang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-20 18:29:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2074983003/60001

[ftang, 2016-06-20 18:58:29]

PTAL

[groby-ooo-7-16, 2016-06-20 19:03:10]

LGTM w/ nits

Thank you!

https://codereview.chromium.org/2074983003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/2074983003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_bubble_view.cc:369:
content::WebContents* web_contents)
Please initialize |should_always_translate_| here

https://codereview.chromium.org/2074983003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_bubble_view.cc:415: views::Checkbox*
always_checkbox = GetAlwaysTranslateCheckbox();
I'd suggest just using |should_always_translate_| here, but that's probably a
personal preference.

https://codereview.chromium.org/2074983003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_bubble_view.cc:457: if
(always_checkbox) {
You might want to DCHECK here, instead of if(...) - it's technically invalid for
us to get a button press, but without a checkbox that generated it.

[commit-bot: I haz the power, 2016-06-20 19:16:36]

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

[commit-bot: I haz the power, 2016-06-20 19:16:38]

Dry run: This issue passed the CQ dry run.

[ftang, 2016-06-20 20:53:09]

change base on review comments

[ftang, 2016-06-20 20:53:37]

The CQ bit was checked by ftang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-20 20:54:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2074983003/80001

[ftang, 2016-06-20 21:30:39]

updated

https://codereview.chromium.org/2074983003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/2074983003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_bubble_view.cc:369:
content::WebContents* web_contents)
On 2016/06/20 19:03:10, groby wrote:
> Please initialize |should_always_translate_| here

Done.

https://codereview.chromium.org/2074983003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_bubble_view.cc:415: views::Checkbox*
always_checkbox = GetAlwaysTranslateCheckbox();
On 2016/06/20 19:03:09, groby wrote:
> I'd suggest just using |should_always_translate_| here, but that's probably a
> personal preference.

Done.

https://codereview.chromium.org/2074983003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_bubble_view.cc:457: if
(always_checkbox) {
On 2016/06/20 19:03:10, groby wrote:
> You might want to DCHECK here, instead of if(...) - it's technically invalid
for
> us to get a button press, but without a checkbox that generated it.

Done.

[commit-bot: I haz the power, 2016-06-20 21:39:55]

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

[commit-bot: I haz the power, 2016-06-20 21:39:56]

Dry run: This issue passed the CQ dry run.

[ftang, 2016-06-20 21:40:37]

The CQ bit was checked by ftang@chromium.org

[ftang, 2016-06-20 21:40:37]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org
Link to the patchset: https://codereview.chromium.org/2074983003/#ps80001
(title: "change base on review comments")

[commit-bot: I haz the power, 2016-06-20 21:41:05]

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

[commit-bot: I haz the power, 2016-06-20 21:46:20]

Description was changed from

==========
Split to use two different ID for the two different checkbox in two different
subview.
Currently the code always listen to the checkbox created in the advanced view.
so even if user click to check/uncheck the box in the before view, the code
listen to the value of the checkbox inside the advanced view which was not shown
to the user.

BUG=620921
==========

to

==========
Split to use two different ID for the two different checkbox in two different
subview.
Currently the code always listen to the checkbox created in the advanced view.
so even if user click to check/uncheck the box in the before view, the code
listen to the value of the checkbox inside the advanced view which was not shown
to the user.

BUG=620921
==========

[commit-bot: I haz the power, 2016-06-20 21:46:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-20 21:51:08]

Description was changed from

==========
Split to use two different ID for the two different checkbox in two different
subview.
Currently the code always listen to the checkbox created in the advanced view.
so even if user click to check/uncheck the box in the before view, the code
listen to the value of the checkbox inside the advanced view which was not shown
to the user.

BUG=620921
==========

to

==========
Split to use two different ID for the two different checkbox in two different
subview.
Currently the code always listen to the checkbox created in the advanced view.
so even if user click to check/uncheck the box in the before view, the code
listen to the value of the checkbox inside the advanced view which was not shown
to the user.

BUG=620921

Committed: https://crrev.com/bf4dd975949936a88973a42df91639cec18d0d2e
Cr-Commit-Position: refs/heads/master@{#400797}
==========

[commit-bot: I haz the power, 2016-06-20 21:51:09]

Patchset 5 (id:??) landed as
https://crrev.com/bf4dd975949936a88973a42df91639cec18d0d2e
Cr-Commit-Position: refs/heads/master@{#400797}