Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1567023:
[Mac] Location icon in omnibox as drag source. (Closed)
|
Created:
10 years, 8 months ago by Scott Hess - ex-Googler Modified:
9 years, 6 months ago CC:
chromium-reviews Visibility:
Public. |
Description[Mac] Location icon in omnibox as drag source.
Wire up location icon to allow dragging if the user click-drags or
click-holds. Otherwise falls through to the page-info display.
BUG=37865
TEST=When omnibox shows an URL, click-drag the globe initiates an URL drag.
TEST=Drop it in web content.
TEST=Drop it in safari.
TEST=Drop it on bookmark bar.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=43971
Patch Set 1 #
Total comments: 17
Patch Set 2 : Nits and fix tests. #
Total comments: 6
Patch Set 3 : more nits #
Total comments: 1
Messages
Total messages: 12 (0 generated)
Seems reasonable to me, though I'm adding Pink since more eyes is better. unit_tests seem broken, btw. http://codereview.chromium.org/1567023/diff/1/2 File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right): http://codereview.chromium.org/1567023/diff/1/2#newcode59 chrome/browser/cocoa/autocomplete_text_field_cell.mm:59: const float kLocationIconDragTimeout = 0.25; There's a type for that, namely |NSTimeInterval|. http://codereview.chromium.org/1567023/diff/1/2#newcode605 chrome/browser/cocoa/autocomplete_text_field_cell.mm:605: NSEvent* event = [NSApp nextEventMatchingMask:(NSLeftMouseDraggedMask | Can anything cause in the browser to go away in this loop? http://codereview.chromium.org/1567023/diff/1/2#newcode614 chrome/browser/cocoa/autocomplete_text_field_cell.mm:614: // TODO(shess): My understanding is that the -isFlipped ? http://codereview.chromium.org/1567023/diff/1/2#newcode624 chrome/browser/cocoa/autocomplete_text_field_cell.mm:624: offset:NSMakeSize(0, 0) NSZeroSize http://codereview.chromium.org/1567023/diff/1/2#newcode632 chrome/browser/cocoa/autocomplete_text_field_cell.mm:632: // If no pasteboard data, fall through to mouse-pressed case. Is this really right? http://codereview.chromium.org/1567023/diff/1/4 File chrome/browser/cocoa/location_bar_view_mac.mm (right): http://codereview.chromium.org/1567023/diff/1/4#newcode627 chrome/browser/cocoa/location_bar_view_mac.mm:627: NSString* title = base::SysUTF16ToNSString(tab->GetTitle()); Is the title ever empty? http://codereview.chromium.org/1567023/diff/1/4#newcode632 chrome/browser/cocoa/location_bar_view_mac.mm:632: [pboard setDataForURL:url title:title]; Does this drag file URLs properly?
On 2010/04/05 23:12:14, viettrungluu wrote: > unit_tests seem broken, btw. Sigh, will go look, sorry that I didn't look before. Too many balls in the air... http://codereview.chromium.org/1567023/diff/1/2 File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right): http://codereview.chromium.org/1567023/diff/1/2#newcode59 chrome/browser/cocoa/autocomplete_text_field_cell.mm:59: const float kLocationIconDragTimeout = 0.25; On 2010/04/05 23:12:14, viettrungluu wrote: > There's a type for that, namely |NSTimeInterval|. Done. http://codereview.chromium.org/1567023/diff/1/2#newcode605 chrome/browser/cocoa/autocomplete_text_field_cell.mm:605: NSEvent* event = [NSApp nextEventMatchingMask:(NSLeftMouseDraggedMask | On 2010/04/05 23:12:14, viettrungluu wrote: > Can anything cause in the browser to go away in this loop? This should be within the scope of -sendEvent:, so things like window.close() should be deferred. http://codereview.chromium.org/1567023/diff/1/2#newcode614 chrome/browser/cocoa/autocomplete_text_field_cell.mm:614: // TODO(shess): My understanding is that the -isFlipped On 2010/04/05 23:12:14, viettrungluu wrote: > ? As the comment says, if I don't shift dragPoint.y by the height when -isFlipped is YES, the position is way wrong. But AFAICT I'm passing the coordinates in controlView's coordinate system. So either I'm missing something or [icon rect] is being calculated incorrectly (which also seems unlikely). http://codereview.chromium.org/1567023/diff/1/2#newcode624 chrome/browser/cocoa/autocomplete_text_field_cell.mm:624: offset:NSMakeSize(0, 0) On 2010/04/05 23:12:14, viettrungluu wrote: > NSZeroSize Done. http://codereview.chromium.org/1567023/diff/1/2#newcode632 chrome/browser/cocoa/autocomplete_text_field_cell.mm:632: // If no pasteboard data, fall through to mouse-pressed case. On 2010/04/05 23:12:14, viettrungluu wrote: > Is this really right? OK, I've poked around and AFAICT GetDragPasteboard() should never get an empty TabContents in this case. [Or, at least, some code checks for NULL, other code just uses it, and I can't tell why this case should check.] So I'll throw in a CHECK() there and a DCHECK() here. http://codereview.chromium.org/1567023/diff/1/4 File chrome/browser/cocoa/location_bar_view_mac.mm (right): http://codereview.chromium.org/1567023/diff/1/4#newcode627 chrome/browser/cocoa/location_bar_view_mac.mm:627: NSString* title = base::SysUTF16ToNSString(tab->GetTitle()); On 2010/04/05 23:12:14, viettrungluu wrote: > Is the title ever empty? How do you mean "empty"? It's a string16&, so it cannot be NULL. A zero-length string should be fine, but in practice if the title is empty I seem to get the URL in GetTitle(). http://codereview.chromium.org/1567023/diff/1/4#newcode632 chrome/browser/cocoa/location_bar_view_mac.mm:632: [pboard setDataForURL:url title:title]; On 2010/04/05 23:12:14, viettrungluu wrote: > Does this drag file URLs properly? If "properly" means "Dropping to Safari takes me to that page, dropping in bookmark bar bookmarks, etc", then yes. Do you mean something more sophisticated? Like sending NSFilenamesPboardType? Only the download shelf seems to do that currently.
On 2010/04/05 23:47:53, shess wrote: > On 2010/04/05 23:12:14, viettrungluu wrote: > > unit_tests seem broken, btw. > > Sigh, will go look, sorry that I didn't look before. Too many balls in the > air... Fixed unit tests, is all ready for another look. Assuming I didn't break another one, somewhere out there.
http://codereview.chromium.org/1567023/diff/1/4 File chrome/browser/cocoa/location_bar_view_mac.mm (right): http://codereview.chromium.org/1567023/diff/1/4#newcode627 chrome/browser/cocoa/location_bar_view_mac.mm:627: NSString* title = base::SysUTF16ToNSString(tab->GetTitle()); On 2010/04/05 23:47:53, shess wrote: > On 2010/04/05 23:12:14, viettrungluu wrote: > > Is the title ever empty? > > How do you mean "empty"? It's a string16&, so it cannot be NULL. A zero-length > string should be fine, but in practice if the title is empty I seem to get the > URL in GetTitle(). I meant empty in the only reasonable sense of "empty". But since you get the URL as title, I'm happy. That's what I was really asking for. http://codereview.chromium.org/1567023/diff/1/4#newcode632 chrome/browser/cocoa/location_bar_view_mac.mm:632: [pboard setDataForURL:url title:title]; On 2010/04/05 23:47:53, shess wrote: > On 2010/04/05 23:12:14, viettrungluu wrote: > > Does this drag file URLs properly? > > If "properly" means "Dropping to Safari takes me to that page, dropping in > bookmark bar bookmarks, etc", then yes. Do you mean something more > sophisticated? Like sending NSFilenamesPboardType? Only the download shelf > seems to do that currently. Oh, I meant but forgot to say, "properly to Finder".
Mostly LGTM. I still am unsure if anyone will ever find this, though. The thing I expect to drag is the object to the left of the url, which is not draggable. The star doesn't seem to imply that it represents the url, nor does it necessarily afford dragging. This is suboptimal from a UI perspective but i understand it's all we have to work with. http://codereview.chromium.org/1567023/diff/7001/8001 File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right): http://codereview.chromium.org/1567023/diff/7001/8001#newcode614 chrome/browser/cocoa/autocomplete_text_field_cell.mm:614: // TODO(shess): My understanding is that the -isFlipped File a bug for the TODO and include it here, or make it not a TODO. http://codereview.chromium.org/1567023/diff/7001/8002 File chrome/browser/cocoa/autocomplete_text_field_unittest.mm (right): http://codereview.chromium.org/1567023/diff/7001/8002#newcode34 chrome/browser/cocoa/autocomplete_text_field_unittest.mm:34: return [NSPasteboard pasteboardWithName:NSDragPboard]; don't use the drag pasteboard in tests, can't you create a new unnamed one via the api to use for this test? http://codereview.chromium.org/1567023/diff/7001/8004 File chrome/browser/cocoa/location_bar_view_mac.mm (right): http://codereview.chromium.org/1567023/diff/7001/8004#newcode623 chrome/browser/cocoa/location_bar_view_mac.mm:623: CHECK(tab); do you really want to crash here?
On 2010/04/06 15:51:04, pink wrote: > The thing > I expect to drag is the object to the left of the url, which is not draggable. > The star doesn't seem to imply that it represents the url, nor does it > necessarily afford dragging. The star is no longer the item to the left of the URL. Now it's the globe (or lock) which is much more immediately obvious to be a drag source.
So which is this making draggable? The globe or the star? The star appears to be a subclass of the class that's draggable. Is the security icon as well? On Tue, Apr 6, 2010 at 11:58 AM, <avi@chromium.org> wrote: > On 2010/04/06 15:51:04, pink wrote: >> >> The thing >> I expect to drag is the object to the left of the url, which is not >> draggable. >> The star doesn't seem to imply that it represents the url, nor does it >> necessarily afford dragging. > > The star is no longer the item to the left of the URL. Now it's the globe > (or > lock) which is much more immediately obvious to be a drag source. > > http://codereview.chromium.org/1567023 > -- Mike Pinkerton Mac Weenie pinkerton@google.com
On 2010/04/06 16:00:54, pink wrote: > So which is this making draggable? The globe or the star? The star > appears to be a subclass of the class that's draggable. Is the > security icon as well? The item to the left of the URL, LocationIconView. Which is a subclass of LocationBarImageView, which StarIconView is also a subclass of. I didn't invent 2/3 of these names :-). I believe the rest of the Chromiverse used the old star button as the drag source.
http://codereview.chromium.org/1567023/diff/1/4 File chrome/browser/cocoa/location_bar_view_mac.mm (right): http://codereview.chromium.org/1567023/diff/1/4#newcode632 chrome/browser/cocoa/location_bar_view_mac.mm:632: [pboard setDataForURL:url title:title]; On 2010/04/06 00:43:13, viettrungluu wrote: > On 2010/04/05 23:47:53, shess wrote: > > On 2010/04/05 23:12:14, viettrungluu wrote: > > > Does this drag file URLs properly? > > > > If "properly" means "Dropping to Safari takes me to that page, dropping in > > bookmark bar bookmarks, etc", then yes. Do you mean something more > > sophisticated? Like sending NSFilenamesPboardType? Only the download shelf > > seems to do that currently. > > Oh, I meant but forgot to say, "properly to Finder". It seems to make a copy of the file where you drop it (versus a webloc file for http: sites). Since my code isn't doing anything interesting, I'm guessing this means that the Finder handles the special-casing. http://codereview.chromium.org/1567023/diff/7001/8001 File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right): http://codereview.chromium.org/1567023/diff/7001/8001#newcode614 chrome/browser/cocoa/autocomplete_text_field_cell.mm:614: // TODO(shess): My understanding is that the -isFlipped On 2010/04/06 15:51:04, pink wrote: > File a bug for the TODO and include it here, or make it not a TODO. Done. http://codereview.chromium.org/1567023/diff/7001/8002 File chrome/browser/cocoa/autocomplete_text_field_unittest.mm (right): http://codereview.chromium.org/1567023/diff/7001/8002#newcode34 chrome/browser/cocoa/autocomplete_text_field_unittest.mm:34: return [NSPasteboard pasteboardWithName:NSDragPboard]; On 2010/04/06 15:51:04, pink wrote: > don't use the drag pasteboard in tests, can't you create a new unnamed one via > the api to use for this test? Done. Unfortunately, the code doesn't really test this aspect, but it does test the clicked case, and if the test goes awry this method will be called, and it should return something... http://codereview.chromium.org/1567023/diff/7001/8004 File chrome/browser/cocoa/location_bar_view_mac.mm (right): http://codereview.chromium.org/1567023/diff/7001/8004#newcode623 chrome/browser/cocoa/location_bar_view_mac.mm:623: CHECK(tab); On 2010/04/06 15:51:04, pink wrote: > do you really want to crash here? If tab is NULL, it's going to crash. The various code is inconsistent, not just the Mac code, some checks for non-NULL, other code just goes ahead and dereferences it. I cannot think of any way to get to this code without a |TabContents|, but that doesn't mean I'm not missing something. I'll drop it to a DCHECK(). I suspect it is not necessary, but I can't quite convince myself enough to assert that it is not necessary.
Ping? Not that I'm impatient, but if we hurry we'll have platform-parity with Linux on this essential feature :-). On 2010/04/07 21:15:26, shess wrote: > http://codereview.chromium.org/1567023/diff/1/4 > File chrome/browser/cocoa/location_bar_view_mac.mm (right): > > http://codereview.chromium.org/1567023/diff/1/4#newcode632 > chrome/browser/cocoa/location_bar_view_mac.mm:632: [pboard setDataForURL:url > title:title]; > On 2010/04/06 00:43:13, viettrungluu wrote: > > On 2010/04/05 23:47:53, shess wrote: > > > On 2010/04/05 23:12:14, viettrungluu wrote: > > > > Does this drag file URLs properly? > > > > > > If "properly" means "Dropping to Safari takes me to that page, dropping in > > > bookmark bar bookmarks, etc", then yes. Do you mean something more > > > sophisticated? Like sending NSFilenamesPboardType? Only the download shelf > > > seems to do that currently. > > > > Oh, I meant but forgot to say, "properly to Finder". > > It seems to make a copy of the file where you drop it (versus a webloc file for > http: sites). Since my code isn't doing anything interesting, I'm guessing this > means that the Finder handles the special-casing. > > http://codereview.chromium.org/1567023/diff/7001/8001 > File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right): > > http://codereview.chromium.org/1567023/diff/7001/8001#newcode614 > chrome/browser/cocoa/autocomplete_text_field_cell.mm:614: // TODO(shess): My > understanding is that the -isFlipped > On 2010/04/06 15:51:04, pink wrote: > > File a bug for the TODO and include it here, or make it not a TODO. > > Done. > > http://codereview.chromium.org/1567023/diff/7001/8002 > File chrome/browser/cocoa/autocomplete_text_field_unittest.mm (right): > > http://codereview.chromium.org/1567023/diff/7001/8002#newcode34 > chrome/browser/cocoa/autocomplete_text_field_unittest.mm:34: return > [NSPasteboard pasteboardWithName:NSDragPboard]; > On 2010/04/06 15:51:04, pink wrote: > > don't use the drag pasteboard in tests, can't you create a new unnamed one via > > the api to use for this test? > > Done. > > Unfortunately, the code doesn't really test this aspect, but it does test the > clicked case, and if the test goes awry this method will be called, and it > should return something... > > http://codereview.chromium.org/1567023/diff/7001/8004 > File chrome/browser/cocoa/location_bar_view_mac.mm (right): > > http://codereview.chromium.org/1567023/diff/7001/8004#newcode623 > chrome/browser/cocoa/location_bar_view_mac.mm:623: CHECK(tab); > On 2010/04/06 15:51:04, pink wrote: > > do you really want to crash here? > > If tab is NULL, it's going to crash. The various code is inconsistent, not just > the Mac code, some checks for non-NULL, other code just goes ahead and > dereferences it. I cannot think of any way to get to this code without a > |TabContents|, but that doesn't mean I'm not missing something. > > I'll drop it to a DCHECK(). I suspect it is not necessary, but I can't quite > convince myself enough to assert that it is not necessary.
LGTM. http://codereview.chromium.org/1567023/diff/15002/18004 File chrome/browser/cocoa/location_bar_view_mac.mm (right): http://codereview.chromium.org/1567023/diff/15002/18004#newcode623 chrome/browser/cocoa/location_bar_view_mac.mm:623: DCHECK(tab); This seems redundant, since it'll crash on the next line anyway. <shrug> |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
