[Android] Show document mode opt-out InfoBar on selected devices.
There are many Android devices that doesn't have a tab switcher button
or have it but inconvinient to use. We already make tabbed mode default
for those devices. This CL adds a document mode opt-out promotion
InfoBar for the existing users on those devices.
BUG=546601
https://codereview.chromium.org/1435263003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java (right): https://codereview.chromium.org/1435263003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java#newcode47 chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:47: .equals("enabled")) { I've found bugs in this condition. Fixing...
5 years, 1 month ago
(2015-11-17 19:44:24 UTC)
#11
Another general question: why is this a "tabbed mode opt-in" instead of a "document mode ...
5 years, 1 month ago
(2015-11-17 23:20:07 UTC)
#14
Another general question: why is this a "tabbed mode opt-in" instead of a
"document mode opt-out"? You're opted-in to document mode and have to opt-out
of it.
Kibeom Kim (inactive)
For opt-in and opt-out, I think both are right and doesn't really matter. Initially I ...
5 years, 1 month ago
(2015-11-20 11:44:15 UTC)
#15
For opt-in and opt-out, I think both are right and doesn't really matter.
Initially I chose tabbed mode opt-in because this is for the users who were
given document mode by default and the infobar is mainly describing tabbed mode.
But anyways, I'll follow what you recommand, do you suggest opt-out?
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:193:
private boolean mIsShowingTabbedModeOptInInfoBarAttempted = false;
On 2015/11/17 23:18:16, dfalcantara wrote:
> don't bother initializing to false; get rid of the one below this too.
Done.
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
(right):
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:249:
* @return Whether foreground session is started by this call, i.e., no other
Chrome activities
On 2015/11/17 23:18:16, dfalcantara wrote:
> This is misleading if another Activity was killed but the process was still
> alive and another Activity started up.
Done.
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java
(right):
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:19:
* Handles InfoBar requesting tabbed mode opt in for the devices that document
mode is inconvenient
On 2015/11/17 23:18:16, dfalcantara wrote:
> nit: opt-in
Done.
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:24:
private static final String PREF_USER_INTERACTED_TABBED_MODE_OPT_IN_INFOBAR =
On 2015/11/17 23:18:16, dfalcantara wrote:
> user_interacted or user_denied? the string is inconsistent.
Done.
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:29:
private Activity mActivity;
On 2015/11/17 23:18:16, dfalcantara wrote:
> Newt says to use the themedApplicationContext instead because InfoBars can be
> transferred between Activities. Check how Tab gets it. You don't seem to
even
> need the Activity, here -- just a Context.
I needed Activity for "DocumentMigrationHelper.migrateTabs(false, mActivity,
true);" at #63. Hmm.. I didn't know that InfoBars can be transferred, so I guess
that means I can't save an activity here and find another way to pipe at the
time accept() runs?
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:39:
public static void showIfNecessary(ChromeActivity activity) {
On 2015/11/17 23:18:16, dfalcantara wrote:
> Why do you pass in a second Activity here? Why do you store the other one if
> you really only need it for a themed context?
commented above.
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:48:
delegate.nativeLaunchTabbedModeOptInInfoBar(activity.getActivityTab());
On 2015/11/17 23:18:16, dfalcantara wrote:
> I don't think I've ever heard of using "Launching" terminology for an InfoBar.
> Can you use "Show" instead?
Done. (initially I stole the name from
FullscreenInfoBarDelegate#nativeLaunchFullscreenInfoBar)
https://codereview.chromium.org/1435263003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:60:
DocumentModeManager mDocumentModeManager =
DocumentModeManager.getInstance(mActivity);
On 2015/11/17 23:18:16, dfalcantara wrote:
> Can you refactor out the logic in DocumentModePreference instead of copying it
> here?
Done.
https://codereview.chromium.org/1435263003/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):
https://codereview.chromium.org/1435263003/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:339:
{"TabbedModeOptInInfobarDelegate",
On 2015/11/17 23:18:16, dfalcantara wrote:
> Keep your InfoBar and Infobar naming consistent. It's always InfoBar.
Done.
https://codereview.chromium.org/1435263003/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/tabbed_mode_opt_in_infobar_delegate.cc
(right):
https://codereview.chromium.org/1435263003/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/tabbed_mode_opt_in_infobar_delegate.cc:81: //
Chrome is restarting as tabbed mode so we don't need to close.
On 2015/11/17 23:18:16, dfalcantara wrote:
> as tabbed mode -> in tabbed mode
Done.
https://codereview.chromium.org/1435263003/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/tabbed_mode_opt_in_infobar_delegate.cc:83: //
slide down and Chrome is restarted before the sliding is finished.
On 2015/11/17 23:18:16, dfalcantara wrote:
> infobar -> InfoBar.
Done.
https://codereview.chromium.org/1435263003/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/tabbed_mode_opt_in_infobar_delegate.h
(right):
https://codereview.chromium.org/1435263003/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/tabbed_mode_opt_in_infobar_delegate.h:18: //
An InfoBar delegate that asks to opt in tabbed mode for the devices that
On 2015/11/17 23:18:16, dfalcantara wrote:
> nits: InfoBarDelegate. opt-in. Finish the sentence with a period.
Done.
https://codereview.chromium.org/1435263003/diff/80001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):
https://codereview.chromium.org/1435263003/diff/80001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12425: Recorded when tabbed mode opt in
promotion InfoBar for selected devices on
On 2015/11/17 23:18:16, dfalcantara wrote:
> 1) opt-in here and everywhere else.
> 2) All of these sentences are pretty confusing.
> 3) Why are you using actions instead of histograms?
Done.
gone
That's the point -- document mode users were opted INto that by default. This InfoBar ...
5 years, 1 month ago
(2015-11-20 18:30:56 UTC)
#16
That's the point -- document mode users were opted INto that by default. This
InfoBar offers them a way OUT of document mode (if they missed the option to do
so on first run, or have decided they hate it).
gone
https://chromiumcodereview.appspot.com/1435263003/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java (right): https://chromiumcodereview.appspot.com/1435263003/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java#newcode29 chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:29: private Activity mActivity; On 2015/11/20 11:44:14, Kibeom Kim wrote: ...
5 years, 1 month ago
(2015-11-20 19:26:14 UTC)
#17
https://chromiumcodereview.appspot.com/1435263003/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java
(right):
https://chromiumcodereview.appspot.com/1435263003/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:29:
private Activity mActivity;
On 2015/11/20 11:44:14, Kibeom Kim wrote:
> On 2015/11/17 23:18:16, dfalcantara wrote:
> > Newt says to use the themedApplicationContext instead because InfoBars can
be
> > transferred between Activities. Check how Tab gets it. You don't seem to
> even
> > need the Activity, here -- just a Context.
>
> I needed Activity for "DocumentMigrationHelper.migrateTabs(false, mActivity,
> true);" at #63. Hmm.. I didn't know that InfoBars can be transferred, so I
guess
> that means I can't save an activity here and find another way to pipe at the
> time accept() runs?
Can you use the most recent foreground ChromeActivity from
ApplicationStatus#getRunningActivities()?
https://chromiumcodereview.appspot.com/1435263003/diff/100001/chrome/android/...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):
https://chromiumcodereview.appspot.com/1435263003/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:193:
private boolean mIsShowingTabbedModeOptInInfoBarAttempted;
nit: this variable name is painful to read.
mAttemptedToShowWhateverInfoBar?
https://chromiumcodereview.appspot.com/1435263003/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java
(right):
https://chromiumcodereview.appspot.com/1435263003/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TabbedModeOptInInfoBarDelegate.java:24:
private static final String PREF_USER_INTERACTED_TABBED_MODE_OPT_IN_INFOBAR =
nit: USER_INTERACTED_WITH
Description was changed from
==========
[Android] Show tabbed mode opt in infobar on selected devices.
There are many Android devices that doesn't have a tab switcher button
or have it but inconvinient to use. We already make tabbed mode default
for those devices. This CL adds a tabbed mode opt in promotion infobar
for the existing users on those devices.
BUG=546601
==========
to
==========
[Android] Show document mode opt-out infobar on selected devices.
There are many Android devices that doesn't have a tab switcher button
or have it but inconvinient to use. We already make tabbed mode default
for those devices. This CL adds a document mode opt-out promotion
infobar for the existing users on those devices.
BUG=546601
==========
Kibeom Kim (inactive)
Description was changed from ========== [Android] Show document mode opt-out infobar on selected devices. There ...
Description was changed from
==========
[Android] Show document mode opt-out infobar on selected devices.
There are many Android devices that doesn't have a tab switcher button
or have it but inconvinient to use. We already make tabbed mode default
for those devices. This CL adds a document mode opt-out promotion
infobar for the existing users on those devices.
BUG=546601
==========
to
==========
[Android] Show document mode opt-out InfoBar on selected devices.
There are many Android devices that doesn't have a tab switcher button
or have it but inconvinient to use. We already make tabbed mode default
for those devices. This CL adds a document mode opt-out promotion
InfoBar for the existing users on those devices.
BUG=546601
==========
On 2015/12/09 01:55:07, Kibeom Kim (OOO till 2016) wrote:
> ptal
Canceling ptal. We're reconsidering this feature. I'll close or ping again once
it's decided.
Issue 1435263003: [Android] Show document mode opt-out InfoBar on selected devices.
(Closed)
Created 5 years, 1 month ago by Kibeom Kim (inactive)
Modified 4 years, 11 months ago
Reviewers: gone, rkaplow, Ted C
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 39
Chromium Code Reviews
Issue
1435263003:
[Android] Show document mode opt-out InfoBar on selected devices. (Closed)
