Fix the HTML5 drag and drop demos on Linux and CrOS

Fix the HTML5 drag and drop demos at http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_draganddrop and http://html5demos.com/drag on Linux and CrOS

This CL makes the behavior of OSExchangeData::Provider::SetString() match that of Windows on Linux and CrOS

This CL also changes the string which is dragged out of the omnibox on Linux and CrOS to match the string which is dragged out from the omnibox on Windows.
On Linux and CrOS, with this CL, the dragged out string for URLs no longer includes 'http://' if 'http://' is not visible in the omnibox. For instance, if the user has navigated to 'www.random.org', the dragged out string is 'http://www.random.org' without this CL and 'www.random.org' with this CL.

BUG=355390
TEST=OSExchangeDataTest.URLAndString

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

[pkotwicz, 2014-03-23 21:18:03]

Ｓｃｏｔｔ， ＰＴＡＬ

[dcheng, 2014-03-24 20:28:34]

Is there anyone that depends on the current behavior? When I previously fixed
this for the non-Aura version of Windows
(http://src.chromium.org/viewvc/chrome?revision=62803&view=revision), I just
inverted the order things were set in, and it seems like we could do the same
thing in WebContentsViewAura.

[pkotwicz, 2014-03-24 23:15:32]

I do not know if anyone depends on the current behavior. The behavior on Windows
is unchanged. (And I assume that things are most likely to depend on the Windows
behavior)

I like this CL because it is easy to test.

[pkotwicz, 2014-03-25 01:55:20]

There might be a dependence in OmniboxViewViews::OnWriteDragData() I need to
learn more about autocomplete in order to find out if the dependence is real

[sky, 2014-03-25 16:37:25]

LGTM

[pkotwicz, 2014-06-20 04:20:06]

pkasting@, can you please take a look at the change in
OmniboxViewViews::OnWriteDragData().

I think that the change reflects the intention of the original code. This CL
does change the string which is dragged out from the omnibox on Windows but not
on CrOS/Linux.

I have thought a bit as to what the proper behavior of OSExchangeData::SetURL()
should be. I think that the correct behavior is that whichever data is set first
prevails.
This matches the behavior of OSExchangeDataProviderWin::SetURL() which adds data
types from most preferable to least preferable.

[Peter Kasting, 2014-06-20 20:45:37]

Your CL description seems to not match your comment to me (is Windows' behavior
changed or not?).  Also, you need to be more detailed about what behavioral
change you're making.  I can't sign off on this until I actually understand
practically what it does.  Certainly when I navigate to the links on the
associated bug here, the demos currently work on Windows.

https://codereview.chromium.org/208313009/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/208313009/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:947: // Setting the URL
also sets the string.
This comment needs to say why it's important that we don't call SetString()
anyway in this case, since it implies doing so would be harmless.

https://codereview.chromium.org/208313009/diff/50001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/208313009/diff/50001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:377: // want to prefer a
custom string over the URL string so we add it first.
This comment lies.  With your changes, SetURL() _doesn't_ set the string, at
least in this case.  How about this as a comment:

Call SetString() before SetURL() when we actually have a custom string. 
SetURL() will itself do SetString() when a string hasn't been set yet, but we
want to prefer drop_data.text.string() over the URL string if it exists.

[pkotwicz, 2014-06-20 22:12:01]

pkasting@ can you please take another look?

Sorry about the confusing CL description.

This CL fixes the HTML5 drag demos on Linux and CrOS (Those demos already worked
on Windows)

This CL does change the text string which is dragged out from the omnibox on
Windows but not on CrOS/Linux
- Previously, Windows would use the string set in
Textfield::WriteDragDataForView() and OmniboxViewViews::OnWriteDragData() would
not affect the string which is dragged out

https://codereview.chromium.org/208313009/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/208313009/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:947: // Setting the URL
also sets the string.
I removed the comment because as you mentioned it is confusing.

The intent of my comment was to make it clear that SetURL() does set the string.
In other words, it is unnecessary to call
data->SetURL(url, title);
data->SetString(url.spec());

https://codereview.chromium.org/208313009/diff/70001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/208313009/diff/70001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:372: // a shortcut so we
add it first.
I am not changing this comment because it is not a lie wrt to
OSExchangeDataProviderWin().

As part of a separate CL, it may be good to change OSExchangeDataProviderWin to
have a consistent behavior wrt to duplicate calls to
OSExchangeDataProviderWin::SetString() and
OSExchangeDataProviderWin::SetData()

[pkotwicz, 2014-06-20 22:17:25]

https://codereview.chromium.org/208313009/diff/70001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/208313009/diff/70001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:372: // a shortcut so we
add it first.
I take the comment about consistency back. SetData() is an implementation of the
win32 IDataObject interface while SetString() is not

[Peter Kasting, 2014-06-20 22:26:22]

On 2014/06/20 22:12:01, pkotwicz wrote:
> pkasting@ can you please take another look?
> 
> Sorry about the confusing CL description.
> 
> This CL fixes the HTML5 drag demos on Linux and CrOS (Those demos already
worked
> on Windows)
> 
> This CL does change the text string which is dragged out from the omnibox on
> Windows but not on CrOS/Linux

Your CL description doesn't say this.  It should.

> - Previously, Windows would use the string set in
> Textfield::WriteDragDataForView() and OmniboxViewViews::OnWriteDragData()
would
> not affect the string which is dragged out

That's too low-level an explanation.  Can you tell me practically what the
consequences of this will be?  I have no idea, offhand, what kinds of strings
these two functions generate.

https://codereview.chromium.org/208313009/diff/70001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/208313009/diff/70001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:372: // a shortcut so we
add it first.
All I can say about this comment is that even after both your notes here I have
no idea what it means and it sounds just as confusing as the other comment I had
you changed.

"We set A before calling foo(), because foo() also sets A, and we want to prefer
our value to foo()'s" sounds inherently contradictory.

If foo() checks whether A has been set, use text like what I say below to
indicate that.

[pkotwicz, 2014-06-20 23:20:31]

> > This CL fixes the HTML5 drag demos on Linux and CrOS (Those demos already
> worked
> > on Windows)
> > 
> > This CL does change the text string which is dragged out from the omnibox on
> > Windows but not on CrOS/Linux
> 
> Your CL description doesn't say this.  It should.
> 
> > - Previously, Windows would use the string set in
> > Textfield::WriteDragDataForView() and OmniboxViewViews::OnWriteDragData()
> would
> > not affect the string which is dragged out
> 
> That's too low-level an explanation.  Can you tell me practically what the
> consequences of this will be?  I have no idea, offhand, what kinds of strings
> these two functions generate.

One difference I know about is that
Dragging and dropping "www.random.org" from the URL used drag the text
"www.random.org" on Windows but "http://www.random.org" on CrOS. Now dragging
and dropping "www.random.org" always drags the text "http://www.random.org"

I unaware of all the interesting strings that
AutocompleteClassifierFactory::Classify() can return.
Based on AutocompleteEditTest and the fact that
AutocompleteClassifierFactory::Classify() ignores inline autocomplete, it does
not look like it can return anything too crazy.
More specifically, I have not managed to get the drag text set by
OmniboxViewViews::OnWriteDragData() to differ from Textfield::GetSelectedText()
by anything other than the presence of 'http://'
Sorry, I should have been clear. Given that you are an OWNER of autocomplete, I
was hoping that you would verify whether there would be other expected
differences.

[Peter Kasting, 2014-06-20 23:27:46]

On 2014/06/20 23:20:31, pkotwicz wrote:
> > > This CL fixes the HTML5 drag demos on Linux and CrOS (Those demos already
> > worked
> > > on Windows)
> > > 
> > > This CL does change the text string which is dragged out from the omnibox
on
> > > Windows but not on CrOS/Linux
> > 
> > Your CL description doesn't say this.  It should.
> > 
> > > - Previously, Windows would use the string set in
> > > Textfield::WriteDragDataForView() and OmniboxViewViews::OnWriteDragData()
> > would
> > > not affect the string which is dragged out
> > 
> > That's too low-level an explanation.  Can you tell me practically what the
> > consequences of this will be?  I have no idea, offhand, what kinds of
strings
> > these two functions generate.
> 
> One difference I know about is that
> Dragging and dropping "www.random.org" from the URL used drag the text
> "www.random.org" on Windows but "http://www.random.org" on CrOS. Now dragging
> and dropping "www.random.org" always drags the text "http://www.random.org"

Again, please write this sort of thing in the change description.

I don't know that I think this change is positive, but I'm not inclined to
oppose it either, so LGTM I guess.

[pkotwicz, 2014-06-23 15:18:39]

dcheng@, can you please take another look? Sorry for dragging out this CL for so
long.

[pkotwicz, 2014-07-08 23:23:11]

dcheng@, ping?

[dcheng, 2014-07-09 00:11:11]

On 2014/07/08 at 23:23:11, pkotwicz wrote:
> dcheng@, ping?

Sorry for missing this. LGTM.

[pkotwicz, 2014-07-09 00:13:30]

Scott, can you please take another look? I have modified
ui/views/controls/textfield/textfield.cc and
chrome/browser/ui/views/omnibox/omnibox_view_views.cc since you have last looked
at the CL

[sky, 2014-07-09 20:39:08]

https://codereview.chromium.org/208313009/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/208313009/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:949:
data->SetString(selected_text);
Why don't we always want to set the string content?

[pkotwicz, 2014-07-09 23:00:18]

https://codereview.chromium.org/208313009/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/208313009/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:949:
data->SetString(selected_text);
The string content is set by the first caller of OSExchangeData::SetString().

If we want the string content to be set by OSExchangeData::SetURL() on Windows
(as it is on CrOS and Linux without this CL), OSExchangeData::SetURL() must be
called before OSExchangeData::SetString()

OSExchangeData::SetURL() calls OSExchangeData::SetString() internally

The string being set by the first call to OSExchangeData::SetString() is weird.
However, on Windows, data is set on OSExchangeData in the order of highest to
lowest priority. If OSExchangeData::SetString() were to overwrite the currently
set string, it is unclear whether the priority would also change. The
requirement to prioritize data formats seems to be driven by IEnumFORMATETC

[sky, 2014-07-10 16:13:04]

https://codereview.chromium.org/208313009/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/208313009/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:949:
data->SetString(selected_text);
On 2014/07/09 23:00:18, pkotwicz wrote:
> The string content is set by the first caller of OSExchangeData::SetString().
> 
> If we want the string content to be set by OSExchangeData::SetURL() on Windows
> (as it is on CrOS and Linux without this CL), OSExchangeData::SetURL() must be
> called before OSExchangeData::SetString()
> 
> OSExchangeData::SetURL() calls OSExchangeData::SetString() internally
> 
> The string being set by the first call to OSExchangeData::SetString() is
weird.
> However, on Windows, data is set on OSExchangeData in the order of highest to
> lowest priority. If OSExchangeData::SetString() were to overwrite the
currently
> set string, it is unclear whether the priority would also change. The
> requirement to prioritize data formats seems to be driven by IEnumFORMATETC
> 

Was this always the case? I wonder if SetURL should not touch the string data.

[pkotwicz, 2014-07-10 16:36:15]

It looks like this was always the case. SetURL() was setting the string data
back in 2009.
http://src.chromium.org/viewvc/chrome/trunk/src/ui/base/dragdrop/os_exchange_...

[sky, 2014-07-10 16:37:58]

On 2014/07/10 16:36:15, pkotwicz wrote:
> It looks like this was always the case. SetURL() was setting the string data
> back in 2009.
>
http://src.chromium.org/viewvc/chrome/trunk/src/ui/base/dragdrop/os_exchange_...

Just so we're clear, does this change things on windows? Your comment doesn't
indicate that one way or the other.

[pkotwicz, 2014-07-10 16:45:18]

I do not think that the change in OmniboxViewViews::OnWriteDragData() has an
impact but I am not sure.

I know for sure that the change in Textfield::WriteDragDataForView() changes
things on Windows. It causes the change mentioned in the CL description.
"On Windows, with this CL, the dragged out string for URLs includes 'http://'.
For instance, if the user has navigated to 'www.random.org', the dragged out
string is 'www.random.org' without this CL and 'http://www.random.org' with this
CL."

[sky, 2014-07-10 19:15:44]

I tend to think we want www.random.org here. pkasting likely has an opinion
here.

  -Scott


On Thu, Jul 10, 2014 at 9:45 AM, <pkotwicz@chromium.org> wrote:

> I do not think that the change in OmniboxViewViews::OnWriteDragData() has
> an
> impact but I am not sure.
>
> I know for sure that the change in Textfield::WriteDragDataForView()
> changes
> things on Windows. It causes the change mentioned in the CL description.
> "On Windows, with this CL, the dragged out string for URLs includes
> 'http://'.
> For instance, if the user has navigated to 'www.random.org', the dragged
> out
> string is 'www.random.org' without this CL and 'http://www.random.org'
> with this
> CL."
>
>
>
> https://codereview.chromium.org/208313009/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-07-10 20:20:15]

On 2014/07/10 19:15:44, sky wrote:
> I tend to think we want http://www.random.org here. pkasting likely has an
opinion
> here.

I shared it earlier; I'm very weakly opposed.

"I don't know that I think this change is positive, but I'm not inclined to
oppose it either, so LGTM I guess."

[sky, 2014-07-10 22:13:58]

I like fixing the demos you site, but I don't really like including http in the
copied string.

[dcheng, 2014-07-10 22:48:23]

On 2014/07/10 at 22:13:58, sky wrote:
> I like fixing the demos you site, but I don't really like including http in
the copied string.

Take my thoughts with a grain of salt... but if the entire URL is selected and
dragged out, to me it seems conceptually similar to selecting the entire URL and
copying it to the clipboard. In the clipboard case, we add the scheme back in;
it seems to me it'd make sense to add the scheme back in for dragging as well.
Otherwise, we shouldn't add the scheme in.

[pkotwicz, 2014-07-10 23:19:43]

Scott, can you please take another look?
I have reverted the changes to omnibox_view_views.cc and
ui/views/controls/textfield/textfield.cc as you have suggested.

Now the behavior on Windows does not change at all. (And the behavior of
Linux/Cros changes to match Windows)

dcheng@, I think that whether the scheme should included in the text is dragged
out can be handled by a different CL. Once a decision has been reached on this
point please assign a bug to me and I will implement whatever the preferred
solution is.

[sky, 2014-07-11 00:07:56]

LGTM

[pkotwicz, 2014-07-11 00:19:19]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-07-11 00:20:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/208313009/110001

[commit-bot: I haz the power, 2014-07-11 03:40:32]

Change committed as 282529