Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8311014:
GTK: add TEXT_URI_LIST as a data type for drags from the omnibox. (Closed)
|
Created:
9 years, 2 months ago by SanjoyPal Modified:
9 years, 2 months ago CC:
chromium-reviews Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionGTK: add CHROME_NAMED_URL as a data type for drags from the omnibox.
This is required when we drag the url from omnibox to render widget of a tab, the tab should navigate to the dropped url.
BUG=53352
TEST=As described in the bug.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107171
Patch Set 1 #
Total comments: 6
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 8
Patch Set 4 : '' #
Total comments: 2
Patch Set 5 : '' #Messages
Total messages: 18 (0 generated)
Please review the fix.
On 2011/10/17 09:09:16, SanjoyPal wrote: > Please review the fix. The functionality is OK. The commit message is terrible so NACK. I let this pass last week. I can see that you added "TEXT_URI_LIST" as a drag source from the actual patch. Your commit message should say why you're doing this.
I agree with Elliot regarding the CL description. Please use prefixes such as "GTK:" to show at a glance what part of the code you're touching, and describe the changes you're making rather than the state of the world after your changes. Hence the first line of the CL desc. would be something like: "GTK: add TEXT_URI_LIST as a data type for drags from the omnibox" Then add more detail in the following lines. http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:249: gtk_target_list_add_uri_targets(targets, ui::TEXT_URI_LIST); this isn't right, if I select just a small part of the text and start dragging then it's not a URI. Basically if we are not adding the scheme onto the front then it shouldn't have this target. http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1620: UTF8ToUTF16(dragged_text_), target_type); the name should be the tab title
Updated. PTAL. http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:249: gtk_target_list_add_uri_targets(targets, ui::TEXT_URI_LIST); On 2011/10/17 21:55:59, Evan Stade wrote: > this isn't right, if I select just a small part of the text and start dragging > then it's not a URI. Yes, then it will be treated as TEXT_PLAIN. "Basically if we are not adding the scheme onto the front then it shouldn't have this target." I guess, i dint understand this statement completely. Can you please explain more? http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1620: UTF8ToUTF16(dragged_text_), target_type); On 2011/10/17 21:55:59, Evan Stade wrote: > the name should be the tab title The title is not being used in TEXT_URI_LIST case inside ui::WriteURLWithName. And user can edit the url in omnibox before dragging, url and title may not match. So can we pass it as empty string?
http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:249: gtk_target_list_add_uri_targets(targets, ui::TEXT_URI_LIST); On 2011/10/18 08:15:30, SanjoyPal wrote: > On 2011/10/17 21:55:59, Evan Stade wrote: > > this isn't right, if I select just a small part of the text and start dragging > > then it's not a URI. > > Yes, then it will be treated as TEXT_PLAIN. > > "Basically if we are not adding the scheme onto the front > then it shouldn't have this target." > > I guess, i dint understand this statement completely. Can you please explain > more? > we don't display the scheme in the http:// case. However in certain cases on copy we do put the http:// on the front of the clipboard data. In these same cases we should make URI list available, but in the cases where we don't put http:// on the front of the clipboard data (e.g., "foo" is selected in bar.com/foo) we should not advertise TEXT_URI_LIST http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1620: UTF8ToUTF16(dragged_text_), target_type); On 2011/10/18 08:15:30, SanjoyPal wrote: > On 2011/10/17 21:55:59, Evan Stade wrote: > > the name should be the tab title > > The title is not being used in TEXT_URI_LIST case inside ui::WriteURLWithName. well, don't use TEXT_URI_LIST then > And user can edit the url in omnibox before dragging, url and title may not > match. So can we pass it as empty string? I see the tab title/URL mismatch as a corner case. But I guess if you want you can pass an empty string when less than the full text is selected or the user has edited the text.
On 2011/10/18 16:27:31, Evan Stade wrote: > http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... > File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): > > http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... > chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:249: > gtk_target_list_add_uri_targets(targets, ui::TEXT_URI_LIST); > On 2011/10/18 08:15:30, SanjoyPal wrote: > > On 2011/10/17 21:55:59, Evan Stade wrote: > > > this isn't right, if I select just a small part of the text and start > dragging > > > then it's not a URI. > > > > Yes, then it will be treated as TEXT_PLAIN. > > > > "Basically if we are not adding the scheme onto the front > > then it shouldn't have this target." > > > > I guess, i dint understand this statement completely. Can you please explain > > more? > > > > we don't display the scheme in the http:// case. However in certain cases on > copy we do put the http:// on the front of the clipboard data. In these same > cases we should make URI list available, but in the cases where we don't put > http:// on the front of the clipboard data (e.g., "foo" is selected in > bar.com/foo) we should not advertise TEXT_URI_LIST Done. > > http://codereview.chromium.org/8311014/diff/1/chrome/browser/ui/gtk/omnibox/o... > chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1620: > UTF8ToUTF16(dragged_text_), target_type); > On 2011/10/18 08:15:30, SanjoyPal wrote: > > On 2011/10/17 21:55:59, Evan Stade wrote: > > > the name should be the tab title > > > > The title is not being used in TEXT_URI_LIST case inside ui::WriteURLWithName. > > well, don't use TEXT_URI_LIST then > > > And user can edit the url in omnibox before dragging, url and title may not > > match. So can we pass it as empty string? > > I see the tab title/URL mismatch as a corner case. But I guess if you want you > can pass an empty string when less than the full text is selected or the user > has edited the text. Done. Please review once more. Thanks.
PTAL. Thanks.
http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:250: copy_targets_ = gtk_text_buffer_get_copy_target_list(text_buffer_); doing this opens a can of worms regarding object ownership/lifetime, I'd rather you just call get_copy_target_list each time you need it. http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1611: // put our doctored selection that might have the 'http://' prefix. Also, GTK you can remove the comment about the switch http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1636: if (GURL(selected_text_).is_valid()) no. You need to use AdjustTextForCopy http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1644: if (gtk_target_list_find(copy_targets_, text_uri_atom, NULL)) I don't think you need this check
Done. Thanks for the comments. I uploaded the patch again. Please review. http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:250: copy_targets_ = gtk_text_buffer_get_copy_target_list(text_buffer_); On 2011/10/21 19:16:11, Evan Stade wrote: > doing this opens a can of worms regarding object ownership/lifetime, I'd rather > you just call get_copy_target_list each time you need it. Done. http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1611: // put our doctored selection that might have the 'http://' prefix. Also, GTK On 2011/10/21 19:16:11, Evan Stade wrote: > you can remove the comment about the switch Done. http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1636: if (GURL(selected_text_).is_valid()) On 2011/10/21 19:16:11, Evan Stade wrote: > no. You need to use AdjustTextForCopy Done. http://codereview.chromium.org/8311014/diff/8001/chrome/browser/ui/gtk/omnibo... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1644: if (gtk_target_list_find(copy_targets_, text_uri_atom, NULL)) On 2011/10/21 19:16:11, Evan Stade wrote: > I don't think you need this check Done.
http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1615: case ui::TEXT_URI_LIST: { you are still using TEXT_URI_LIST
http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1615: case ui::TEXT_URI_LIST: { On 2011/10/24 22:46:31, Evan Stade wrote: Can you please suggest which target will be appropriate in this case? Is it CHROME_NAMED_URL? --Thanks.
On 2011/10/24 22:46:31, Evan Stade wrote: > http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... > File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): > > http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... > chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1615: case ui::TEXT_URI_LIST: > { > you are still using TEXT_URI_LIST just a doubt, what is the reason that we should not use TEXT_URI_LIST here?
On 2011/10/25 06:56:17, SanjoyPal wrote: > On 2011/10/24 22:46:31, Evan Stade wrote: > > > http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... > > File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): > > > > > http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... > > chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1615: case > ui::TEXT_URI_LIST: > > { > > you are still using TEXT_URI_LIST > > just a doubt, what is the reason that we should not use TEXT_URI_LIST here? Can you please suggest which target will be appropriate in this case? Is it CHROME_NAMED_URL? --Thanks.
On 2011/10/24 22:46:31, Evan Stade wrote: > http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... > File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): > > http://codereview.chromium.org/8311014/diff/17001/chrome/browser/ui/gtk/omnib... > chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1615: case ui::TEXT_URI_LIST: > { > you are still using TEXT_URI_LIST Updated the patch, now using CHROME_NAMED_URL. PTAL.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ncj674@motorola.com/8311014/21002
Change committed as 107171 |
