Fix the IllegalStateException in OtherFormsOfHistoryDialogFragment.show()

Fix the IllegalStateException in OtherFormsOfHistoryDialogFragment.show()

OtherFormsOfHistoryDialogFragment.show() can throw an IllegalStateException
if it's called after the Activity's onSaveInstanceState(). New fragments
cannot be added after the Acitvity state was saved, as these changes to
the state would be lost.

This CL fixes the problem by only attempting to show the fragment if the
Activity is not in a background state.

BUG=648515
Committed: https://crrev.com/dbdc108bed93786d012ce905a38535e81f52e80d
Cr-Commit-Position: refs/heads/master@{#420638}

[msramek, 2016-09-21 12:09:53]

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

[commit-bot: I haz the power, 2016-09-21 12:10:02]

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

[msramek, 2016-09-21 12:14:20]

msramek@chromium.org changed reviewers:
+ finnur@chromium.org

[msramek, 2016-09-21 12:14:20]

Hi Finnur!

Somehow I never noticed that we have preferences/ owners after Newt left,
perhaps I should have been sending more CLs to you in the past :)

Please have a look at this crash fix! Not sure if there's a more elegant way to
do it...

Thanks,
Martin

[Finnur, 2016-09-21 13:10:47]

I don't know... twellington@ should probably be the primary candidate; she works
on Chrome Android UI, which is an I haven't done much of late in.

This LGTM, though.

[commit-bot: I haz the power, 2016-09-21 13:27:57]

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

[commit-bot: I haz the power, 2016-09-21 13:27:58]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-09-21 13:33:54]

msramek@chromium.org changed reviewers:
+ twellington@chromium.org

[msramek, 2016-09-21 13:33:55]

Thanks! Then let me ask +Theresa as well - does this LGTY or do you maybe know a
more elegant way to determine if a DialogFragment cannot be shown?

[Theresa, 2016-09-21 17:08:37]

https://codereview.chromium.org/2359753002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/2359753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:335:
// the |mPaused| state of this fragment to determine that.
OnPause() is no longer a reliable way to know whether the activity is in the
background. In Android N multi-window mode, only one activity is resumed at any
given time. The other visible activity is in the paused state.

This method getting called while the activity is visible but paused seems like a
side case. If you test and think the behavior is okay, I would be fine with
landing as is. 

Otherwise, you may want to use MultiWindowUtils#isActivityVisible() which checks
whether the activity state is paused or resumed. That method is currently
private but it could easily be changed to a public static.

[msramek, 2016-09-23 12:59:25]

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

[commit-bot: I haz the power, 2016-09-23 12:59:37]

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

[msramek, 2016-09-23 13:03:24]

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

[msramek, 2016-09-23 13:03:28]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-09-23 13:03:33]

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

[msramek, 2016-09-23 13:08:08]

Thanks for the advice, Theresa! PTAL :)

https://codereview.chromium.org/2359753002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/2359753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:335:
// the |mPaused| state of this fragment to determine that.
On 2016/09/21 17:08:37, Theresa Wellington wrote:
> OnPause() is no longer a reliable way to know whether the activity is in the
> background. In Android N multi-window mode, only one activity is resumed at
any
> given time. The other visible activity is in the paused state.
> 
> This method getting called while the activity is visible but paused seems like
a
> side case. If you test and think the behavior is okay, I would be fine with
> landing as is. 
> 
> Otherwise, you may want to use MultiWindowUtils#isActivityVisible() which
checks
> whether the activity state is paused or resumed. That method is currently
> private but it could easily be changed to a public static.

In multiwindow scenarios, we still want to show the dialog if the activity is
visible, even if it's not the currently active one.

I changed MultiWindowUtils#isActivityVisible() to public and tested on single
and double screens. It works as expected and prevents this crash.

[commit-bot: I haz the power, 2016-09-23 14:18:08]

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

[commit-bot: I haz the power, 2016-09-23 14:18:09]

Dry run: This issue passed the CQ dry run.

[Theresa, 2016-09-23 16:54:48]

lgtm

[msramek, 2016-09-23 16:57:49]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-09-23 16:57:50]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/2359753002/#ps40001
(title: "MultiWindowUtils.isActivityVisible")

[commit-bot: I haz the power, 2016-09-23 16:58:25]

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-09-23 17:04:55]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-23 17:06:34]

Description was changed from

==========
Fix the IllegalStateException in OtherFormsOfHistoryDialogFragment.show()

OtherFormsOfHistoryDialogFragment.show() can throw an IllegalStateException
if it's called after the Activity's onSaveInstanceState(). New fragments
cannot be added after the Acitvity state was saved, as these changes to
the state would be lost.

This CL fixes the problem by only attempting to show the fragment if the
Activity is not in a background state.

BUG=648515
==========

to

==========
Fix the IllegalStateException in OtherFormsOfHistoryDialogFragment.show()

OtherFormsOfHistoryDialogFragment.show() can throw an IllegalStateException
if it's called after the Activity's onSaveInstanceState(). New fragments
cannot be added after the Acitvity state was saved, as these changes to
the state would be lost.

This CL fixes the problem by only attempting to show the fragment if the
Activity is not in a background state.

BUG=648515

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

[commit-bot: I haz the power, 2016-09-23 17:06:35]

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