Show search engine promo dialog for special locale

Show search engine promo dialog for special locale

In some locales we will set Sogou as default search engine. This CL adds
a dialog to notify the users of such change.

BUG=638062
Committed: https://crrev.com/afec1c913d505b5b28cf8554731873d11e3ef71e
Cr-Commit-Position: refs/heads/master@{#422891}

[Ian Wen, 2016-09-29 17:49:31]

ianwen@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Ian Wen, 2016-09-29 17:49:32]

PTAL :)

[Maria, 2016-10-01 00:00:46]

https://codereview.chromium.org/2380133002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2380133002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:530:
mLocaleManager.showSearchEnginePromoIfNeeded(ChromeTabbedActivity.this);
Why do we want to do this? Seems like showing after native library has loaded
would be sufficient?

https://codereview.chromium.org/2380133002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/SearchEnginePromoDialog.java
(right):

https://codereview.chromium.org/2380133002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/locale/SearchEnginePromoDialog.java:30:
private ClickableSpan mSpan = new NoUnderlineClickableSpan() {
final

https://codereview.chromium.org/2380133002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/locale/SearchEnginePromoDialog.java:51:
dismiss();
so the dialog won't work in landscape mode?

[Ian Wen, 2016-10-03 17:05:38]

PTAL. In the new patchset I made LocaleManager a singleton.

https://chromiumcodereview.appspot.com/2380133002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://chromiumcodereview.appspot.com/2380133002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:530:
mLocaleManager.showSearchEnginePromoIfNeeded(ChromeTabbedActivity.this);
On 2016/10/01 00:00:46, Maria wrote:
> Why do we want to do this? Seems like showing after native library has loaded
> would be sufficient?

It might take some time for the variation server to return. Meanwhile the user
might already has passed FRE. Here we want to show the promo when a new tab is
created.

https://chromiumcodereview.appspot.com/2380133002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/SearchEnginePromoDialog.java
(right):

https://chromiumcodereview.appspot.com/2380133002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/locale/SearchEnginePromoDialog.java:30:
private ClickableSpan mSpan = new NoUnderlineClickableSpan() {
On 2016/10/01 00:00:46, Maria wrote:
> final

Done.

https://chromiumcodereview.appspot.com/2380133002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/locale/SearchEnginePromoDialog.java:51:
dismiss();
On 2016/10/01 00:00:46, Maria wrote:
> so the dialog won't work in landscape mode?

It will work if orientation changes because ChromeActivity handles config
changes and is not recreated after orientation change.

It will not be reconstructed though if the activity is killed in background,
because I specifically disable it here. This is just to be careful since this
dialog requires native library to be loaded.

[Maria, 2016-10-04 03:42:23]

lgtm after comments are addressed

https://chromiumcodereview.appspot.com/2380133002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/SearchEnginePromoDialog.java
(right):

https://chromiumcodereview.appspot.com/2380133002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/locale/SearchEnginePromoDialog.java:51:
dismiss();
On 2016/10/03 17:05:37, Ian Wen wrote:
> On 2016/10/01 00:00:46, Maria wrote:
> > so the dialog won't work in landscape mode?
> 
> It will work if orientation changes because ChromeActivity handles config
> changes and is not recreated after orientation change.
> 
> It will not be reconstructed though if the activity is killed in background,
> because I specifically disable it here. This is just to be careful since this
> dialog requires native library to be loaded.

Can you mention this in the comment above?

https://chromiumcodereview.appspot.com/2380133002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://chromiumcodereview.appspot.com/2380133002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:671:
mLocaleManager.showSearchEnginePromoIfNeeded(this);
this is a bit weird -- you launch it even if FRE didn't succeed -- seems like
this belongs only within the if() condition.

https://chromiumcodereview.appspot.com/2380133002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://chromiumcodereview.appspot.com/2380133002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:29:
if (sInstance == null) {
this should either be synchronized or assert that it's called on the ui thread
only

[Ian Wen, 2016-10-04 18:28:45]

https://codereview.chromium.org/2380133002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2380133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:671:
mLocaleManager.showSearchEnginePromoIfNeeded(this);
On 2016/10/04 03:42:22, Maria wrote:
> this is a bit weird -- you launch it even if FRE didn't succeed -- seems like
> this belongs only within the if() condition.

Good point. Done.

https://codereview.chromium.org/2380133002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://codereview.chromium.org/2380133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:29:
if (sInstance == null) {
On 2016/10/04 03:42:22, Maria wrote:
> this should either be synchronized or assert that it's called on the ui thread
> only

Done.

[Ian Wen, 2016-10-04 18:28:48]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-10-04 18:28:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2380133002/#ps60001
(title: "maria's comment")

[commit-bot: I haz the power, 2016-10-04 18:29:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 19:48:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-04 19:52:30]

Description was changed from

==========
Show search engine promo dialog for special locale

In some locales we will set Sogou as default search engine. This CL adds
a dialog to notify the users of such change.

BUG=638062
==========

to

==========
Show search engine promo dialog for special locale

In some locales we will set Sogou as default search engine. This CL adds
a dialog to notify the users of such change.

BUG=638062

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

[commit-bot: I haz the power, 2016-10-04 19:52:31]

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