Delete ash-based code for Windows drag-and-drop graphics.

Cleanup ash-based code for Windows drag-and-drop graphics.

This moves the windows-specific implementation into the window
OSExchangeData::Provider. Since there's no ash that code does
not need to handle both anymore.

On windows+mus we use a OSExchangeDataProviderMus in the UI
process instead, which will do the right thing (the same thing
that drag_utils used to do instead of calling to the
OSExchangeData::Provider).

Changes the return type of GetDrawImage{Offset} to be more
friendly to implementations that do not return a ref to a
member variable.

R=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2792183002
Cr-Commit-Position: refs/heads/master@{#465653}
Committed: https://chromium.googlesource.com/chromium/src/+/e100d023fafad14a1a1eff16c68c8328c4a78770

[danakj, 2017-04-03 22:34:02]

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
File ui/base/dragdrop/drag_utils_win.cc (left):

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
ui/base/dragdrop/drag_utils_win.cc:26: static void
SetDragImageOnDataObject(HBITMAP hbitmap,
Since these have one caller, I collapsed them all together. I felt like this
made things more clear and I didn't need to do things like build gfx::Size to
pass here. It didn't seem these were providing much now that we don't have to
handle ash as a fallback.

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
ui/base/dragdrop/drag_utils_win.cc:43: // Blit the contents of the canvas to a
new HBITMAP. It is the caller's
I read MSDN for a while and I think this comment is wrong (the code does not
handle deleting bits which I think is right). I wrote a new comment.

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
File ui/base/dragdrop/drag_utils_win.cc (right):

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
ui/base/dragdrop/drag_utils_win.cc:47: base::win::ScopedGetDC screen_dc(NULL);
I scoped this DC to just be around CreateDIBSection. I think this is right but
please confirm.  It used to scope around the memcpy to bits also.

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/os_exchang...
File ui/base/dragdrop/os_exchange_data.h (right):

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/os_exchang...
ui/base/dragdrop/os_exchange_data.h:133: #if (defined(USE_AURA) &&
!defined(OS_WIN)) || defined(OS_MACOSX)
I took them out of the interface on WIN which I think is what we want.. bots
will show..

[danakj, 2017-04-03 22:34:05]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 22:34:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 23:11:31]

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

[commit-bot: I haz the power, 2017-04-03 23:11:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Peter Kasting, 2017-04-04 00:31:14]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2017-04-04 00:31:14]

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
File ui/base/dragdrop/drag_utils_win.cc (right):

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
ui/base/dragdrop/drag_utils_win.cc:47: base::win::ScopedGetDC screen_dc(NULL);
On 2017/04/03 22:34:01, danakj wrote:
> I scoped this DC to just be around CreateDIBSection. I think this is right but
> please confirm.  It used to scope around the memcpy to bits also.

It makes me a little leery.  I suggest just not putting in this scoping; it
won't hurt anything, it might make it safer, it makes the code a little shorter,
and the scoping implies to the reader that it's important for correctness that
the locals be destroyed before the function ends, which isn't true.

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/os_exchang...
File ui/base/dragdrop/os_exchange_data.h (right):

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/os_exchang...
ui/base/dragdrop/os_exchange_data.h:133: #if (defined(USE_AURA) &&
!defined(OS_WIN)) || defined(OS_MACOSX)
On 2017/04/03 22:34:01, danakj wrote:
> I took them out of the interface on WIN which I think is what we want.. bots
> will show..

Maybe this whole #if should be USE_ASH instead?

[sadrul, 2017-04-04 04:16:14]

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/os_exchang...
File ui/base/dragdrop/os_exchange_data.h (right):

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/os_exchang...
ui/base/dragdrop/os_exchange_data.h:133: #if (defined(USE_AURA) &&
!defined(OS_WIN)) || defined(OS_MACOSX)
On 2017/04/04 00:31:14, Peter Kasting wrote:
> On 2017/04/03 22:34:01, danakj wrote:
> > I took them out of the interface on WIN which I think is what we want.. bots
> > will show..
> 
> Maybe this whole #if should be USE_ASH instead?

We do use this on desktop-linux. So maybe just (OS_LINUX || OS_MACOSX)

[danakj, 2017-04-04 15:25:33]

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
File ui/base/dragdrop/drag_utils_win.cc (right):

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
ui/base/dragdrop/drag_utils_win.cc:47: base::win::ScopedGetDC screen_dc(NULL);
On 2017/04/04 00:31:14, Peter Kasting wrote:
> On 2017/04/03 22:34:01, danakj wrote:
> > I scoped this DC to just be around CreateDIBSection. I think this is right
but
> > please confirm.  It used to scope around the memcpy to bits also.
> 
> It makes me a little leery.  I suggest just not putting in this scoping; it
> won't hurt anything, it might make it safer, it makes the code a little
shorter,
> and the scoping implies to the reader that it's important for correctness that
> the locals be destroyed before the function ends, which isn't true.

I like to scope things as tightly as possible, if I extend the scope to the end
of the function, the next time someone changes this they will say "does the DC
need to stay alive for InitializeFromBitmap()"? the same way I am wondering
about the memcpy. Letting the scope run longer than it needs to make the
relationships more ambiguous IMO. CreateDIBSection doesn't say that the hdc
needs to be kept alive.

http://www.undefined-value.info/hDC_lifetime.html says "You should call
ReleaseDC as soon as possible. I assume if it's holding onto the handle, that
it's not releasing the DC. DCs are a very prescious resource."

I found a number of examples of code that deletes the HDC right after making a
bitmap and then using the bitmap.

https://www-user.tu-chemnitz.de/~heha/petzold/ch14e.htm
          hdc = GetDC (hwnd) ;
          hdcMem  = CreateCompatibleDC (hdc) ;
          hBitmap = CreateCompatibleBitmap (hdc, cxBitmap, cyBitmap) ;
          ReleaseDC (hwnd, hdc) ;
          SelectObject (hdcMem, hBitmap) ;

http://stackoverflow.com/questions/12655700/read-pixels-from-1bpp-bitmap-crea...
access the bits after releasing the HDC.

https://social.msdn.microsoft.com/Forums/en-US/2c96d6c5-352d-4afe-9b13-b9f966...
returns the bits pointer after releasing the HDC.

So, I think this will work.

[danakj, 2017-04-04 15:33:05]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 15:33:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-04-04 15:33:37]

PTAL

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/os_exchang...
File ui/base/dragdrop/os_exchange_data.h (right):

https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/os_exchang...
ui/base/dragdrop/os_exchange_data.h:133: #if (defined(USE_AURA) &&
!defined(OS_WIN)) || defined(OS_MACOSX)
On 2017/04/04 04:16:14, sadrul wrote:
> On 2017/04/04 00:31:14, Peter Kasting wrote:
> > On 2017/04/03 22:34:01, danakj wrote:
> > > I took them out of the interface on WIN which I think is what we want..
bots
> > > will show..
> > 
> > Maybe this whole #if should be USE_ASH instead?
> 
> We do use this on desktop-linux. So maybe just (OS_LINUX || OS_MACOSX)

Done.

[commit-bot: I haz the power, 2017-04-04 16:10:44]

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

[commit-bot: I haz the power, 2017-04-04 16:10:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[danakj, 2017-04-04 17:34:05]

E:\b\c\b\win_clang\src\ui\aura\mus\os_exchange_data_provider_mus.h(91,57): 
error: only virtual member functions can be marked 'override'
                    const gfx::Vector2d& cursor_offset) override;
                                                        ^~~~~~~~

os_exchange_data_provider_mus.h is being used on windows. But it has this ash
stuff. What do?

[danakj, 2017-04-05 22:14:16]

On 2017/04/04 17:34:05, danakj wrote:
> E:\b\c\b\win_clang\src\ui\aura\mus\os_exchange_data_provider_mus.h(91,57): 
> error: only virtual member functions can be marked 'override'
>                     const gfx::Vector2d& cursor_offset) override;
>                                                         ^~~~~~~~
> 
> os_exchange_data_provider_mus.h is being used on windows. But it has this ash
> stuff. What do?

sadrul do you have any input here? I'm happy to dig more but not sure what to
look for really in terms of what we want. I'm not sure if you're expecting that
we'd be using the mus one on windows, when there is a win one too?

[danakj, 2017-04-05 22:16:27]

On 2017/04/05 22:14:16, danakj wrote:
> On 2017/04/04 17:34:05, danakj wrote:
> > E:\b\c\b\win_clang\src\ui\aura\mus\os_exchange_data_provider_mus.h(91,57): 
> > error: only virtual member functions can be marked 'override'
> >                     const gfx::Vector2d& cursor_offset) override;
> >                                                         ^~~~~~~~
> > 
> > os_exchange_data_provider_mus.h is being used on windows. But it has this
ash
> > stuff. What do?
> 
> sadrul do you have any input here? I'm happy to dig more but not sure what to
> look for really in terms of what we want. I'm not sure if you're expecting
that
> we'd be using the mus one on windows, when there is a win one too?

ui/aura/BUILD.gn has very few is_win exceptions, they're not really helpful to
me
understanding.

[Peter Kasting, 2017-04-05 23:18:47]

On 2017/04/04 17:34:05, danakj wrote:
> E:\b\c\b\win_clang\src\ui\aura\mus\os_exchange_data_provider_mus.h(91,57): 
> error: only virtual member functions can be marked 'override'
>                     const gfx::Vector2d& cursor_offset) override;
>                                                         ^~~~~~~~
> 
> os_exchange_data_provider_mus.h is being used on windows. But it has this ash
> stuff. What do?

If the claim is that the functions are only called for Ash, then the overrides
are also only called for Ash, and should be #ifdefed similarly to the base ones.

If these are actually being called in Windows under mus, then that would falsify
the original claim that these are Ash-only.

[Peter Kasting, 2017-04-05 23:19:10]

On 2017/04/04 15:25:33, danakj wrote:
>
https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
> File ui/base/dragdrop/drag_utils_win.cc (right):
> 
>
https://codereview.chromium.org/2792183002/diff/1/ui/base/dragdrop/drag_utils...
> ui/base/dragdrop/drag_utils_win.cc:47: base::win::ScopedGetDC screen_dc(NULL);
> On 2017/04/04 00:31:14, Peter Kasting wrote:
> > On 2017/04/03 22:34:01, danakj wrote:
> > > I scoped this DC to just be around CreateDIBSection. I think this is right
> but
> > > please confirm.  It used to scope around the memcpy to bits also.
> > 
> > It makes me a little leery.  I suggest just not putting in this scoping; it
> > won't hurt anything, it might make it safer, it makes the code a little
> shorter,
> > and the scoping implies to the reader that it's important for correctness
that
> > the locals be destroyed before the function ends, which isn't true.
> 
> I like to scope things as tightly as possible, if I extend the scope to the
end
> of the function, the next time someone changes this they will say "does the DC
> need to stay alive for InitializeFromBitmap()"? the same way I am wondering
> about the memcpy. Letting the scope run longer than it needs to make the
> relationships more ambiguous IMO. CreateDIBSection doesn't say that the hdc
> needs to be kept alive.
> 
> http://www.undefined-value.info/hDC_lifetime.html says "You should call
> ReleaseDC as soon as possible. I assume if it's holding onto the handle, that
> it's not releasing the DC. DCs are a very prescious resource."
> 
> I found a number of examples of code that deletes the HDC right after making a
> bitmap and then using the bitmap.
> 
> https://www-user.tu-chemnitz.de/~heha/petzold/ch14e.htm
>           hdc = GetDC (hwnd) ;
>           hdcMem  = CreateCompatibleDC (hdc) ;
>           hBitmap = CreateCompatibleBitmap (hdc, cxBitmap, cyBitmap) ;
>           ReleaseDC (hwnd, hdc) ;
>           SelectObject (hdcMem, hBitmap) ;
> 
>
http://stackoverflow.com/questions/12655700/read-pixels-from-1bpp-bitmap-crea...
> access the bits after releasing the HDC.
> 
>
https://social.msdn.microsoft.com/Forums/en-US/2c96d6c5-352d-4afe-9b13-b9f966...
> returns the bits pointer after releasing the HDC.
> 
> So, I think this will work.

Yep, I agree.  Thanks for this due diligence!  LGTM

[sadrul, 2017-04-06 04:39:31]

On 2017/04/05 22:16:27, danakj wrote:
> On 2017/04/05 22:14:16, danakj wrote:
> > On 2017/04/04 17:34:05, danakj wrote:
> > > E:\b\c\b\win_clang\src\ui\aura\mus\os_exchange_data_provider_mus.h(91,57):

> > > error: only virtual member functions can be marked 'override'
> > >                     const gfx::Vector2d& cursor_offset) override;
> > >                                                         ^~~~~~~~
> > > 
> > > os_exchange_data_provider_mus.h is being used on windows. But it has this
> ash
> > > stuff. What do?
> > 
> > sadrul do you have any input here? I'm happy to dig more but not sure what
to
> > look for really in terms of what we want. I'm not sure if you're expecting
> that
> > we'd be using the mus one on windows, when there is a win one too?
> 
> ui/aura/BUILD.gn has very few is_win exceptions, they're not really helpful to
> me
> understanding.

I think you can just update the USE_AURA to OS_LINUX in there, and that should
be sufficient?

[danakj, 2017-04-07 21:56:35]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2017-04-07 21:57:04]

On Thu, Apr 6, 2017 at 12:39 AM, <sadrul@chromium.org> wrote:

> On 2017/04/05 22:16:27, danakj wrote:
> > On 2017/04/05 22:14:16, danakj wrote:
> > > On 2017/04/04 17:34:05, danakj wrote:
> > > > E:\b\c\b\win_clang\src\ui\aura\mus\os_exchange_data_
> provider_mus.h(91,57):
>
> > > > error: only virtual member functions can be marked 'override'
> > > > const gfx::Vector2d& cursor_offset) override;
> > > > ^~~~~~~~
> > > >
> > > > os_exchange_data_provider_mus.h is being used on windows. But it
> has this
> > ash
> > > > stuff. What do?
> > >
> > > sadrul do you have any input here? I'm happy to dig more but not sure
> what
> to
> > > look for really in terms of what we want. I'm not sure if you're
> expecting
> > that
> > > we'd be using the mus one on windows, when there is a win one too?
> >
> > ui/aura/BUILD.gn has very few is_win exceptions, they're not really
> helpful to
> > me
> > understanding.
>
> I think you can just update the USE_AURA to OS_LINUX in there, and that
> should
> be sufficient?
>

Ooh right sorry, I shoulda thought of that. Thanks, done!

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-04-07 21:57:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 22:30:05]

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

[commit-bot: I haz the power, 2017-04-07 22:30:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sadrul, 2017-04-10 18:24:14]

lgtm

[danakj, 2017-04-12 17:24:24]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 17:25:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 18:03:52]

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

[commit-bot: I haz the power, 2017-04-12 18:03:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[danakj, 2017-04-18 15:32:15]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 15:32:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-04-18 15:54:24]

E:\b\c\goma_client/gomacc.exe
../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo
/showIncludes /FC @obj/ui/aura/aura/drag_drop_controller_mus.obj.rsp /c
../../ui/aura/mus/drag_drop_controller_mus.cc
/Foobj/ui/aura/aura/drag_drop_controller_mus.obj /Fd"obj/ui/aura/aura_cc.pdb"
E:\b\c\b\win_clang\src\ui\aura\mus\drag_drop_controller_mus.cc(161,24):  error:
no member named 'GetDragImage' in 'ui::OSExchangeData::Provider'
      *data.provider().GetDragImage().bitmap(),
       ~~~~~~~~~~~~~~~ ^
E:\b\c\b\win_clang\src\ui\aura\mus\drag_drop_controller_mus.cc(162,23):  error:
no member named 'GetDragImageOffset' in 'ui::OSExchangeData::Provider'
      data.provider().GetDragImageOffset(), drag_operations, mojo_source);
      ~~~~~~~~~~~~~~~ ^
2 errors generated.

[danakj, 2017-04-18 15:54:26]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 15:54:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-04-18 15:56:46]

On 2017/04/18 15:54:24, danakj wrote:
> E:\b\c\goma_client/gomacc.exe
> ../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo
> /showIncludes /FC @obj/ui/aura/aura/drag_drop_controller_mus.obj.rsp /c
> ../../ui/aura/mus/drag_drop_controller_mus.cc
> /Foobj/ui/aura/aura/drag_drop_controller_mus.obj /Fd"obj/ui/aura/aura_cc.pdb"
> E:\b\c\b\win_clang\src\ui\aura\mus\drag_drop_controller_mus.cc(161,24): 
error:
> no member named 'GetDragImage' in 'ui::OSExchangeData::Provider'
>       *data.provider().GetDragImage().bitmap(),
>        ~~~~~~~~~~~~~~~ ^
> E:\b\c\b\win_clang\src\ui\aura\mus\drag_drop_controller_mus.cc(162,23): 
error:
> no member named 'GetDragImageOffset' in 'ui::OSExchangeData::Provider'
>       data.provider().GetDragImageOffset(), drag_operations, mojo_source);
>       ~~~~~~~~~~~~~~~ ^
> 2 errors generated.

So mus on windows is trying to use these ash methods. Probably because mus is
being written assuming ash? Is that a bug?

https://cs.chromium.org/chromium/src/ui/aura/mus/drag_drop_controller_mus.cc?...

It's calling them to pass to window_tree_->PerformDragDrop().

[danakj, 2017-04-18 15:57:36]

If that isn't a bug then we need to define those methods for AURA I guess, not
LINUX. But the windows override should be NOTREACHED()? :/

[commit-bot: I haz the power, 2017-04-18 16:47:29]

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

[commit-bot: I haz the power, 2017-04-18 16:47:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Peter Kasting, 2017-04-18 17:02:29]

Mus assuming Ash sounds like a bug to me.  I would rope in sky/msw and ask one
of them.

[sadrul, 2017-04-18 17:03:35]

sadrul@chromium.org changed reviewers:
+ erg@chromium.org

[sadrul, 2017-04-18 17:03:35]

+erg@ for drag-and-drop code in mus, and how it is expected to work on Windows
(see https://codereview.chromium.org/2792183002/#msg36 etc. for context)

[Elliot Glaysher, 2017-04-18 18:27:39]

On 2017/04/18 17:03:35, sadrul wrote:
> +erg@ for drag-and-drop code in mus, and how it is expected to work on Windows
> (see https://codereview.chromium.org/2792183002/#msg36 etc. for context)

I'm pretty sure that GetDragImage(Offset) aren't ash methods; they're used on
desktop linux and desktop mac. I'm pretty sure the USE_AURA ifdefs in
os_exchange_data.h are correct and shouldn't be changed to OS_LINUX.

[danakj, 2017-04-18 18:51:17]

On Tue, Apr 18, 2017 at 2:27 PM, <erg@chromium.org> wrote:

> On 2017/04/18 17:03:35, sadrul wrote:
> > +erg@ for drag-and-drop code in mus, and how it is expected to work on
> Windows
> > (see https://codereview.chromium.org/2792183002/#msg36 etc. for context)
>
> I'm pretty sure that GetDragImage(Offset) aren't ash methods; they're used
> on
> desktop linux and desktop mac. I'm pretty sure the USE_AURA ifdefs in
> os_exchange_data.h are correct and shouldn't be changed to OS_LINUX.
>

That's why it was changed from AURA||MAC to LINUX||MAC to include desktop
linux and mac still. However windows does not use those methods. But mus is
trying to use them on windows. The windows code for those methods said it
was only there to support ash, but its presence seems to have allowed this
mus code to grow.

Are you saying drag_utils_win should be using these on windows when we're
running "mus" instead of doing the windows native stuff?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Elliot Glaysher, 2017-04-18 19:15:16]

On 2017/04/18 18:51:17, danakj wrote:
> On Tue, Apr 18, 2017 at 2:27 PM, <mailto:erg@chromium.org> wrote:
> 
> > On 2017/04/18 17:03:35, sadrul wrote:
> > > +erg@ for drag-and-drop code in mus, and how it is expected to work on
> > Windows
> > > (see https://codereview.chromium.org/2792183002/#msg36 etc. for context)
> >
> > I'm pretty sure that GetDragImage(Offset) aren't ash methods; they're used
> > on
> > desktop linux and desktop mac. I'm pretty sure the USE_AURA ifdefs in
> > os_exchange_data.h are correct and shouldn't be changed to OS_LINUX.
> >
> 
> That's why it was changed from AURA||MAC to LINUX||MAC to include desktop
> linux and mac still. However windows does not use those methods. But mus is
> trying to use them on windows. The windows code for those methods said it
> was only there to support ash, but its presence seems to have allowed this
> mus code to grow.
> 
> Are you saying drag_utils_win should be using these on windows when we're
> running "mus" instead of doing the windows native stuff?

Yes.

Let's say you have a mus client like views_examples or (we're getting there!)
chrome. It should make no calls to the native win32/x11/apple windowing server
and should instead call mojo methods on the cross platform mus windowing server,
which then performs the per-platform specific calls. Right now, this is
aspirational and there are still several native things left to convert, but
we're getting there one ui surface at a time. The end state is that ui code
should be sandboxable, and we can't do that today with manual access to the
filesystem and hwnd manipulation.

[danakj, 2017-04-18 19:20:29]

On 2017/04/18 19:15:16, Elliot Glaysher wrote:
> On 2017/04/18 18:51:17, danakj wrote:
> > On Tue, Apr 18, 2017 at 2:27 PM, <mailto:erg@chromium.org> wrote:
> > 
> > > On 2017/04/18 17:03:35, sadrul wrote:
> > > > +erg@ for drag-and-drop code in mus, and how it is expected to work on
> > > Windows
> > > > (see https://codereview.chromium.org/2792183002/#msg36 etc. for context)
> > >
> > > I'm pretty sure that GetDragImage(Offset) aren't ash methods; they're used
> > > on
> > > desktop linux and desktop mac. I'm pretty sure the USE_AURA ifdefs in
> > > os_exchange_data.h are correct and shouldn't be changed to OS_LINUX.
> > >
> > 
> > That's why it was changed from AURA||MAC to LINUX||MAC to include desktop
> > linux and mac still. However windows does not use those methods. But mus is
> > trying to use them on windows. The windows code for those methods said it
> > was only there to support ash, but its presence seems to have allowed this
> > mus code to grow.
> > 
> > Are you saying drag_utils_win should be using these on windows when we're
> > running "mus" instead of doing the windows native stuff?
> 
> Yes.
> 
> Let's say you have a mus client like views_examples or (we're getting there!)
> chrome. It should make no calls to the native win32/x11/apple windowing server
> and should instead call mojo methods on the cross platform mus windowing
server,
> which then performs the per-platform specific calls. Right now, this is
> aspirational and there are still several native things left to convert, but
> we're getting there one ui surface at a time. The end state is that ui code
> should be sandboxable, and we can't do that today with manual access to the
> filesystem and hwnd manipulation.

Ok cool. So then while I'm here, I don't want to leave the code as it was
though. It was calling CreateDIBSection() and if it failed, it would fall back
to SetDragImage().

It sounds to me like _mus.cc code should be able to call that method, but
_win.cc code should not (cuz that was for ash). Put another way, it seems like
mus-windows should not be calling SetDragImageOnDataObject() at all and just
call SetDragImage() directly? How would you fix this code?

[Elliot Glaysher, 2017-04-18 19:52:54]

On 2017/04/18 19:20:29, danakj wrote:
> Ok cool. So then while I'm here, I don't want to leave the code as it was
> though. It was calling CreateDIBSection() and if it failed, it would fall back
> to SetDragImage().

Yeah, that sounds like it technically works, but is pretty bogus and should be
changed.

> It sounds to me like _mus.cc code should be able to call that method, but
> _win.cc code should not (cuz that was for ash). Put another way, it seems like
> mus-windows should not be calling SetDragImageOnDataObject() at all and just
> call SetDragImage() directly? How would you fix this code?

I'd wrap it in a check to `aura::Env::mode()`:

```
if (Env::GetInstance()->mode() == Env::Mode::MUS) {
  // mus SetDragImage()
} else {
  // default hwnd stuff
}
```

[danakj, 2017-04-18 20:41:46]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 20:42:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-04-18 20:43:18]

danakj@chromium.org changed reviewers:
+ sky@chromium.org

[danakj, 2017-04-18 20:43:19]

OK Thanks.

It looks like OSExchangeDataProviderWin would only be used when you wanted the
native behaviour cuz we'd use the Mus provider on windows when we wanted the mus
behaviour. So we can use the type to provide that differentiation. I've deleted
drag_utils from ui/base and moved the windows behaviour into the
Native-Win-specific OSExchangeDataProviderWin.

PTAL sadrul. Also +sky for ash/ and any overall thoughts.

[danakj, 2017-04-18 20:44:48]

Description was changed from

==========
Delete ash-based code for Windows drag-and-drop graphics.

R=sadrul@chromium.org
==========

to

==========
Cleanup ash-based code for Windows drag-and-drop graphics.

This moves the windows-specific implementation into the window
OSExchangeData::Provider. Since there's no ash that code does
not need to handle both anymore.

On windows+mus we use a OSExchangeDataProviderMus instead which
will do the right thing (the same thing that drag_utils used to
do instead of calling to the OSExchangeData::Provider).

R=sadrul@chromium.org
==========

[danakj, 2017-04-18 20:45:18]

Description was changed from

==========
Cleanup ash-based code for Windows drag-and-drop graphics.

This moves the windows-specific implementation into the window
OSExchangeData::Provider. Since there's no ash that code does
not need to handle both anymore.

On windows+mus we use a OSExchangeDataProviderMus instead which
will do the right thing (the same thing that drag_utils used to
do instead of calling to the OSExchangeData::Provider).

R=sadrul@chromium.org
==========

to

==========
Cleanup ash-based code for Windows drag-and-drop graphics.

This moves the windows-specific implementation into the window
OSExchangeData::Provider. Since there's no ash that code does
not need to handle both anymore.

On windows+mus we use a OSExchangeDataProviderMus in the UI
process instead, which will do the right thing (the same thing
that drag_utils used to do instead of calling to the
OSExchangeData::Provider).

R=sadrul@chromium.org
==========

[commit-bot: I haz the power, 2017-04-18 20:47:30]

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

[commit-bot: I haz the power, 2017-04-18 20:47:31]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[danakj, 2017-04-18 21:41:11]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 21:41:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 21:48:40]

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

[commit-bot: I haz the power, 2017-04-18 21:48:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[danakj, 2017-04-18 21:59:07]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 21:59:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-04-18 22:32:57]

LGTM

[commit-bot: I haz the power, 2017-04-18 22:41:24]

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

[commit-bot: I haz the power, 2017-04-18 22:41:25]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[danakj, 2017-04-18 22:50:35]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 22:51:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 00:10:03]

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

[commit-bot: I haz the power, 2017-04-19 00:10:04]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-04-19 14:00:53]

PTAL sadrul, u can CQ if you're ok with it? Thanks!

[sadrul, 2017-04-19 17:04:50]

still lgtm

https://codereview.chromium.org/2792183002/diff/120001/ui/base/dragdrop/os_ex...
File ui/base/dragdrop/os_exchange_data.h (left):

https://codereview.chromium.org/2792183002/diff/120001/ui/base/dragdrop/os_ex...
ui/base/dragdrop/os_exchange_data.h:137: virtual const gfx::Vector2d&
GetDragImageOffset() const = 0;
Was this a deliberate change? Can you document the reason in the CL description?

[danakj, 2017-04-19 17:20:07]

Description was changed from

==========
Cleanup ash-based code for Windows drag-and-drop graphics.

This moves the windows-specific implementation into the window
OSExchangeData::Provider. Since there's no ash that code does
not need to handle both anymore.

On windows+mus we use a OSExchangeDataProviderMus in the UI
process instead, which will do the right thing (the same thing
that drag_utils used to do instead of calling to the
OSExchangeData::Provider).

R=sadrul@chromium.org
==========

to

==========
Cleanup ash-based code for Windows drag-and-drop graphics.

This moves the windows-specific implementation into the window
OSExchangeData::Provider. Since there's no ash that code does
not need to handle both anymore.

On windows+mus we use a OSExchangeDataProviderMus in the UI
process instead, which will do the right thing (the same thing
that drag_utils used to do instead of calling to the
OSExchangeData::Provider).

Changes the return type of GetDrawImage{Offset} to be more
friendly to implementations that do not return a ref to a
member variable.

R=sadrul@chromium.org
==========

[danakj, 2017-04-19 17:20:23]

Updated the message. Thanks!

https://codereview.chromium.org/2792183002/diff/120001/ui/base/dragdrop/os_ex...
File ui/base/dragdrop/os_exchange_data.h (left):

https://codereview.chromium.org/2792183002/diff/120001/ui/base/dragdrop/os_ex...
ui/base/dragdrop/os_exchange_data.h:137: virtual const gfx::Vector2d&
GetDragImageOffset() const = 0;
On 2017/04/19 17:04:50, sadrul wrote:
> Was this a deliberate change? Can you document the reason in the CL
description?

Yes, because the win one doesn't have anything to return, but this requires
non-local storage of an object to return. So it would have to make a static
variable or something weird for NOTREACHED() code.

[danakj, 2017-04-19 17:20:31]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-04-19 17:20:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2792183002/#ps120001
(title: "ashdrag: addheaders")

[commit-bot: I haz the power, 2017-04-19 17:21:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 17:26:51]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492622431117060,
"parent_rev": "76393b28bda6890a9da2b5c866ea8d4ac435a32f", "commit_rev":
"e100d023fafad14a1a1eff16c68c8328c4a78770"}

[commit-bot: I haz the power, 2017-04-19 17:27:01]

Description was changed from

==========
Cleanup ash-based code for Windows drag-and-drop graphics.

This moves the windows-specific implementation into the window
OSExchangeData::Provider. Since there's no ash that code does
not need to handle both anymore.

On windows+mus we use a OSExchangeDataProviderMus in the UI
process instead, which will do the right thing (the same thing
that drag_utils used to do instead of calling to the
OSExchangeData::Provider).

Changes the return type of GetDrawImage{Offset} to be more
friendly to implementations that do not return a ref to a
member variable.

R=sadrul@chromium.org
==========

to

==========
Cleanup ash-based code for Windows drag-and-drop graphics.

This moves the windows-specific implementation into the window
OSExchangeData::Provider. Since there's no ash that code does
not need to handle both anymore.

On windows+mus we use a OSExchangeDataProviderMus in the UI
process instead, which will do the right thing (the same thing
that drag_utils used to do instead of calling to the
OSExchangeData::Provider).

Changes the return type of GetDrawImage{Offset} to be more
friendly to implementations that do not return a ref to a
member variable.

R=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2792183002
Cr-Commit-Position: refs/heads/master@{#465653}
Committed:
https://chromium.googlesource.com/chromium/src/+/e100d023fafad14a1a1eff16c68c...
==========

[commit-bot: I haz the power, 2017-04-19 17:27:03]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e100d023fafad14a1a1eff16c68c...