highlight intermediate tabs

highlight intermediate tabs


BUG=124830
TEST=Added new tests

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

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

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

[sky, 2013-01-22 18:05:01]

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/tab_scrubber.cc (right):

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:48:
ash::Shell::GetInstance()->AddPreTargetHandler(this);
Should this be removed at some point?

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:59: const gfx::Point
TabScrubber::GetStartPoint(
Make order match header.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:64: gfx::Rect tab_bounds =
tab_strip->tab_at(index)->bounds();
If an animation is occurring bounds() is going to be wrong.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:97: int last_tab_index =
overridden_tab_ == -1 ?
What happens if overrideen_tab_ is no longer a valid index in the model?

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:210: if (browser_ &&
should_cancel_immersive_reveal_) {
What is browser_ changes between the time you start the timer and this fires?
Seems like ImmersiveModeController needs some sort of count so this class and
other places that try to change ImmersiveModeController don't step on each
other.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/tab_scrubber.h (right):

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:30: enum Direction {LEFT, RIGHT};
enums before methods. Also, how does this with when rtl? Would it be better to
use LEADING/TRAILING or INCREASING/DECREASING?

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:34: static const gfx::Point
GetStartPoint(TabStrip* tab_strip,
No point in a const return type.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:58: // The browser that we are
scrubbing.
Update comments for members below. Not all members are obvious. For example,
overriden_tab_ is not clear. Also, document what coordinate system coordinates
are in.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:63: Direction swipe_direction_;
You don't member initialize this.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:75:
DISALLOW_COPY_AND_ASSIGN(TabScrubber);
nit: newline between 74/75

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
File ui/aura/test/event_generator.cc (right):

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.cc:305: base::TimeDelta timestamp =
base::TimeDelta::FromInternalValue(
Should this use ui::EventTimeForNow ?

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.cc:536: void EventGenerator::DoDispatchKeyEvent() {
How about a single member for std::list<Event*> with one DispatchEvent and
DoDispatchEvent members? Seems like it would be a lot less code.

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
File ui/aura/test/event_generator.h (right):

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:23: class TouchEvent;
nit: sort

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:213: const base::TimeDelta& step_delay,
no const & here (I know, confusing).

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:214: const std::vector<gfx::Point> offsets,
const vector&

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:230: void DispatchKeyEvent(ui::KeyEvent&
key_event);
const on all these args.

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:272: // Set to true to cause events to be posted
asynchronously. Event types
This second sentence isn't clear, please clarify it.

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h
File ui/base/events/event.h (right):

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h#ne...
ui/base/events/event.h:203: void set_time_stamp(const base::TimeDelta&
time_stamp) {
We generally don't pass TimeDeltas by const-ref.

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h#ne...
ui/base/events/event.h:277: LocatedEvent(const LocatedEvent& model);
explicit

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h#ne...
ui/base/events/event.h:339: MouseEvent(const MouseEvent& model)
explicit

https://codereview.chromium.org/11881042/diff/22005/ui/base/gestures/gesture_...
File ui/base/gestures/gesture_sequence.cc (right):

https://codereview.chromium.org/11881042/diff/22005/ui/base/gestures/gesture_...
ui/base/gestures/gesture_sequence.cc:723: flags_ | EF_FROM_TOUCH,
Why would we only set EF_FROM_TOUCH for fling starts? If we're going to start
setting EF_FROM_TOUCH, shouldn't it be set every where?

https://codereview.chromium.org/11881042/diff/22005/ui/views/controls/glow_ho...
File ui/views/controls/glow_hover_controller.cc (right):

https://codereview.chromium.org/11881042/diff/22005/ui/views/controls/glow_ho...
ui/views/controls/glow_hover_controller.cc:53: animation_.Show();
Add comment why you do the Show() then End().

[sadrul, 2013-01-22 20:07:24]

Could you please add some more details in the CL description?

It looks like this is turning on tab-scrubbing by default. Is that expected?

You could optionally split this CL up into a few pieces (to help with
revert/merge/bisect etc.):
 * Changes in the EventSender.
 * Add the highlight effect for tab-scrubbing.
 * Turn tab-scrubbing on by default.

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h
File ui/base/events/event.h (right):

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h#ne...
ui/base/events/event.h:145: // events. No gesture events have native events.
Update the comment.

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event_hand...
File ui/base/events/event_handler.cc (right):

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event_hand...
ui/base/events/event_handler.cc:33:
OnScrollEvent(static_cast<ScrollEvent*>(event));
The ordering change here should no longer be necessary, right?

https://codereview.chromium.org/11881042/diff/22005/ui/base/gestures/gesture_...
File ui/base/gestures/gesture_sequence.cc (right):

https://codereview.chromium.org/11881042/diff/22005/ui/base/gestures/gesture_...
ui/base/gestures/gesture_sequence.cc:723: flags_ | EF_FROM_TOUCH,
On 2013/01/22 18:05:01, sky wrote:
> Why would we only set EF_FROM_TOUCH for fling starts? If we're going to start
> setting EF_FROM_TOUCH, shouldn't it be set every where?

I agree. The best place would be to set the flag in the GestureEvent constructor

[sky, 2013-01-22 22:32:38]

+1 on the splitting

On Tue, Jan 22, 2013 at 12:07 PM,  <sadrul@chromium.org> wrote:
> Could you please add some more details in the CL description?
>
> It looks like this is turning on tab-scrubbing by default. Is that expected?
>
> You could optionally split this CL up into a few pieces (to help with
> revert/merge/bisect etc.):
>  * Changes in the EventSender.
>  * Add the highlight effect for tab-scrubbing.
>  * Turn tab-scrubbing on by default.
>
>
>
> https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h
> File ui/base/events/event.h (right):
>
>
https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h#ne...
> ui/base/events/event.h:145: // events. No gesture events have native
> events.
> Update the comment.
>
>
https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event_hand...
> File ui/base/events/event_handler.cc (right):
>
>
https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event_hand...
> ui/base/events/event_handler.cc:33:
> OnScrollEvent(static_cast<ScrollEvent*>(event));
> The ordering change here should no longer be necessary, right?
>
>
>
https://codereview.chromium.org/11881042/diff/22005/ui/base/gestures/gesture_...
> File ui/base/gestures/gesture_sequence.cc (right):
>
>
https://codereview.chromium.org/11881042/diff/22005/ui/base/gestures/gesture_...
> ui/base/gestures/gesture_sequence.cc:723: flags_ | EF_FROM_TOUCH,
> On 2013/01/22 18:05:01, sky wrote:
>>
>> Why would we only set EF_FROM_TOUCH for fling starts? If we're going
>
> to start
>>
>> setting EF_FROM_TOUCH, shouldn't it be set every where?
>
>
> I agree. The best place would be to set the flag in the GestureEvent
> constructor
>
> https://codereview.chromium.org/11881042/

[DaveMoore, 2013-01-27 21:21:54]

I've addressed the comments in this cl so that they're easier to follow, then
split the cl into three as requested:

Event changes: https://codereview.chromium.org/12088015/
Tab highlighting: https://codereview.chromium.org/12096014/
Enabled by default: https://codereview.chromium.org/12087017/

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/tab_scrubber.cc (right):

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:48:
ash::Shell::GetInstance()->AddPreTargetHandler(this);
I thought the registrar takes care of that upon destruction.
On 2013/01/22 18:05:01, sky wrote:
> Should this be removed at some point?

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:59: const gfx::Point
TabScrubber::GetStartPoint(
On 2013/01/22 18:05:01, sky wrote:
> Make order match header.

Done.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:64: gfx::Rect tab_bounds =
tab_strip->tab_at(index)->bounds();
Is there some way to get the right location then? Or should I just refuse to
start / continue a scrub if the tabstrip is animating? Is that discoverable?

On 2013/01/22 18:05:01, sky wrote:
> If an animation is occurring bounds() is going to be wrong.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:97: int last_tab_index =
overridden_tab_ == -1 ?
Added check above and abort if not valid.
On 2013/01/22 18:05:01, sky wrote:
> What happens if overrideen_tab_ is no longer a valid index in the model?

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:210: if (browser_ &&
should_cancel_immersive_reveal_) {
There already was protection against this when browser_ got assigned, but I
added a call to force the reveal cancel to happen immediately.
On 2013/01/22 18:05:01, sky wrote:
> What is browser_ changes between the time you start the timer and this fires?
> Seems like ImmersiveModeController needs some sort of count so this class and
> other places that try to change ImmersiveModeController don't step on each
> other.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/tab_scrubber.h (right):

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:30: enum Direction {LEFT, RIGHT};
Moved declaration, but I think RIGHT / LEFT here means really right or left.
Sidetabs would impact it but they don't exist anymore.

On 2013/01/22 18:05:01, sky wrote:
> enums before methods. Also, how does this with when rtl? Would it be better to
> use LEADING/TRAILING or INCREASING/DECREASING?

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:34: static const gfx::Point
GetStartPoint(TabStrip* tab_strip,
On 2013/01/22 18:05:01, sky wrote:
> No point in a const return type.

Done.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:58: // The browser that we are
scrubbing.
On 2013/01/22 18:05:01, sky wrote:
> Update comments for members below. Not all members are obvious. For example,
> overriden_tab_ is not clear. Also, document what coordinate system coordinates
> are in.

Done.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:63: Direction swipe_direction_;
On 2013/01/22 18:05:01, sky wrote:
> You don't member initialize this.

Done.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:63: Direction swipe_direction_;
On 2013/01/22 18:05:01, sky wrote:
> You don't member initialize this.

Done.

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:75:
DISALLOW_COPY_AND_ASSIGN(TabScrubber);
On 2013/01/22 18:05:01, sky wrote:
> nit: newline between 74/75

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
File ui/aura/test/event_generator.cc (right):

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.cc:305: base::TimeDelta timestamp =
base::TimeDelta::FromInternalValue(
On 2013/01/22 18:05:01, sky wrote:
> Should this use ui::EventTimeForNow ?

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.cc:536: void EventGenerator::DoDispatchKeyEvent() {
rewritten to minimize copied code.
On 2013/01/22 18:05:01, sky wrote:
> How about a single member for std::list<Event*> with one DispatchEvent and
> DoDispatchEvent members? Seems like it would be a lot less code.

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
File ui/aura/test/event_generator.h (right):

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:23: class TouchEvent;
On 2013/01/22 18:05:01, sky wrote:
> nit: sort

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:213: const base::TimeDelta& step_delay,
On 2013/01/22 18:05:01, sky wrote:
> no const & here (I know, confusing).

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:214: const std::vector<gfx::Point> offsets,
On 2013/01/22 18:05:01, sky wrote:
> const vector&

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:230: void DispatchKeyEvent(ui::KeyEvent&
key_event);
That ends up requiring a copy within the call to the RootWindowHostDelegate as
those methods take non const pointers. I thought this was better.
On 2013/01/22 18:05:01, sky wrote:
> const on all these args.

https://codereview.chromium.org/11881042/diff/22005/ui/aura/test/event_genera...
ui/aura/test/event_generator.h:272: // Set to true to cause events to be posted
asynchronously. Event types
On 2013/01/22 18:05:01, sky wrote:
> This second sentence isn't clear, please clarify it.

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h
File ui/base/events/event.h (right):

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h#ne...
ui/base/events/event.h:145: // events. No gesture events have native events.
On 2013/01/22 20:07:24, sadrul wrote:
> Update the comment.

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h#ne...
ui/base/events/event.h:203: void set_time_stamp(const base::TimeDelta&
time_stamp) {
On 2013/01/22 18:05:01, sky wrote:
> We generally don't pass TimeDeltas by const-ref.

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h#ne...
ui/base/events/event.h:277: LocatedEvent(const LocatedEvent& model);
On 2013/01/22 18:05:01, sky wrote:
> explicit

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event.h#ne...
ui/base/events/event.h:339: MouseEvent(const MouseEvent& model)
On 2013/01/22 18:05:01, sky wrote:
> explicit

Done, but AutoFillDialogViews was depending on the existing implicit copy.
That's changed now.

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event_hand...
File ui/base/events/event_handler.cc (right):

https://codereview.chromium.org/11881042/diff/22005/ui/base/events/event_hand...
ui/base/events/event_handler.cc:33:
OnScrollEvent(static_cast<ScrollEvent*>(event));
On 2013/01/22 20:07:24, sadrul wrote:
> The ordering change here should no longer be necessary, right?

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/base/gestures/gesture_...
File ui/base/gestures/gesture_sequence.cc (right):

https://codereview.chromium.org/11881042/diff/22005/ui/base/gestures/gesture_...
ui/base/gestures/gesture_sequence.cc:723: flags_ | EF_FROM_TOUCH,
On 2013/01/22 18:05:01, sky wrote:
> Why would we only set EF_FROM_TOUCH for fling starts? If we're going to start
> setting EF_FROM_TOUCH, shouldn't it be set every where?

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/base/gestures/gesture_...
ui/base/gestures/gesture_sequence.cc:723: flags_ | EF_FROM_TOUCH,
On 2013/01/22 20:07:24, sadrul wrote:
> On 2013/01/22 18:05:01, sky wrote:
> > Why would we only set EF_FROM_TOUCH for fling starts? If we're going to
start
> > setting EF_FROM_TOUCH, shouldn't it be set every where?
> 
> I agree. The best place would be to set the flag in the GestureEvent
constructor

Done.

https://codereview.chromium.org/11881042/diff/22005/ui/views/controls/glow_ho...
File ui/views/controls/glow_hover_controller.cc (right):

https://codereview.chromium.org/11881042/diff/22005/ui/views/controls/glow_ho...
ui/views/controls/glow_hover_controller.cc:53: animation_.Show();
On 2013/01/22 18:05:01, sky wrote:
> Add comment why you do the Show() then End().

Done.

[sky, 2013-01-28 19:18:52]

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/tab_scrubber.cc (right):

https://codereview.chromium.org/11881042/diff/22005/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:64: gfx::Rect tab_bounds =
tab_strip->tab_at(index)->bounds();
On 2013/01/27 21:21:54, DaveMoore wrote:
> Is there some way to get the right location then?

You can use ideal_bounds(index) to get the ideal bounds.

> Or should I just refuse to
> start / continue a scrub if the tabstrip is animating? Is that discoverable?

If an animation is occurring it's a good indication something else is happening
to the tabstrip (eg closing, moving...). Doing nothing in this case is fine too.

> 
> On 2013/01/22 18:05:01, sky wrote:
> > If an animation is occurring bounds() is going to be wrong.
>

[DaveMoore, 2013-02-06 22:47:50]

I added support for tracking the highlighted tab if the tab order changes while
waiting to activate along with tests.

[sky, 2013-02-06 22:55:16]

LGTM

https://codereview.chromium.org/11881042/diff/55001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/tab_scrubber.cc (right):

https://codereview.chromium.org/11881042/diff/55001/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.cc:234: if (highlighted_tab_ == -1)
Do you really need this? Maybe you should nuke the Observe above and instead do
cleanup here?

https://codereview.chromium.org/11881042/diff/55001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/tab_scrubber.h (right):

https://codereview.chromium.org/11881042/diff/55001/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/tab_scrubber.h:98: base::WeakPtrFactory<TabScrubber>
weak_ptr_factory_;
I would recommend explicitly doing this first in the destructor. It's easy to
miss a comment like this here, but harder when there is something explicit in
the destructor.