Add confirmation dialog when enabling a 3rd party IME.

Add confirmation dialog when enabling a 3rd party IME.

Based on patch 290083004. Third party IMEs provide a privacy risk as they can monitor everything you type. The patch provides an additional measure of security by prompting the user for confirmation with a warning of the potential risks.

BUG=367861
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276535

[kevers, 2014-06-09 15:26:46]

Hi Dan and Shu,

Can you please take a look at this CL.

[Dan Beam, 2014-06-09 21:37:36]

overall looks pretty good...

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js
(right):

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js:47:
SettingsDialog.prototype.handleConfirm.call(this);
why are you calling into the super second?

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js:83:
instance.showConfirmationDialog_.apply(instance, arguments);
ThirdPartyImeConfirmOverlay.getInstance().showConfirmationDialog(data);

is more common.  I get what you're doing but it's probably not necessary...

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js:84:
}
};

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/language_options.js (right):

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/language_options.js:831: if
(!!checkbox.imeProvider && checkbox.checked) {
you don't need the !!

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/language_options.js:835: };
nit: var confirmationCallback = this.handleCheckboxUpdate_.bind(this, checkbox);

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/language_options.js:838: }
};

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:124:
ext_ime_descriptors);
indent off

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:561:
list->GetDictionary(i, &entry);
nit: \n

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:563:
entry->GetString("id", &input_method_id);
nit: \n

[Shu Chen, 2014-06-10 01:02:18]

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/language_options.js (right):

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/language_options.js:831: if
(!!checkbox.imeProvider && checkbox.checked) {
I think it would be safer to check IME ID instead of imeProvider to show the
warning dialog.

Please use /^_ext_ime_/.test(checkbox.inputMethodId).

[kevers, 2014-06-10 17:56:45]

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js
(right):

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js:47:
SettingsDialog.prototype.handleConfirm.call(this);
On 2014/06/09 21:37:36, Dan Beam wrote:
> why are you calling into the super second?

Either order is fine since there is no preference attached to the checkbox. If
there were a preference attached, the super call should be second in order to
pick up the correct state as confirmCallback does extra work in updating
'settings.language.enabled_extension_imes'.

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js:83:
instance.showConfirmationDialog_.apply(instance, arguments);
On 2014/06/09 21:37:35, Dan Beam wrote:
> ThirdPartyImeConfirmOverlay.getInstance().showConfirmationDialog(data);
> 
> is more common.  I get what you're doing but it's probably not necessary...

Done.

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js:84:
}
On 2014/06/09 21:37:35, Dan Beam wrote:
> };

Done.

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/language_options.js (right):

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/language_options.js:831: if
(!!checkbox.imeProvider && checkbox.checked) {
On 2014/06/10 01:02:18, Shu Chen wrote:
> I think it would be safer to check IME ID instead of imeProvider to show the
> warning dialog.
> 
> Please use /^_ext_ime_/.test(checkbox.inputMethodId).

Done.

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/language_options.js:835: };
On 2014/06/09 21:37:36, Dan Beam wrote:
> nit: var confirmationCallback = this.handleCheckboxUpdate_.bind(this,
checkbox);

Done.

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/language_options.js:838: }
On 2014/06/09 21:37:36, Dan Beam wrote:
> };

Done.

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:124:
ext_ime_descriptors);
On 2014/06/09 21:37:36, Dan Beam wrote:
> indent off

fixed

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:561:
list->GetDictionary(i, &entry);
On 2014/06/09 21:37:36, Dan Beam wrote:
> nit: \n

Done.

https://codereview.chromium.org/325633003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:563:
entry->GetString("id", &input_method_id);
On 2014/06/09 21:37:36, Dan Beam wrote:
> nit: \n

Done.

[Dan Beam, 2014-06-10 18:08:17]

lgtm

https://codereview.chromium.org/325633003/diff/80001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/325633003/diff/80001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:3242: </message>
^ I would just use the standard messages for this

https://codereview.chromium.org/325633003/diff/80001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:3249: This input method may be able to collect
all the text you type, including personal data like passwords and credit card
numbers. It comes from the extension "<ph name="EXTENSION_NAME">$1<ex>Google
Input Tools extension</ex></ph>". Use this input method?
some of these " probably need to be &quot;

https://codereview.chromium.org/325633003/diff/80001/chrome/browser/resources...
File
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js
(right):

https://codereview.chromium.org/325633003/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js:47:
SettingsDialog.prototype.handleConfirm.call(this);
if the order doesn't matter, put the super first (this is a good practice, imo)

[kevers, 2014-06-10 23:02:35]

https://codereview.chromium.org/325633003/diff/80001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/325633003/diff/80001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:3242: </message>
On 2014/06/10 18:08:17, Dan Beam wrote:
> ^ I would just use the standard messages for this

Done.

https://codereview.chromium.org/325633003/diff/80001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:3249: This input method may be able to collect
all the text you type, including personal data like passwords and credit card
numbers. It comes from the extension "<ph name="EXTENSION_NAME">$1<ex>Google
Input Tools extension</ex></ph>". Use this input method?
On 2014/06/10 18:08:17, Dan Beam wrote:
> some of these " probably need to be &quot;

Either appears to work, but switched to &quot;

https://codereview.chromium.org/325633003/diff/80001/chrome/browser/resources...
File
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js
(right):

https://codereview.chromium.org/325633003/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/third_party_ime_confirm_overlay.js:47:
SettingsDialog.prototype.handleConfirm.call(this);
On 2014/06/10 18:08:17, Dan Beam wrote:
> if the order doesn't matter, put the super first (this is a good practice,
imo)

Done.

[Shu Chen, 2014-06-11 00:39:17]

lgtm

[kevers, 2014-06-11 13:27:46]

The CQ bit was checked by kevers@chromium.org

[commit-bot: I haz the power, 2014-06-11 13:29:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kevers@chromium.org/325633003/100001

[commit-bot: I haz the power, 2014-06-11 21:27:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:31:10]

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

[commit-bot: I haz the power, 2014-06-11 21:31:11]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[kevers, 2014-06-12 01:24:59]

The CQ bit was checked by kevers@chromium.org

[commit-bot: I haz the power, 2014-06-12 01:25:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kevers@chromium.org/325633003/100001

[commit-bot: I haz the power, 2014-06-12 04:54:43]

Change committed as 276535