✈ Remove unnecessary flag and DocumentModeManager.

Remove unnecessary flag and DocumentModeManager.

- Clean-ups due to simpler logic of only migrating from document mode
  back to tabbed mode on update.

- Simplify opt-out check for document mode and consolidate to
  DocumentModeAssassin.

- Remove unused tests and simplify assumptions related to document
  mode.

BUG=612245, 582539
Committed: https://crrev.com/4804efe6387351c7802a2e9f34afe84e7ccbe7fe
Cr-Commit-Position: refs/heads/master@{#396341}

[Peter Wen, 2016-05-25 17:14:34]

wnwen@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Peter Wen, 2016-05-25 17:14:35]

✏

[gone, 2016-05-25 17:48:51]

Seems fine to me so far, but this hammer is also going to be dropping:

https://codereview.chromium.org/2011433003

Our CLs somehow don't intersect too badly, but your CL isn't currently
compiling.

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java
(right):

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:107:
public static final String OPT_OUT_STATE = "opt_out_state";
Add a comment here describing how these are/were used?  I don't think some are
used anymore.

[gone, 2016-05-25 18:08:23]

Can you mark this as part of bug 582539, too?

[gone, 2016-05-25 18:26:05]

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java
(right):

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:555:
OPT_OUT_STATE, OPTED_OUT_OF_DOCUMENT_MODE) == OPTED_OUT_OF_DOCUMENT_MODE;
I'm really wary of this logic change.  Can you do manual testing to confirm that
users in document mode on M50 are migrated out correctly?  You'll need a local
M50 build in document mode to check.

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:559:
public static void setOptedOutState(int state) {
private?

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
(right):

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:118:
// isOptedOutOfDocumentMode is true by default.
This comment is correct in the current state but misleading.  Just remove the
comment.

[Peter Wen, 2016-05-26 16:15:53]

Description was changed from

==========
Remove unnecessary flag and DocumentModeManager.

- Clean-ups due to simpler logic of only migrating from document mode
  back to tabbed mode on update.

- Simplify opt-out check for document mode and consolidate to
  DocumentModeAssassin.

- Remove unused tests and simplify assumptions related to document
  mode.

BUG=612245
==========

to

==========
Remove unnecessary flag and DocumentModeManager.

- Clean-ups due to simpler logic of only migrating from document mode
  back to tabbed mode on update.

- Simplify opt-out check for document mode and consolidate to
  DocumentModeAssassin.

- Remove unused tests and simplify assumptions related to document
  mode.

BUG=612245,582539
==========

[Peter Wen, 2016-05-26 16:17:55]

⛳

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java
(right):

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:107:
public static final String OPT_OUT_STATE = "opt_out_state";
On 2016/05/25 17:48:51, dfalcantara wrote:
> Add a comment here describing how these are/were used?  I don't think some are
> used anymore.

Done.

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:555:
OPT_OUT_STATE, OPTED_OUT_OF_DOCUMENT_MODE) == OPTED_OUT_OF_DOCUMENT_MODE;
On 2016/05/25 18:26:05, dfalcantara wrote:
> I'm really wary of this logic change.  Can you do manual testing to confirm
that
> users in document mode on M50 are migrated out correctly?  You'll need a local
> M50 build in document mode to check.

I've been trying to test the migration path this morning, but having a lot of
difficulty doing so. I tried downloading the app from go/clank-apks and building
release official stable, but the ToT version has "developer version" and doesn't
want to migrate. How do you normally test this or get a local build of M50?

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:559:
public static void setOptedOutState(int state) {
On 2016/05/25 18:26:05, dfalcantara wrote:
> private?

Done.

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
(right):

https://chromiumcodereview.appspot.com/2013883002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:118:
// isOptedOutOfDocumentMode is true by default.
On 2016/05/25 18:26:05, dfalcantara wrote:
> This comment is correct in the current state but misleading.  Just remove the
> comment.

Done.

[gone, 2016-05-26 16:20:37]

I did this when checking that I didn't break anything:
1) Check out a local Clankium m50 repo
2) Build and installing that APK
3) Turning on Hera
4) Compile my local Trunk build
5) Install my updated trunk APK to trigger migration

[gone, 2016-05-26 16:38:43]

dfalcantara@chromium.org changed reviewers:
+ yusufo@chromium.org

[gone, 2016-05-26 16:38:44]

+Yusuf to look over the DocumentModeManager adaptations.  This path is sensitive
enough to warrant (who I believe was) the original author looking at it and
making sure I'm reading it right.

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

https://chromiumcodereview.appspot.com/2013883002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:109:
// (Unused) private static final int OPT_IN_TO_DOCUMENT_MODE = 0;
Might as well just delete these.

https://chromiumcodereview.appspot.com/2013883002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:553:
/** @return Whether the user set a preference to not use the document mode. */
This comment doesn't make sense because it wasn't necessarily a user preference.

@return True if the user is not in document mode.

https://chromiumcodereview.appspot.com/2013883002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:561:
ContextUtils.getAppSharedPreferences().edit().remove(OPT_OUT_STATE).apply();
On its own, this does the exact opposite of the call you replaced.  Was this an
intentional change?  Are you implicitly relying on the default value of the
#getInt call above?

[Peter Wen, 2016-05-26 19:13:56]

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

https://chromiumcodereview.appspot.com/2013883002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:109:
// (Unused) private static final int OPT_IN_TO_DOCUMENT_MODE = 0;
On 2016/05/26 16:38:43, dfalcantara wrote:
> Might as well just delete these.

Done.

https://chromiumcodereview.appspot.com/2013883002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:553:
/** @return Whether the user set a preference to not use the document mode. */
On 2016/05/26 16:38:43, dfalcantara wrote:
> This comment doesn't make sense because it wasn't necessarily a user
preference.
> 
> @return True if the user is not in document mode.

Done.

https://chromiumcodereview.appspot.com/2013883002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:561:
ContextUtils.getAppSharedPreferences().edit().remove(OPT_OUT_STATE).apply();
On 2016/05/26 16:38:43, dfalcantara wrote:
> On its own, this does the exact opposite of the call you replaced.  Was this
an
> intentional change?  Are you implicitly relying on the default value of the
> #getInt call above?

Yep, assuming we make the change to the default value.

[Yusuf, 2016-05-26 20:17:05]

DocumentModeManager bits lgtm

[Peter Wen, 2016-05-26 21:37:35]

On 2016/05/26 20:17:05, Yusuf wrote:
> DocumentModeManager bits lgtm

ToT with this patch upgrades fine for me, didn't even see the splash screen.

[gone, 2016-05-26 21:46:01]

lgtm

[Peter Wen, 2016-05-26 21:51:02]

The CQ bit was checked by wnwen@chromium.org

[commit-bot: I haz the power, 2016-05-26 21:51:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2013883002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2013883002/40001

[commit-bot: I haz the power, 2016-05-27 01:15:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-27 01:16:23]

Description was changed from

==========
Remove unnecessary flag and DocumentModeManager.

- Clean-ups due to simpler logic of only migrating from document mode
  back to tabbed mode on update.

- Simplify opt-out check for document mode and consolidate to
  DocumentModeAssassin.

- Remove unused tests and simplify assumptions related to document
  mode.

BUG=612245,582539
==========

to

==========
Remove unnecessary flag and DocumentModeManager.

- Clean-ups due to simpler logic of only migrating from document mode
  back to tabbed mode on update.

- Simplify opt-out check for document mode and consolidate to
  DocumentModeAssassin.

- Remove unused tests and simplify assumptions related to document
  mode.

BUG=612245,582539

Committed: https://crrev.com/4804efe6387351c7802a2e9f34afe84e7ccbe7fe
Cr-Commit-Position: refs/heads/master@{#396341}
==========

[commit-bot: I haz the power, 2016-05-27 01:16:24]

Patchset 3 (id:??) landed as
https://crrev.com/4804efe6387351c7802a2e9f34afe84e7ccbe7fe
Cr-Commit-Position: refs/heads/master@{#396341}