bookmarks: GetNameForURL() function is only used by gtk.

bookmarks: GetNameForURL() function is only used by gtk.

So, move it to c/b/ui/gtk/bookmarks. Where it's more appropriate.

BUG=144783
R=sky@chromium.org


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

[sky, 2013-01-14 16:17:49]

What do the other platforms that they don't use this code?

[tfarina, 2013-01-14 16:31:49]

On 2013/01/14 16:17:49, sky wrote:
> What do the other platforms that they don't use this code?

It seems to be gtk specific, because one of the calls is under ui::TEXT_PLAIN
case, and the other is under ui::TEXT_URI_LIST. And the  enum is gtk specific
(ui/base/dragdrop/gtk_dnd_util.h).

For views, this might be handled by CreateValidFileNameFromTitle from
ui/base/dragdrop/os_exchange_data_provider_win.cc.

[sky, 2013-01-14 16:39:48]

On Mon, Jan 14, 2013 at 8:31 AM,  <tfarina@chromium.org> wrote:
> On 2013/01/14 16:17:49, sky wrote:
>>
>> What do the other platforms that they don't use this code?
>
>
> It seems to be gtk specific, because one of the calls is under
> ui::TEXT_PLAIN
> case, and the other is under ui::TEXT_URI_LIST. And the  enum is gtk
> specific
> (ui/base/dragdrop/gtk_dnd_util.h).
>
> For views, this might be handled by CreateValidFileNameFromTitle from
> ui/base/dragdrop/os_exchange_data_provider_win.cc.
>
> https://codereview.chromium.org/11827058/

That's the configuring case, not the drop case (which I think is where
the code you're changing is used on gtk). I think its GetURLAndTitle
that should be updated.

  -Scott

[tfarina, 2013-01-14 16:58:16]

On Mon, Jan 14, 2013 at 2:39 PM, Scott Violet <sky@chromium.org> wrote:
>> For views, this might be handled by CreateValidFileNameFromTitle from
>> ui/base/dragdrop/os_exchange_data_provider_win.cc.
>>
>> https://codereview.chromium.org/11827058/
>
> That's the configuring case, not the drop case (which I think is where
> the code you're changing is used on gtk). I think its GetURLAndTitle
> that should be updated.
>
I don't want to change the behavior in this change. As I don't have a
Windows box capable to try it.

Are you saying to patch ui/base/dragdrop/os_exchange_data_provider_win.cc:383?

The gtk code looks like this:

case ui::TEXT_PLAIN: {
      guchar* text = gtk_selection_data_get_text(selection_data);
      if (!text)
        break;
      GURL url(reinterpret_cast<char*>(text));
      g_free(text);
      // TODO(estade): It would be nice to head this case off at drag
motion,
      // so that it doesn't look like we can drag onto the bookmark
bar.
      if (!url.is_valid())
        break;
      string16 title = bookmark_utils::GetNameForURL(url);
      model_->AddURL(dest_node, index, title, url);
      dnd_success = TRUE;
      break;
    }

-- 
Thiago

[sky, 2013-01-15 00:29:50]

On Mon, Jan 14, 2013 at 8:58 AM, Thiago Farina <tfarina@chromium.org> wrote:
> On Mon, Jan 14, 2013 at 2:39 PM, Scott Violet <sky@chromium.org> wrote:
>>> For views, this might be handled by CreateValidFileNameFromTitle from
>>> ui/base/dragdrop/os_exchange_data_provider_win.cc.
>>>
>>> https://codereview.chromium.org/11827058/
>>
>> That's the configuring case, not the drop case (which I think is where
>> the code you're changing is used on gtk). I think its GetURLAndTitle
>> that should be updated.
>>
> I don't want to change the behavior in this change. As I don't have a
> Windows box capable to try it.
>
> Are you saying to patch
ui/base/dragdrop/os_exchange_data_provider_win.cc:383?
>
>
> The gtk code looks like this:
>
> case ui::TEXT_PLAIN: {
>       guchar* text = gtk_selection_data_get_text(selection_data);
>       if (!text)
>         break;
>       GURL url(reinterpret_cast<char*>(text));
>       g_free(text);
>       // TODO(estade): It would be nice to head this case off at drag
> motion,
>       // so that it doesn't look like we can drag onto the bookmark
> bar.
>       if (!url.is_valid())
>         break;
>       string16 title = bookmark_utils::GetNameForURL(url);
>       model_->AddURL(dest_node, index, title, url);
>       dnd_success = TRUE;
>       break;
>     }
>
> --
> Thiago

I'm saying if we're going to change anything, we should fix it the
right way. I'm not sure if the right fix is adjusting
OSExchangeDataProviderWin::GetURLAndTitle, or tweaking the bookmark
specific place. Either way if we're going to change something we
should fix it the right way.

  -Scott

[tfarina, 2013-01-15 01:21:33]

On Mon, Jan 14, 2013 at 10:29 PM, Scott Violet <sky@chromium.org> wrote:
> I'm saying if we're going to change anything, we should fix it the
> right way. I'm not sure if the right fix is adjusting
> OSExchangeDataProviderWin::GetURLAndTitle, or tweaking the bookmark
> specific place. Either way if we're going to change something we
> should fix it the right way.
>

What you are seeing broken on Windows/ChromeOS/Mac?

If I'm interpreting this code right, what it does is:

- If you drag an url, for example, from the omnibox, to the bookmarks
bar, it gives you the bookmark name/title for that url.

In my Windows machine (Version 24.0.1312.52 m), if I drag the
google.com url to the bookmarks bar it creates a bookmark with
'Google' name/title.

So from my understanding, both platforms are working correctly, just
doing it by different ways.

With this patch I want to isolate what is only used by one platform,
in the platform specific directory. Moving it away from the
cross-platform API/directory. If you are fine with this, can you
approve this change? Or let me know what I should look for?

-- 
Thiago

[sky, 2013-01-15 16:39:49]

On Mon, Jan 14, 2013 at 5:21 PM, Thiago Farina <tfarina@chromium.org> wrote:
> On Mon, Jan 14, 2013 at 10:29 PM, Scott Violet <sky@chromium.org> wrote:
>> I'm saying if we're going to change anything, we should fix it the
>> right way. I'm not sure if the right fix is adjusting
>> OSExchangeDataProviderWin::GetURLAndTitle, or tweaking the bookmark
>> specific place. Either way if we're going to change something we
>> should fix it the right way.
>>
>
> What you are seeing broken on Windows/ChromeOS/Mac?
>
> If I'm interpreting this code right, what it does is:
>
> - If you drag an url, for example, from the omnibox, to the bookmarks
> bar, it gives you the bookmark name/title for that url.

From what I see this code path is used when you drop a url on the
bookmark bar that contains only plain text. The plain text is used as
the url and GetNameForURL() is used to determine the title. On the
windows side, in such a scenario
(OSExchangeDataProviderWin::GetURLAndTitle) we end up using no title.
What I'm saying is the two should behave consistently.

  -Scott

> In my Windows machine (Version 24.0.1312.52 m), if I drag the
> google.com url to the bookmarks bar it creates a bookmark with
> 'Google' name/title.
>
> So from my understanding, both platforms are working correctly, just
> doing it by different ways.
>
> With this patch I want to isolate what is only used by one platform,
> in the platform specific directory. Moving it away from the
> cross-platform API/directory. If you are fine with this, can you
> approve this change? Or let me know what I should look for?
>
> --
> Thiago

[tfarina, 2013-01-15 17:03:59]

Is patch set 2 what you are asking me for?

[sky, 2013-01-15 18:23:51]

Bingo. I wasn't sure if we want to change OSExchangeDataProviderWin directly, or
have bookmarkbarview do the title changes. Lets try this and see if if causes
any problems.

LGTM

[commit-bot: I haz the power, 2013-01-15 20:32:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/11827058/15001

[tfarina, 2013-01-15 22:50:13]

Scott, the changes done in os_exchange_data_provider_win.cc broken some
bookmark_node_data_unittest.cc

e:\b\build\slave\win\build\src\chrome\browser\bookmarks\bookmark_node_data_unittest.cc(72):
error: Value of: drag_data.elements[0].title
Actual: L"google.com"
Expected: title
Which is: L"title"

Can I revert the changes that I did there and land it?

[sky, 2013-01-15 23:24:12]

The text expectations need to be updated.

On Tue, Jan 15, 2013 at 2:50 PM,  <tfarina@chromium.org> wrote:
> Scott, the changes done in os_exchange_data_provider_win.cc broken some
> bookmark_node_data_unittest.cc
>
>
e:\b\build\slave\win\build\src\chrome\browser\bookmarks\bookmark_node_data_unittest.cc(72):
> error: Value of: drag_data.elements[0].title
> Actual: L"google.com"
> Expected: title
> Which is: L"title"
>
> Can I revert the changes that I did there and land it?
>
> https://codereview.chromium.org/11827058/

[commit-bot: I haz the power, 2013-01-15 23:30:26]

Retried try job too often on win_rel for step(s) unit_tests

[tfarina, 2013-01-15 23:32:41]

Like patch set 4?

[sky, 2013-01-16 00:40:59]

yes LGTM

[tfarina, 2013-01-16 00:48:44]

Updated os_exchange_data_win_unittest.cc too.

[commit-bot: I haz the power, 2013-01-16 01:29:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/11827058/29002

[commit-bot: I haz the power, 2013-01-16 03:27:37]

Retried try job too often on win for step(s) compile

[commit-bot: I haz the power, 2013-01-16 11:28:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/11827058/29002

[commit-bot: I haz the power, 2013-01-16 13:11:09]

Change committed as 177137