Improve panel tests by properly waiting for expected conditions.

Improve panel tests by properly waiting for expected conditions.

Pulled logic from PanelActiveStateObserver into a base class for reuse.
Updated ActivatePanelOrTabbedWindow to apply to browserless panels.
Fixed DetachedPanelBrowserTest.DrawAttentionResetOnActivate to actually create detached panels.
Added guard in Panel::SetPanelBounds() to check if there is no change. Native layer doesn't always check (anymore).

TBR=thakis
BUG=140971, 145428, 145740, 147536, 147643
TEST=tests adjusted

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156704

[jennb, 2012-09-13 00:04:13]

Try jobs are looking good so far. I'll run a few more linux ones before deciding
this patch makes a difference in flaky tests. Ok to review in meanwhile.

[jennb, 2012-09-13 00:45:34]

Additional notes to help with review:

https://chromiumcodereview.appspot.com/10914242/diff/1006/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10914242/diff/1006/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_browsertest.cc:362: if
(panel->GetBounds().height() < panel->TitleOnlyHeight())
This test may have been flaky due to GTK returning something other than expected
title height in TitleOnlyHeight() if window has not been fully allocated yet.
Made change in PanelGtk::TitleOnlyHeight() that should help this test actualy
Wait() here.

https://chromiumcodereview.appspot.com/10914242/diff/1006/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_gtk.cc (right):

https://chromiumcodereview.appspot.com/10914242/diff/1006/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_gtk.cc:954: LOG(WARNING) << "Checking title
height before window allocated";
I intentionally left this log msg in to see if this ever gets called before
window is allocated. Will help track AutoResize() failure if it still flakes.

[Dmitry Titov, 2012-09-13 18:19:00]

https://chromiumcodereview.appspot.com/10914242/diff/1006/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10914242/diff/1006/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_browsertest.cc:893: Panel* panel1 =
CreatePanel("Panel1");
I wonder why do we need panel1 here?

https://chromiumcodereview.appspot.com/10914242/diff/1006/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_gtk.cc (right):

https://chromiumcodereview.appspot.com/10914242/diff/1006/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_gtk.cc:954: LOG(WARNING) << "Checking title
height before window allocated";
On 2012/09/13 00:45:34, jennb wrote:
> I intentionally left this log msg in to see if this ever gets called before
> window is allocated. Will help track AutoResize() failure if it still flakes.

Could you please move this note into a comment for this LOG? It'd be easier to
figure out why it is here later.

https://chromiumcodereview.appspot.com/10914242/diff/1006/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_gtk.cc:955: GtkAllocation allocation;
Do we actually need this code? It seems it should not be used in normal
conditions, right? Why not just NOTREACHED("Checking title height before window
was allocated")?

[jennb, 2012-09-13 18:40:48]

http://codereview.chromium.org/10914242/diff/1006/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_gtk.cc (right):

http://codereview.chromium.org/10914242/diff/1006/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_gtk.cc:955: GtkAllocation allocation;
On 2012/09/13 18:19:00, Dmitry Titov wrote:
> Do we actually need this code? It seems it should not be used in normal
> conditions, right? Why not just NOTREACHED("Checking title height before
window
> was allocated")?

Changed to NOTREACHED using the log msg.

[Dmitry Titov, 2012-09-13 18:51:58]

Very nice fixes. Hope they'll kill the flakiness!
LGTM.

[commit-bot: I haz the power, 2012-09-13 18:55:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10914242/8003

[commit-bot: I haz the power, 2012-09-13 18:55:21]

Presubmit check for 10914242-8003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.5s to calculate.

[Dmitry Titov, 2012-09-13 23:06:21]

still lgtm, with a comment:

https://chromiumcodereview.appspot.com/10914242/diff/7030/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10914242/diff/7030/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_browsertest.cc:453:
chrome::NOTIFICATION_PANEL_STRIP_UPDATED,
Should this too be modified?

[jennb, 2012-09-13 23:10:39]

https://chromiumcodereview.appspot.com/10914242/diff/7030/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10914242/diff/7030/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_browsertest.cc:453:
chrome::NOTIFICATION_PANEL_STRIP_UPDATED,
On 2012/09/13 23:06:21, Dmitry Titov wrote:
> Should this too be modified?

This should only result in one strip update here so we don't need to switch to
watching for a stream of strip updates.

[commit-bot: I haz the power, 2012-09-13 23:20:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10914242/7030

[commit-bot: I haz the power, 2012-09-13 23:32:05]

Retried try job too often for step(s) interactive_ui_tests, jingle_unittests,
gpu_unittests, base_unittests, sync_integration_tests, sql_unittests,
content_unittests, safe_browsing_tests, ipc_tests, cacheinvalidation_unittests,
remoting_unittests, browser_tests, net_unittests, media_unittests, check_deps,
crypto_unittests, unit_tests, nacl_integration, content_browsertests,
printing_unittests, sandbox_linux_unittests, sync_unit_tests

[commit-bot: I haz the power, 2012-09-14 00:44:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10914242/35

[jennb, 2012-09-14 01:02:47]

On 2012/09/14 00:44:58, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/jennb%40chromium.org/10914242/35

FYI - landed this manually due to CQ having problems applying the patch. Seems
to be a bug that affects others too.