Mac: Make a stop-gap visual indicator for dropping URLs on the tab strip.

Mac: Make a stop-gap visual indicator for dropping URLs on the tab strip.

A.k.a. a present for pkasting. Eventually we'll probably want to do something more sophisticated (at the very least arrows should fade in/out), but for now this doesn't look (much?) worse than what's on Win/Chrome.

BUG=25405
TEST=Drag URLs to the tab strip, move around, make sure the ugly arrow appears in the right locations, dropping to confirm. Repeat with various themes to make sure arrow is reasonably visible.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33924

[viettrungluu, 2009-12-05 05:57:58]

Looking for people who might review this over the weekend.... It's a stop-gap.
I'll remove/replace it soon, I promise. Really.

(Actually, the code shouldn't be that bad, really. It's safe since I don't
introduce anything new which could leak or explode. It's a bit odd that I do
manual drawing rather than just putting up an image from a pdf, but (a) I don't
have a suitable pdf nor a way of drawing one and (b) I would really need a very
perfect pdf since scaling really messes things up at this small size.)

[Nico, 2009-12-05 07:17:32]

Does this work if I drag (but don't drop) a link over a tab to the right of the
active tab, then hit cmd-w and then drop the link? Does it open in the tab that
the arrow points at? (If the dnd code is not buggy, it should work I think).

If so, LG. I don't quite get the urgency though, since this is for M5 and the
dev channel currently comes from the branch anyway.

Some rambling below.

http://codereview.chromium.org/466051/diff/2001/12
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/466051/diff/2001/12#newcode1545
chrome/browser/cocoa/tab_strip_controller.mm:1545: [tabView_
setNeedsDisplay:YES];
/me complains that tabView_ isn't called tabStripView_. Maybe I'll fix it some
day.

http://codereview.chromium.org/466051/diff/2001/14
File chrome/browser/cocoa/tab_strip_view.mm (right):

http://codereview.chromium.org/466051/diff/2001/14#newcode8
chrome/browser/cocoa/tab_strip_view.mm:8: #include "base/nsimage_cache_mac.h"
looks like you don't need this?

http://codereview.chromium.org/466051/diff/2001/14#newcode51
chrome/browser/cocoa/tab_strip_view.mm:51: // This is a slope (with higher being
sharper)....
I think it's well-understood what a slope is; drop the parens. Also, three dots,
or preferrably just one.

FWIW, in my boxy mind it's easier to think about the arrow tip in terms of width
and height of bounding box instead of height of bounding box and slope, but this
is fine.

http://codereview.chromium.org/466051/diff/2001/14#newcode53
chrome/browser/cocoa/tab_strip_view.mm:53: const CGFloat kArrowTipSharpness =
1.2;
"kArrowTipSlope"?

http://codereview.chromium.org/466051/diff/2001/14#newcode65
chrome/browser/cocoa/tab_strip_view.mm:65: DCHECK(availableHeight > 0);
maybe "> 10" or something? a 0.01-pixel high arrow doesn't sound helpful.

http://codereview.chromium.org/466051/diff/2001/14#newcode80
chrome/browser/cocoa/tab_strip_view.mm:80: // Define the arrow's shape!
Yes!

http://codereview.chromium.org/466051/diff/2001/14#newcode94
chrome/browser/cocoa/tab_strip_view.mm:94: [arrow fill];
This whole block is very nice and readable!

[viettrungluu, 2009-12-05 08:04:12]

It's not so urgent, though it makes the CL I committed earlier usable. In that
way it's good, since it'll get those code paths tested earlier (and hence more
often).

Drags on Macs are done in a nested event loop deep in the bowels of Cocoa, from
which Cocoa doesn't let much happen (it'll let you drag, it'll let you press
Esc, and not much else -- so Cmd-W doesn't work).

Thanks, Nico.

http://codereview.chromium.org/466051/diff/2001/12
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/466051/diff/2001/12#newcode1545
chrome/browser/cocoa/tab_strip_controller.mm:1545: [tabView_
setNeedsDisplay:YES];
On 2009/12/05 07:17:32, Nico wrote:
> /me complains that tabView_ isn't called tabStripView_. Maybe I'll fix it some
> day.

/me has complained about that many times too, but still hasn't done anything
about it.

http://codereview.chromium.org/466051/diff/2001/14
File chrome/browser/cocoa/tab_strip_view.mm (right):

http://codereview.chromium.org/466051/diff/2001/14#newcode8
chrome/browser/cocoa/tab_strip_view.mm:8: #include "base/nsimage_cache_mac.h"
On 2009/12/05 07:17:32, Nico wrote:
> looks like you don't need this?

Correct. Gone.

http://codereview.chromium.org/466051/diff/2001/14#newcode51
chrome/browser/cocoa/tab_strip_view.mm:51: // This is a slope (with higher being
sharper)....
On 2009/12/05 07:17:32, Nico wrote:
> I think it's well-understood what a slope is; drop the parens. Also, three
dots,
> or preferrably just one.

The parens came before I pointed out that the arrow is pointing downwards....
Also, note that if a sentence terminates after an ellipsis, then four dots are
called for. But of course one dot is fine in this case.

(Also, you have evidently never taught in the U.S. if you think it's
"well-understood". ;-) )

> FWIW, in my boxy mind it's easier to think about the arrow tip in terms of
width
> and height of bounding box instead of height of bounding box and slope, but
this
> is fine.

It'd have to be a ratio (height:width or vice versa), at which point it's just
either 0.5 or 2 times this. I like it as a slope, since 1 is then clearly the
45-degree shape (and 1.2 is just a bit sharper).

http://codereview.chromium.org/466051/diff/2001/14#newcode53
chrome/browser/cocoa/tab_strip_view.mm:53: const CGFloat kArrowTipSharpness =
1.2;
On 2009/12/05 07:17:32, Nico wrote:
> "kArrowTipSlope"?

Sure. Done.

http://codereview.chromium.org/466051/diff/2001/14#newcode65
chrome/browser/cocoa/tab_strip_view.mm:65: DCHECK(availableHeight > 0);
On 2009/12/05 07:17:32, Nico wrote:
> maybe "> 10" or something? a 0.01-pixel high arrow doesn't sound helpful.

Done.

[Nico, 2009-12-07 05:33:57]

Forgot to ask: Did you file a bug for the non-stopgap-version of this?

[viettrungluu, 2009-12-07 06:13:13]

On 2009/12/07 05:33:57, Nico wrote:
> Forgot to ask: Did you file a bug for the non-stopgap-version of this?

Yup: <http://crbug.com/29496>.