[Document mode] Allow multiple popup windows to appear.

[Document mode] Allow multiple popup windows to appear.

Introduce a queue in TabDelegate for creating multiple Tabs in a row.
This prevents triggering some issue in Android's ActivityManager that
causes it to throw away the tasks for Activities after it launches them,
which in turn causes Android to destroy our Activities.

BUG=498920
Committed: https://crrev.com/e7c6880439e7b4320338fbdf206151296f99ac71
Cr-Commit-Position: refs/heads/master@{#339799}

[gone, 2015-07-20 21:01:26]

dfalcantara@chromium.org changed reviewers:
+ mariakhomenko@google.com, tedchoc@chromium.org

[gone, 2015-07-20 21:01:26]

CL does what we talked about on Friday about serializing the popup creation.  In
practice hopefully this isn't hit too much, unless the user is browsing less
savory websites.  It'll still be a problem if two tabs in parallel try to create
tabs simultaneously because they don't share the same TabDelegate, but that
should be even rarer in practice.

[gone, 2015-07-20 21:04:09]

cc'ing Peter for PopupTest changes.

[gone, 2015-07-20 22:29:47]

Actually; hold off. I'm going to pull out the TabLaunchRunnable into its own
class.

[gone, 2015-07-20 23:37:54]

Moved it out, PTAL.

[Ted C, 2015-07-21 21:06:47]

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:454:
public static int launchDocumentInstance(
javadoc the return

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java
(right):

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java:217:
if (activity == null) continue;
this can't happen the instance of check above would fail for null

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java:219:
if (activity.getTabModelSelector().getModelForTabId(id) != null) return
activity;
I would probably use getTabById instead of getModelForTabId...for no good reason
other than readability.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/AsyncDocumentLaunchSerializer.java
(right):

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/AsyncDocumentLaunchSerializer.java:24:
public class AsyncDocumentLaunchSerializer {
I would drop Serializer and just make this the AsyncDocumentLauncher.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/AsyncDocumentLaunchSerializer.java:31:
private static final int MAX_WAIT = 3000;
add _MS here and below to POLLING_DELAY

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/AsyncDocumentLaunchSerializer.java:91:
private LaunchRunnable mCurrentRunnable = null;
= null is the default, so we can leave that off.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java
(right):

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java:106:
return TextUtils.equals("Popup #3", selector.getCurrentTab().getTitle());
Was it wrong checking for Popup #2 before? ahh ha, you changed the test file. 
nvm

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java:108: },
7500, 1000));
the second polling is quite large, why not just use
CriteriaHelper.DEFAULT_POLLING_INTERVAL.  The 1sec will make this test slower
than it needs to be.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java:114:
loadUrl(POPUP_HTML_FILENAME);
popups always load in the background right?  This will load in the still
visible/original activity?  Just making sure getActivity() is actually checking
the proper thing.

[gone, 2015-07-21 21:30:12]

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:454:
public static int launchDocumentInstance(
On 2015/07/21 21:06:46, Ted C wrote:
> javadoc the return

Done.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java
(right):

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java:217:
if (activity == null) continue;
On 2015/07/21 21:06:46, Ted C wrote:
> this can't happen the instance of check above would fail for null

Needed to make findbugs not complain.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java:219:
if (activity.getTabModelSelector().getModelForTabId(id) != null) return
activity;
On 2015/07/21 21:06:46, Ted C wrote:
> I would probably use getTabById instead of getModelForTabId...for no good
reason
> other than readability.

Done.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/AsyncDocumentLaunchSerializer.java
(right):

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/AsyncDocumentLaunchSerializer.java:24:
public class AsyncDocumentLaunchSerializer {
On 2015/07/21 21:06:46, Ted C wrote:
> I would drop Serializer and just make this the AsyncDocumentLauncher.

Done.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/AsyncDocumentLaunchSerializer.java:31:
private static final int MAX_WAIT = 3000;
On 2015/07/21 21:06:47, Ted C wrote:
> add _MS here and below to POLLING_DELAY

Done.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/AsyncDocumentLaunchSerializer.java:91:
private LaunchRunnable mCurrentRunnable = null;
On 2015/07/21 21:06:47, Ted C wrote:
> = null is the default, so we can leave that off.

Done.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java
(right):

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java:106:
return TextUtils.equals("Popup #3", selector.getCurrentTab().getTitle());
On 2015/07/21 21:06:47, Ted C wrote:
> Was it wrong checking for Popup #2 before? ahh ha, you changed the test file. 
> nvm

Yeah, I wanted to make sure we'd trigger the bug without the fix.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java:108: },
7500, 1000));
On 2015/07/21 21:06:47, Ted C wrote:
> the second polling is quite large, why not just use
> CriteriaHelper.DEFAULT_POLLING_INTERVAL.  The 1sec will make this test slower
> than it needs to be.

Done.

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java:114:
loadUrl(POPUP_HTML_FILENAME);
On 2015/07/21 21:06:47, Ted C wrote:
> popups always load in the background right?  This will load in the still
> visible/original activity?  Just making sure getActivity() is actually
checking
> the proper thing.

No, they seem to load in the foreground or background, depending on whether the
user permitted them last time.  Haven't found a rhyme or reason.

getActivity() is fine in this instance, though, since it doesn't actually matter
what window triggers the popups to appear again.  You could switch to
selector.getCurrentTab().loadUrl() and it should still work fine, if you wanted.

[Ted C, 2015-07-21 22:07:42]

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
File chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java
(right):

https://codereview.chromium.org/1229853006/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/PopupTest.java:114:
loadUrl(POPUP_HTML_FILENAME);
On 2015/07/21 21:30:12, dfalcantara wrote:
> On 2015/07/21 21:06:47, Ted C wrote:
> > popups always load in the background right?  This will load in the still
> > visible/original activity?  Just making sure getActivity() is actually
> checking
> > the proper thing.
> 
> No, they seem to load in the foreground or background, depending on whether
the
> user permitted them last time.  Haven't found a rhyme or reason.
> 
> getActivity() is fine in this instance, though, since it doesn't actually
matter
> what window triggers the popups to appear again.  You could switch to
> selector.getCurrentTab().loadUrl() and it should still work fine, if you
wanted.

I would hope that we wouldn't open popups for background tabs, but that isn't
in the scope here.

As long as loadUrl and getNumInfobarsShowing() calls on the same tab, it should
be fine.

[Ted C, 2015-07-21 22:51:54]

lgtm

[gone, 2015-07-21 22:53:42]

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

[commit-bot: I haz the power, 2015-07-21 22:54:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1229853006/80001

[Maria, 2015-07-21 23:00:18]

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

[Maria, 2015-07-21 23:00:18]

lgtm

[gone, 2015-07-21 23:13:27]

The CQ bit was unchecked by dfalcantara@chromium.org

[gone, 2015-07-21 23:13:32]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2015-07-21 23:14:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1229853006/80001

[commit-bot: I haz the power, 2015-07-22 00:43:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-22 00:45:07]

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