Restored tabs should set their initial visibility

Restored tabs should set their initial visibility

Follow on for https://src.chromium.org/viewvc/chrome?revision=218548
which fixed various background tab issues related to visibility (see
for example bugs 118269, 142061, 155365 and 272962) by introducing a
WebContents initially_hidden creation-time flag.

Restored tabs don't currently set that flag, and that causes various
visibility issues (see bug). Fix that by ensuring that restored tabs
set their initially_hidden creation-time flag.

Note the delegated interface to tab restore service caused the error
so add tests for that: a direct test of the delegate, and a test for
a delegate user (recent tabs menu).

TEST=browser_tests --gtest_filter="*TabDisposition" 
BUG=358517
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266466

[sky, 2014-04-07 13:13:28]

How about some test coverage?

[Noel Gordon, 2014-04-07 14:12:29]

I did try to write a test.  I was unable to reproduce the bug in the session
restore test harness.

[sky, 2014-04-07 14:41:09]

This code is run when restoring a closed tab. That is, don't exit the
browser but instead close a tab then restore it (control-shift-t).

  -Scott

On Mon, Apr 7, 2014 at 7:12 AM,  <noel@chromium.org> wrote:
> I did try to write a test.  I was unable to reproduce the bug in the session
> restore test harness.
>
>
> https://codereview.chromium.org/227043004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Noel Gordon, 2014-04-10 02:34:44]

Thanks, looked over keyboard-based tests and an in crbug.  Mac won't
synthesize cmd-shift-t in test, many flaky / disabled keyboard tests
in chrome.

> This code is run when restoring a closed tab.

Yes over a delegated interface to the tab restore service.  That path
is not used by the session restore code for "Start where I left off".

The delegated tab restore interface needs a test:
 - added a test for that.

The recent tabs menu needs a test:
 - added a test that invokes the recent tabs sub-menu directly.
 - avoids the keyboard event test code flakiness for now.

Tests fail on mac / win without my patch, and pass with my patch.

[Noel Gordon, 2014-04-16 03:08:20]

Anything more I need do here?

[sky, 2014-04-16 16:42:05]

Why is this test in interactive_ui_tests? The file name should end in
interactive_uitest or browsertest.

https://codereview.chromium.org/227043004/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_tab_restore_uitest.cc (right):

https://codereview.chromium.org/227043004/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_tab_restore_uitest.cc:21: typedef InProcessBrowserTest
TabRestoreTest;
Make this match your file name, eg BrowserTabRestoreTest.

https://codereview.chromium.org/227043004/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_tab_restore_uitest.cc:35:
base::PlatformThread::YieldCurrentThread();
Don't do this. Add observers to wait for the condition you want.

[Noel Gordon, 2014-04-19 03:20:00]

On 2014/04/16 16:42:05, sky wrote:
> Why is this test in interactive_ui_tests? The file name should end in
> interactive_uitest or browsertest.

Moved it to browser_tests, renamed file.
 
https://codereview.chromium.org/227043004/diff/20001/chrome/browser/ui/browse...
> chrome/browser/ui/browser_tab_restore_uitest.cc:21: typedef
InProcessBrowserTest
> TabRestoreTest;
> Make this match your file name, eg BrowserTabRestoreTest.

Done.

https://codereview.chromium.org/227043004/diff/20001/chrome/browser/ui/browse...
> chrome/browser/ui/browser_tab_restore_uitest.cc:35:
> base::PlatformThread::YieldCurrentThread();
> Don't do this. Add observers to wait for the condition you want.

Removed. content::ExecuteScriptAndExtractBool is the observer.

[sky, 2014-04-21 15:41:13]

https://codereview.chromium.org/227043004/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_tabrestore_browsertest.cc (right):

https://codereview.chromium.org/227043004/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_tabrestore_browsertest.cc:30: const char kGetReadyJS[]
= "window.domAutomationController.send("
Isn't there a better way to wait for this rather than continually executing
script?

[Noel Gordon, 2014-04-22 14:24:35]

How about sending the ready state from the test page script and detecting that
using content::DOMMessageQueue's message waiter?

[sky, 2014-04-22 16:04:16]

Are you sure there isn't a notification for this already on
WebContentsObserver?

On Tue, Apr 22, 2014 at 7:24 AM,  <noel@chromium.org> wrote:
> How about sending the ready state from the test page script and detecting
> that
> using content::DOMMessageQueue's message waiter?
>
>
> https://codereview.chromium.org/227043004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Noel Gordon, 2014-04-24 11:09:47]

On 2014/04/22 16:04:16, sky wrote:
> Are you sure there isn't a notification for this already on
> WebContentsObserver?

Yeap, I'm sure there's no such notification. content::DOMMessageQueue
seems to handle it well though.

[Noel Gordon, 2014-04-24 14:12:14]

content::DOMMessageQueue also simplifies the code; new patch uploaded.

[sky, 2014-04-24 16:24:19]

LGTM

[Noel Gordon, 2014-04-26 08:29:19]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2014-04-26 08:29:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noel@chromium.org/227043004/140001

[commit-bot: I haz the power, 2014-04-26 13:53:59]

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

[commit-bot: I haz the power, 2014-04-26 13:54:00]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Noel Gordon, 2014-04-26 15:19:54]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2014-04-26 15:20:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noel@chromium.org/227043004/140001

[commit-bot: I haz the power, 2014-04-28 07:17:50]

Change committed as 266466