Mac: Added a delay before panels go to minimized state from title-only state.

Mac: Added a delay before panels go to minimized state from title-only state.

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

[jianli, 2011-10-28 01:04:11]

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

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:36: #if defined (OS_MACOSX)
Extra empty space.

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:425: ++sLastTaskIndex),
Where do we reset sLastTaskIndex? What if increasing sLastTaskIndex causes
overflow?

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:433: // if the panels are not go back
to minimized state too fast. For example,
are not => do not

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:439:
MessageLoop::current()->PostDelayedTask(
This seems to  be similar to the above PostDelayedTask code. Can they be merged?

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:452: void
PanelManager::DelayedBringUpOrDownTitlebarsCheck(int taskIndex) {
taskIndex => task_index

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:453: // task was already processed or
cancelled - bail out.
task needs to be capitalized.

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:202: static int sLastTaskIndex;
The variable name does not follow the coding style guideline. In addition, can
we avoid using the static integer?

[Dmitry Titov, 2011-10-28 20:58:23]

Updated. Please take another look!

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

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:36: #if defined (OS_MACOSX)
On 2011/10/28 01:04:11, jianli wrote:
> Extra empty space.

Done.

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:425: ++sLastTaskIndex),
On 2011/10/28 01:04:11, jianli wrote:
> Where do we reset sLastTaskIndex? What if increasing sLastTaskIndex causes
> overflow?

If we post a task every time user moves the mouse above/below target and user
does it every second, it will take 126 years to overflow 32-bit integer... I
think we can just ignore overflow.

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:433: // if the panels are not go back
to minimized state too fast. For example,
On 2011/10/28 01:04:11, jianli wrote:
> are not => do not

Done.

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:439:
MessageLoop::current()->PostDelayedTask(
On 2011/10/28 01:04:11, jianli wrote:
> This seems to  be similar to the above PostDelayedTask code. Can they be
merged?

Done.

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:452: void
PanelManager::DelayedBringUpOrDownTitlebarsCheck(int taskIndex) {
On 2011/10/28 01:04:11, jianli wrote:
> taskIndex => task_index

Done.

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:453: // task was already processed or
cancelled - bail out.
On 2011/10/28 01:04:11, jianli wrote:
> task needs to be capitalized.

Done.

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8341098/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:202: static int sLastTaskIndex;
On 2011/10/28 01:04:11, jianli wrote:
> The variable name does not follow the coding style guideline. In addition, can
> we avoid using the static integer?

Fixed style.
Tried to figure out simple way that would work w/o a static counter. Do you have
a good idea?

[jianli, 2011-10-28 21:31:33]

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

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:116:
content::NotificationService::current()->Notify(
Probably we do not need this notification since RunAllPending() seems to be
enough.

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

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:55: void
MoveMouseAndWiatForExpansionStateChange(Panel* panel,
Wiat => Wait

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:56: gfx::Point position) {
const gfx::Point&

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:406: int task_delay = 0;
Please add an empty line here.
Also, task_delay_time?

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:436: if (reduced_delays_for_testing_)
This sounds more like removed_delays_for_testing_

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:443: ++s_last_task_index),
I think we can avoid using this static variable in the following way:
  if (!delayed_titlebars_action_task_posted_) {
    MessageLoop::current()->PostDelayedTask(...);
    delayed_titlebars_action_task_posted_ = true;
  }

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:458: bool need_to_bring_titlebars_up =
(delayed_titlebar_action_ == BRING_UP);
need_to_bring_titlebars_up => need_to_bring_up_titlebars

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:460: // We are going to process the
task one way or another after this point.
This comment is kind of confusing. Why not removing it?

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:464: // mouse moved and does not call
for the titlebars change, cancel.
Comment here is a bit hard to understand. How about rephrasing like:
  // Check if the action is still needed based on the latest mouse position. The
user could move the mouse into the tracking area and then quickly move it out of
the area. In case of this, cancel the action.

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:209: void
DelayedBringUpOrDownTitlebarsCheck(int task_index);
This method should stay with all other methods.

[Dmitry Titov, 2011-10-29 00:04:56]

Another iteration! :-)

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

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:116:
content::NotificationService::current()->Notify(
On 2011/10/28 21:31:33, jianli wrote:
> Probably we do not need this notification since RunAllPending() seems to be
> enough.

Discussed offline. Decided to go with regualr pattern of waiting for explicit
notification rather then depend on RunAllPending().

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

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:55: void
MoveMouseAndWiatForExpansionStateChange(Panel* panel,
On 2011/10/28 21:31:33, jianli wrote:
> Wiat => Wait

Done.

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:406: int task_delay = 0;
On 2011/10/28 21:31:33, jianli wrote:
> Please add an empty line here.
> Also, task_delay_time?

made it task_delay_milliseconds.

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:436: if (reduced_delays_for_testing_)
On 2011/10/28 21:31:33, jianli wrote:
> This sounds more like removed_delays_for_testing_

renamed to remove_delays_for_testing_

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:443: ++s_last_task_index),
On 2011/10/28 21:31:33, jianli wrote:
> I think we can avoid using this static variable in the following way:
>   if (!delayed_titlebars_action_task_posted_) {
>     MessageLoop::current()->PostDelayedTask(...);
>     delayed_titlebars_action_task_posted_ = true;
>   }
>   

Replaced by using Cancel() on a task. Keep the pointer to the task now.

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:458: bool need_to_bring_titlebars_up =
(delayed_titlebar_action_ == BRING_UP);
On 2011/10/28 21:31:33, jianli wrote:
> need_to_bring_titlebars_up => need_to_bring_up_titlebars

Done

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:460: // We are going to process the
task one way or another after this point.
On 2011/10/28 21:31:33, jianli wrote:
> This comment is kind of confusing. Why not removing it?

Done.

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:464: // mouse moved and does not call
for the titlebars change, cancel.
On 2011/10/28 21:31:33, jianli wrote:
> Comment here is a bit hard to understand. How about rephrasing like:
>   // Check if the action is still needed based on the latest mouse position.
The
> user could move the mouse into the tracking area and then quickly move it out
of
> the area. In case of this, cancel the action.

Done.

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8341098/diff/4/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:209: void
DelayedBringUpOrDownTitlebarsCheck(int task_index);
On 2011/10/28 21:31:33, jianli wrote:
> This method should stay with all other methods.

Done.

[jennb, 2011-10-29 00:16:51]

Drive by...

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:37: const int
kMillisecondsBeforeCollapsingFromTitleonlyState = 3000;
TitleOnly ?

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:435: task_delay_milliseconds =
kMillisecondsBeforeCollapsingFromTitleonlyState;
Should this be += in case we also need to wait for
kMaxMillisecondsWaitForBottomBarVisibilityChange?

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:445: bring_up_down_task_ =
NewRunnableMethod(
Why the change from method_factor_? Just asking for my own educational purposes.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:166: void
DelayedBringUpOrDownTitlebarsCheck();
If old comment is no longer correct, could you put a new comment here?

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:230:
DISABLE_RUNNABLE_METHOD_REFCOUNT(PanelManager);
Could you add comment explaining why we need this?

[jianli, 2011-10-29 00:19:18]

LGTM.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:442: if (bring_up_down_task_)
Please comment why we need to cancel the task.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:209: CancelableTask*
bring_up_down_task_;  // Owned by MessageLoop after posting.
For consistency, perhaps we can rename it something like titlebar_action_task_,
or bring_up_or_down_titlebars_task_.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:210:
ScopedRunnableMethodFactory<PanelManager> method_factory_;
Seems that this is not needed any more.

Also, do we still need to make PanelManager ref counted?

[Dmitry Titov, 2011-10-29 00:41:58]

And another one!

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:37: const int
kMillisecondsBeforeCollapsingFromTitleonlyState = 3000;
On 2011/10/29 00:16:51, jennb wrote:
> TitleOnly ?

Done.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:435: task_delay_milliseconds =
kMillisecondsBeforeCollapsingFromTitleonlyState;
On 2011/10/29 00:16:51, jennb wrote:
> Should this be += in case we also need to wait for
> kMaxMillisecondsWaitForBottomBarVisibilityChange?

Not sure (there is no TitleonlyStata delay for Windows and no autohide support
for Mac, so it's hard to tell, since both delays are empirical. Added TODO.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:442: if (bring_up_down_task_)
On 2011/10/29 00:19:18, jianli wrote:
> Please comment why we need to cancel the task.

Done.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:445: bring_up_down_task_ =
NewRunnableMethod(
On 2011/10/29 00:16:51, jennb wrote:
> Why the change from method_factor_? Just asking for my own educational
purposes.

metod_factory does not give the task back. it manages them inside, making sure
they are cancelled in destruction time. I need to call Cancel() explicitly  so I
need to use CancelableTask directly and Cancel it also in the ~PanelManager.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:166: void
DelayedBringUpOrDownTitlebarsCheck();
On 2011/10/29 00:16:51, jennb wrote:
> If old comment is no longer correct, could you put a new comment here?

Done.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:209: CancelableTask*
bring_up_down_task_;  // Owned by MessageLoop after posting.
On 2011/10/29 00:19:18, jianli wrote:
> For consistency, perhaps we can rename it something like
titlebar_action_task_,
> or bring_up_or_down_titlebars_task_.

Done.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:210:
ScopedRunnableMethodFactory<PanelManager> method_factory_;
On 2011/10/29 00:19:18, jianli wrote:
> Seems that this is not needed any more.
> 
> Also, do we still need to make PanelManager ref counted?

Removed factory and refcounting.
Added DISABLE_RUNNABLE_METHOD_REFCOUNT(PanelManager) to define empty
AddRef/Release for RunnableMethod impl.

http://codereview.chromium.org/8341098/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:230:
DISABLE_RUNNABLE_METHOD_REFCOUNT(PanelManager);
On 2011/10/29 00:16:51, jennb wrote:
> Could you add comment explaining why we need this?

Done.

[Dmitry Titov, 2011-10-31 05:50:38]

Jian,
I disabled a part of MaximizeRestore Test in win-specific browser test (see the
last upload here) because it doesn't work now. The problem is that the fix I did
to prevent minimizing panels if mouse briefly went into trackign area and
briefly exited it now prevents the test from working because test is using
PanelManager::BringUpOrDownTitlebars() directly and that does not update
PanelManager::are_titlebars_up_.
I didn't see a quick obvious fix so I disabled it and will talk to you on Monday
on how to fix this.