Fix app list drag view being hidden on reparent when synchronous drag is setting up.

Fix app list drag view being hidden on reparent when synchronous drag is setting up.

This CL fixes an issue where reparenting would hide the folder grid view that
was hosting the drag view, causing the root level to get visible drag view. To
get around this, allow the folder grid view to tell the root grid view to show
its drag view until the synchronous drag sets up.

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

[tapted, 2014-05-01 05:54:46]

nice work figuring this one out - that's some awesome debugging!

I think it can be done without threading through an extra bit of state, so we
keep the synchronous drag stuff more cohesive.

What do you think of something like this?

https://codereview.chromium.org/265723003/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/265723003/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:498: synchronous_drag_ = NULL;
here,
    if (folder_delegate_)
      folder_delegate_->SetRootLevelDragViewHidden(true);

https://codereview.chromium.org/265723003/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:500: }
  } else {
    // The OS drag is not ready yet. If the root grid has a drag view because
    // a reparent has started, ensure it is visible.
    if (folder_delegate_)
      folder_delegate_->SetRootLevelDragViewHidden(false);
  }

[calamity, 2014-05-01 08:04:40]

https://codereview.chromium.org/265723003/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/265723003/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:498: synchronous_drag_ = NULL;
On 2014/05/01 05:54:47, tapted wrote:
> here,
>     if (folder_delegate_)
>       folder_delegate_->SetRootLevelDragViewHidden(true);

Done.

https://codereview.chromium.org/265723003/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.cc:500: }
On 2014/05/01 05:54:47, tapted wrote:
>   } else {
>     // The OS drag is not ready yet. If the root grid has a drag view because
>     // a reparent has started, ensure it is visible.
>     if (folder_delegate_)
>       folder_delegate_->SetRootLevelDragViewHidden(false);
>   }

Done.

[tapted, 2014-05-02 01:52:46]

lgtm

https://codereview.chromium.org/265723003/diff/10001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/265723003/diff/10001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:505: } else if
(!synchronous_drag_->running()) {
nit: I think the condition is redundant (i.e. can just be else {)

That is, because the Run() above would immediately run it, there's never a state
where running is true and CanRun is false.

(running && !CanRun)
  ==> (running && !(has_shortcut_path_ && !running_)
  ==> (running && (!has_shortcut_path  || running)
  ==> (running && !has_shortcut_path)
  ==> (has_shortcut_path && !has_shortcut_path)

(because running ==> CanRun was true at least once, requiring has_shortcut_path
to be true)

So paradox means !canrun ==> !running so this `if` is always true.

But my brain hurts now

[calamity, 2014-05-02 04:28:37]

https://codereview.chromium.org/265723003/diff/10001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/265723003/diff/10001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:505: } else if
(!synchronous_drag_->running()) {
On 2014/05/02 01:52:47, tapted wrote:
> nit: I think the condition is redundant (i.e. can just be else {)
> 
> That is, because the Run() above would immediately run it, there's never a
state
> where running is true and CanRun is false.
> 
> (running && !CanRun)
>   ==> (running && !(has_shortcut_path_ && !running_)
>   ==> (running && (!has_shortcut_path  || running)
>   ==> (running && !has_shortcut_path)
>   ==> (has_shortcut_path && !has_shortcut_path)
> 
> (because running ==> CanRun was true at least once, requiring
has_shortcut_path
> to be true)
> 
> So paradox means !canrun ==> !running so this `if` is always true.
> 
> But my brain hurts now

Ah, true.

[calamity, 2014-05-05 01:00:30]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-05-05 01:00:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/265723003/30001

[commit-bot: I haz the power, 2014-05-05 01:02:24]

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

[commit-bot: I haz the power, 2014-05-05 01:02:24]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium

[calamity, 2014-05-05 02:34:09]

On 2014/05/02 01:52:46, tapted wrote:
> lgtm
> 
>
https://codereview.chromium.org/265723003/diff/10001/ui/app_list/views/apps_g...
> File ui/app_list/views/apps_grid_view.cc (right):
> 
>
https://codereview.chromium.org/265723003/diff/10001/ui/app_list/views/apps_g...
> ui/app_list/views/apps_grid_view.cc:505: } else if
> (!synchronous_drag_->running()) {
> nit: I think the condition is redundant (i.e. can just be else {)
> 
> That is, because the Run() above would immediately run it, there's never a
state
> where running is true and CanRun is false.
> 
> (running && !CanRun)
>   ==> (running && !(has_shortcut_path_ && !running_)
>   ==> (running && (!has_shortcut_path  || running)
>   ==> (running && !has_shortcut_path)
>   ==> (has_shortcut_path && !has_shortcut_path)
> 
> (because running ==> CanRun was true at least once, requiring
has_shortcut_path
> to be true)
> 
> So paradox means !canrun ==> !running so this `if` is always true.
> 
> But my brain hurts now

Wait! It seems that since the OnDragSourceMoved() calls UpdateDrag(), this is
called again during the synchronous drag! The condition is necessary after all
=X

[tapted, 2014-05-05 03:43:31]

On 2014/05/05 02:34:09, calamity wrote:
> On 2014/05/02 01:52:46, tapted wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/265723003/diff/10001/ui/app_list/views/apps_g...
> > File ui/app_list/views/apps_grid_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/265723003/diff/10001/ui/app_list/views/apps_g...
> > ui/app_list/views/apps_grid_view.cc:505: } else if
> > (!synchronous_drag_->running()) {
> > nit: I think the condition is redundant (i.e. can just be else {)
> > 
> > That is, because the Run() above would immediately run it, there's never a
> state
> > where running is true and CanRun is false.
> > 
> > (running && !CanRun)
> >   ==> (running && !(has_shortcut_path_ && !running_)
> >   ==> (running && (!has_shortcut_path  || running)
> >   ==> (running && !has_shortcut_path)
> >   ==> (has_shortcut_path && !has_shortcut_path)
> > 
> > (because running ==> CanRun was true at least once, requiring
> has_shortcut_path
> > to be true)
> > 
> > So paradox means !canrun ==> !running so this `if` is always true.
> > 
> > But my brain hurts now
> 
> Wait! It seems that since the OnDragSourceMoved() calls UpdateDrag(), this is
> called again during the synchronous drag! The condition is necessary after all
> =X

Oh man - yeah - I forgot this function ends up being recursive.. Damn
synchronous drag o_O

[calamity, 2014-05-05 06:13:40]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-05-05 06:13:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/265723003/50001

[commit-bot: I haz the power, 2014-05-05 13:11:09]

Change committed as 268157