Fix Panel to always detect when a tab is added.

Fix Panel to always detect when a tab is added.

This solves the problem when chrome.windows.create() is used to create a panel with a tabid. Panel was watching for NOTIFICATION_TAB_ADDED, which is only sent upon browser navigation. As there is no navigation when existing tab contents are used, the Panel does not set up auto-resize in this scenario.

BUG=None
TEST=PanelBrowserTest.CreateWithExistingContents

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110179

[jennb, 2011-11-11 22:14:47]

This solves the problem when chrome.windows.create() is used to create a panel
with a tabid. NOTIFICATION_TAB_ADDED is not sent in that case.

[Dmitry Titov, 2011-11-11 22:21:18]

Drive-by:

Could you please add a description of "why" in addition to "what". It's not
clear from the cursory look what this patch is fixing...

[jianli, 2011-11-11 22:31:23]

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:48: // Invoked by native panel so do not
access native_panel_ here.
Could you please consider adding "destructor" after "native panel" to make this
comment more clear?

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:542: int index,
Spacing.

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:551: registrar_.RemoveAll();
Why do we need to call RemoveAll first? Can TabInsertedAt be called multiple
times?

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:1413: // Swap tab contents over to
the panel from the browser.
browser => tabbed browser

[jennb, 2011-11-11 22:35:40]

Done. HTH.

On Fri, Nov 11, 2011 at 2:21 PM, <dimich@chromium.org> wrote:

> Drive-by:
>
> Could you please add a description of "why" in addition to "what". It's not
> clear from the cursory look what this patch is fixing...
>
>
http://codereview.chromium.**org/8539025/<http://codereview.chromium.org/8539...
>

[Dmitry Titov, 2011-11-11 22:40:32]

On 2011/11/11 22:35:40, jennb wrote:
> Done. HTH.
Thanks!

[jennb, 2011-11-11 22:42:50]

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:48: // Invoked by native panel so do not
access native_panel_ here.
On 2011/11/11 22:31:23, jianli wrote:
> Could you please consider adding "destructor" after "native panel" to make
this
> comment more clear?

Done.

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:542: int index,
On 2011/11/11 22:31:23, jianli wrote:
> Spacing.

Done.

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:551: registrar_.RemoveAll();
On 2011/11/11 22:31:23, jianli wrote:
> Why do we need to call RemoveAll first? Can TabInsertedAt be called multiple
> times?

When there's a new tab, I don't want notifications about the old tab anymore.

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8539025/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:1413: // Swap tab contents over to
the panel from the browser.
On 2011/11/11 22:31:23, jianli wrote:
> browser => tabbed browser

Done.

[prasadt, 2011-11-11 23:08:39]

http://codereview.chromium.org/8539025/diff/4003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel.cc (left):

http://codereview.chromium.org/8539025/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.cc:178: #endif
I'm making this change to fix the bug mentioned in the comments, for which you
suggested adding a test for before check-in. Please wait for the test to be in
before submitting. Thanks.

[jianli, 2011-11-11 23:15:21]

lgtm

[Paweł Hajdan Jr., 2011-11-14 12:24:33]

Drive-by with testing comments.

http://codereview.chromium.org/8539025/diff/10/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8539025/diff/10/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browsertest.cc:860: while (panel->GetBounds() ==
initial_bounds) {
Ouch, this is generally bad style (running MessageLoop in a loop). I didn't
notice it before, but I'd appreciate it if you could convert it to wait for an
event in a follow-up (I can explain the rationale off-rietveld if you'd like).

http://codereview.chromium.org/8539025/diff/10/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browsertest.cc:861:
MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
By the way, I think this QuitTask is redundant with RunAllPending, _unless_ you
do some trickery (like tasks being posted after this QuitTasks), in which case
you have one more point for converting this.

http://codereview.chromium.org/8539025/diff/10/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browsertest.cc:1448: while (panel->GetBounds() ==
initial_bounds) {
Ah, and here the bad code is being repeated. This is new code, so pretty please
use waiting for an event here.

[jennb, 2011-11-15 02:08:50]

Cleaned up. Thanks for catching that!

On 2011/11/14 12:24:33, Paweł Hajdan Jr. wrote:
> Drive-by with testing comments.
> 
>
http://codereview.chromium.org/8539025/diff/10/chrome/browser/ui/panels/panel...
> File chrome/browser/ui/panels/panel_browsertest.cc (right):
> 
>
http://codereview.chromium.org/8539025/diff/10/chrome/browser/ui/panels/panel...
> chrome/browser/ui/panels/panel_browsertest.cc:860: while (panel->GetBounds()
==
> initial_bounds) {
> Ouch, this is generally bad style (running MessageLoop in a loop). I didn't
> notice it before, but I'd appreciate it if you could convert it to wait for an
> event in a follow-up (I can explain the rationale off-rietveld if you'd like).
> 
>
http://codereview.chromium.org/8539025/diff/10/chrome/browser/ui/panels/panel...
> chrome/browser/ui/panels/panel_browsertest.cc:861:
> MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
> By the way, I think this QuitTask is redundant with RunAllPending, _unless_
you
> do some trickery (like tasks being posted after this QuitTasks), in which case
> you have one more point for converting this.
> 
>
http://codereview.chromium.org/8539025/diff/10/chrome/browser/ui/panels/panel...
> chrome/browser/ui/panels/panel_browsertest.cc:1448: while (panel->GetBounds()
==
> initial_bounds) {
> Ah, and here the bad code is being repeated. This is new code, so pretty
please
> use waiting for an event here.

[Paweł Hajdan Jr., 2011-11-15 08:54:04]

Excellent, code I commented in the drive-by LGTM. Thank you!