Implement MD simple gesture nav

Implement MD simple gesture nav

This CL implements Material Design version of simple gesture nav to
replace the existing overscroll effect.

As a part of this CL, back and forward navigation icons are moved to
ui/vector_icons/ so it can be accessible in contents/, too.

BUG=668296
Review-Url: https://codereview.chromium.org/2656463002
Cr-Commit-Position: refs/heads/master@{#450272}
Committed: https://chromium.googlesource.com/chromium/src/+/c8b9ca7e60bf607a91e1f73a9bc12e8cde0940e4

[mohsen, 2017-01-23 21:47:53]

mohsen@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[mohsen, 2017-01-23 21:47:54]

mfomitchev@: FYI

[mfomitchev, 2017-01-23 23:04:43]

Thanks Mohsen!

I've posted some comments, but since this is WIP, they are not comprehensive.
Are you planning to finish this up and get it landed? If you'd like me to take
it over instead, that would be okay too.

[mfomitchev, 2017-01-23 23:04:52]

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/gesture_nav_simple.cc (right):

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:39: const unsigned int
kMaxRippleOpacity = 0x66;
Would it be better to use U8CPU as the type here, since this is what SkColorSetA
and setAlpha take?

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:63: // TODO: don't
inline
Agree

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:75:
DISALLOW_COPY_AND_ASSIGN(Delegate);
Should be in the private section

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:134:
drag_progress_(0.f),
I recently learned that it's preferred to do this sort of initializations at
declaration point rather than in the constructor. 

See chromioum-dev thread:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/inline...

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:239: float progress = (1
- abort_progress_) * drag_progress_;
A DCHECK similar to this would be helpful here (both for correctness and as a
pseudo comment).
DCHECK(abort_progress_ == 0 || state != ABORTING)

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:291: if (delegate_)
Can we just have Affordance delete itself here and get rid of
Affordance::Delegate/AffordanceDone()?

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/gesture_nav_simple.h (right):

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.h:26: class
AffordanceDeleter;
Doesn't seem like this needs to be forward-declared?

[mohsen, 2017-01-27 21:54:18]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 21:55:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-01-27 21:57:54]

Description was changed from

==========
[WIP] Implement MD simple gesture nav

BUG=668296
==========

to

==========
Implement MD simple gesture nav

This CL implements Material Design version of simple gesture nav to
replace the existing overscroll effect.

As a part of this CL, back and forward navigation icons are moved to
ui/resources/ so it can be accessible in contents/, too.

BUG=668296
==========

[commit-bot: I haz the power, 2017-01-27 22:01:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:01:30]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mohsen, 2017-01-27 22:46:14]

Patchset #3 (id:40001) has been deleted

[mohsen, 2017-01-27 22:48:40]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 22:49:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 22:54:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:54:36]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mohsen, 2017-01-27 23:21:59]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 23:22:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 23:25:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 23:25:50]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mohsen, 2017-01-27 23:29:48]

Patchset #4 (id:80001) has been deleted

[mohsen, 2017-01-27 23:31:01]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 23:31:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 23:38:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 23:38:50]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mohsen, 2017-01-31 18:40:12]

Patchset #5 (id:120001) has been deleted

[mohsen, 2017-01-31 18:58:44]

Patchset #5 (id:140001) has been deleted

[mohsen, 2017-01-31 19:23:48]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 19:24:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 19:30:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:30:45]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mohsen, 2017-01-31 19:43:37]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 19:44:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-01-31 21:10:35]

mfomitchev@: The CL is ready for review. Can you please take another look?

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/gesture_nav_simple.cc (right):

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:39: const unsigned int
kMaxRippleOpacity = 0x66;
On 2017/01/23 at 23:04:52, mfomitchev wrote:
> Would it be better to use U8CPU as the type here, since this is what
SkColorSetA and setAlpha take?

I will actually move it to the kRippleColor declaration and get rid of this
variable entirely.

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:75:
DISALLOW_COPY_AND_ASSIGN(Delegate);
On 2017/01/23 at 23:04:51, mfomitchev wrote:
> Should be in the private section

Done.

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:134:
drag_progress_(0.f),
On 2017/01/23 at 23:04:51, mfomitchev wrote:
> I recently learned that it's preferred to do this sort of initializations at
declaration point rather than in the constructor. 
> 
> See chromioum-dev thread:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/inline...

Done.

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:239: float progress = (1
- abort_progress_) * drag_progress_;
On 2017/01/23 at 23:04:52, mfomitchev wrote:
> A DCHECK similar to this would be helpful here (both for correctness and as a
pseudo comment).
> DCHECK(abort_progress_ == 0 || state != ABORTING)

Done.

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:291: if (delegate_)
On 2017/01/23 at 23:04:52, mfomitchev wrote:
> Can we just have Affordance delete itself here and get rid of
Affordance::Delegate/AffordanceDone()?

Sure. That would make it simpler.

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/gesture_nav_simple.h (right):

https://codereview.chromium.org/2656463002/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.h:26: class
AffordanceDeleter;
On 2017/01/23 at 23:04:52, mfomitchev wrote:
> Doesn't seem like this needs to be forward-declared?

I declared Affordance class privately nested in GestureNavSimple to avoid
forward-declaring it publicly. Because AffordanceDeleter needed access to
Affordance, I had to declare it nested in SimpleGestureNav, too. That's why I've
forward-declared it here.

Anyways, I'm getting rid of AffordanceDeleter as you suggested in another
comment. However, we could've moved these out to their own files to avoid this
much nesting!

[commit-bot: I haz the power, 2017-01-31 21:40:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 21:40:25]

Dry run: This issue passed the CQ dry run.

[mfomitchev, 2017-01-31 22:54:13]

Awesome, thanks Mohsen!
I am just wondering - perhaps putting the new resources under content/resources
would be better than ui/resources? Logically content it seems like it would be
better to put the resources at the highest level possible. Chrome is on top of
content on top of ui. We need the resources in content and chrome, but not in
ui, so seems like content would be a better place?

[mfomitchev, 2017-01-31 22:55:46]

On 2017/01/31 22:54:13, mfomitchev wrote:
> Awesome, thanks Mohsen!
> I am just wondering - perhaps putting the new resources under
content/resources
> would be better than ui/resources? Logically content it seems like it would be
> better to put the resources at the highest level possible. Chrome is on top of
> content on top of ui. We need the resources in content and chrome, but not in
> ui, so seems like content would be a better place?

Sorry, the resources are not new, but I think my point still stands.

[mohsen, 2017-02-01 17:32:56]

On 2017/01/31 at 22:55:46, mfomitchev wrote:
> On 2017/01/31 22:54:13, mfomitchev wrote:
> > Awesome, thanks Mohsen!
> > I am just wondering - perhaps putting the new resources under
content/resources
> > would be better than ui/resources? Logically content it seems like it would
be
> > better to put the resources at the highest level possible. Chrome is on top
of
> > content on top of ui. We need the resources in content and chrome, but not
in
> > ui, so seems like content would be a better place?
> 
> Sorry, the resources are not new, but I think my point still stands.

From what I understand from DEPS files, chrome/ only has access to parts of
content/public. Right?

[mfomitchev, 2017-02-01 20:29:23]

On 2017/02/01 17:32:56, mohsen wrote:
> On 2017/01/31 at 22:55:46, mfomitchev wrote:
> > On 2017/01/31 22:54:13, mfomitchev wrote:
> > > Awesome, thanks Mohsen!
> > > I am just wondering - perhaps putting the new resources under
> content/resources
> > > would be better than ui/resources? Logically content it seems like it
would
> be
> > > better to put the resources at the highest level possible. Chrome is on
top
> of
> > > content on top of ui. We need the resources in content and chrome, but not
> in
> > > ui, so seems like content would be a better place?
> > 
> > Sorry, the resources are not new, but I think my point still stands.
> 
> From what I understand from DEPS files, chrome/ only has access to parts of
> content/public. Right?

Looks like chrome/browser has a dependency on content/app/resources.
content/browser also has a dependency on that. So maybe that would be a good
place for these to live?

[mfomitchev, 2017-02-03 02:14:37]

Looks great, thanks Mohsen!
Comments below, mostly nits.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
File content/browser/web_contents/aura/gesture_nav_simple.cc (left):

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:223: // Adding the arrow
as a child of the content window is not sufficient,
Nit: This comment seems like it might be worth updating and preserving (probably
in the Affordance class).

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
File content/browser/web_contents/aura/gesture_nav_simple.cc (right):

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:43: // Parameters
defining the affordance ripple.
Nit: Maybe explain how ripple and burst work?

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:92: // Returns the root
layer of the affordance to be added to the appropriate
Nit: We could drop "to be added to the appropriate layer...". That sort of
comment fits better at the usage site IMHO. (e.g. if we change the way the
public API is used without changing the way it works, we shouldn't need to
update the API comments)

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:148:
root_layer_.reset(new ui::Layer(ui::LAYER_NOT_DRAWN));
I believe it's preferred to use base::MakeUnique (there was a thread on
chromium-dev about it). With MakeUnique I think you could also move the
initialization to initialization list. Ditto for the other layer.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:155:
painted_layer_->SetBounds(gfx::Rect(
Nit: There is a lot going on in this one statement making it a bit hard to
parse? Perhaps factor out the x coordinate definition?

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:249: gfx::PointF
center_point(kMaxRippleBurstRadius, kMaxRippleBurstRadius);
Make this a constexpr in the anonymous namespace?

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:268: // Draw background.
Nit: Smth like:
"Draw the arrow background circle with the shadow." 
Just to make it a bit more clear?

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:281: float arrow_y =
center_point.y() - kArrowSize / 2.f;
These two can be constexpr as well.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:282: float
arrow_x_offset =
Nit: Might be nice to comment that this moves the arrow relative to the circular
background (while the circular background itself along with the ripple are moved
via a transform).

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:315: break;
Nit: Consider putting UpdateTransform() + SchedulePaint at the end of this
method and removing from SetAbortProgress/SetCompleteProgress (less code
duplication).

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:354: if (!affordance_)
DCHECK that we are in the DRAGGING state?

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:358: return true;
If you end up moving UpdateTransform()/SchedulePaint() to AnimationProgressed,
we should move them here out of SetDragProgress as well for consistency.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:385:
GetOverscrollConfig(OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START_TOUCHSCREEN);
Why do we need to offset completion threshold by
OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START_TOUCHSCREEN? Even if this is needed, it
should probably be done in OverscrollController? Also, it depends on whether we
are using a touchpad or touchscreen.

[mohsen, 2017-02-03 22:32:00]

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
File content/browser/web_contents/aura/gesture_nav_simple.cc (left):

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:223: // Adding the arrow
as a child of the content window is not sufficient,
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> Nit: This comment seems like it might be worth updating and preserving
(probably in the Affordance class).

Done. (here, not in the Affordance class).

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
File content/browser/web_contents/aura/gesture_nav_simple.cc (right):

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:43: // Parameters
defining the affordance ripple.
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> Nit: Maybe explain how ripple and burst work?

Done. Please check.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:92: // Returns the root
layer of the affordance to be added to the appropriate
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> Nit: We could drop "to be added to the appropriate layer...". That sort of
comment fits better at the usage site IMHO. (e.g. if we change the way the
public API is used without changing the way it works, we shouldn't need to
update the API comments)

Done.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:148:
root_layer_.reset(new ui::Layer(ui::LAYER_NOT_DRAWN));
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> I believe it's preferred to use base::MakeUnique (there was a thread on
chromium-dev about it). With MakeUnique I think you could also move the
initialization to initialization list. Ditto for the other layer.

Done. Done.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:155:
painted_layer_->SetBounds(gfx::Rect(
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> Nit: There is a lot going on in this one statement making it a bit hard to
parse? Perhaps factor out the x coordinate definition?

Done.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:249: gfx::PointF
center_point(kMaxRippleBurstRadius, kMaxRippleBurstRadius);
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> Make this a constexpr in the anonymous namespace?

I prefer not to make this (and |arrow_x| and |arrow_y| down) constexpr. I feel
defining them somewhere else makes the code more difficult to understand that
isn't worth the micro-optimization. Also, these are just put into some temporary
variables here to make the lines shorter and the code more readable. If I want
to perform this optimization, then why not doing that for all other "constant"
calculations? (e.g. (kMaxRippleBurstRadius - kMaxRippleRadius) on line 261, or
gfx::Vector2d(0, kBgShadowOffsetY) on line 274, etc.)

WDYT?

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:268: // Draw background.
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> Nit: Smth like:
> "Draw the arrow background circle with the shadow." 
> Just to make it a bit more clear?

Done.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:281: float arrow_y =
center_point.y() - kArrowSize / 2.f;
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> These two can be constexpr as well.

Same as above. Also, |arrow_x| is modified on line 284.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:282: float
arrow_x_offset =
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> Nit: Might be nice to comment that this moves the arrow relative to the
circular background (while the circular background itself along with the ripple
are moved via a transform).

Done.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:315: break;
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> Nit: Consider putting UpdateTransform() + SchedulePaint at the end of this
method and removing from SetAbortProgress/SetCompleteProgress (less code
duplication).

UpdateTransfrom() is only needed in SetAbortProgress().

More importantly, IMO, those function calls are conceptually part of
SetAbortProgress/SetCompleteProgress (similar to opacity that is set in
SetCompleteProgress). If for example we need to call these functions
(Set*Progress) from somewhere else, we shouldn't need to call those
(UpdateTransform/SchedulePaint) again.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:354: if (!affordance_)
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> DCHECK that we are in the DRAGGING state?

State is an internal detail of Affordance not visible here.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:358: return true;
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> If you end up moving UpdateTransform()/SchedulePaint() to AnimationProgressed,
we should move them here out of SetDragProgress as well for consistency.

It's even more obvious here. SetDragProgress is a public method that, from POV
of Affordance, might be called by anyone. The caller should not be made
responsible for calling UpdateTransform/SchedulePaint.

https://codereview.chromium.org/2656463002/diff/180001/content/browser/web_co...
content/browser/web_contents/aura/gesture_nav_simple.cc:385:
GetOverscrollConfig(OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START_TOUCHSCREEN);
On 2017/02/03 at 02:14:37, mfomitchev wrote:
> Why do we need to offset completion threshold by
OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START_TOUCHSCREEN? Even if this is needed, it
should probably be done in OverscrollController? Also, it depends on whether we
are using a touchpad or touchscreen.

Overscroll starts at threshold_start and completes at threshold_complete.
However the delta value that is passed in is between (threshold_start) and
(threshold_complete - threshold_start) (i.e. what I'm calculating here). There
is only one threshold_complete for both touchscreen and touchpad and the value
for touchpad_threshod_start and touchscreen_threshold_start happen to be the
same for the moment (50 which is hard-coded in the old simple gesture nav code,
line 174). We should either pass in the device type so that we can choose the
correct value, or pass in the calculated values. What do you think? Maybe in a
follow-up CL.

[mfomitchev, 2017-02-06 22:45:58]

LGTM

Thanks Mohsen!

[mohsen, 2017-02-07 20:35:58]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 20:36:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 21:45:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 21:45:34]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-02-07 22:17:45]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 22:18:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-02-07 22:22:45]

mohsen@chromium.org changed reviewers:
+ estade@chromium.org

[mohsen, 2017-02-07 22:22:46]

estade@: Can you take a look at changes to vector icon targets?

[commit-bot: I haz the power, 2017-02-07 23:37:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 23:37:52]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-02-08 00:00:25]

https://codereview.chromium.org/2656463002/diff/240001/ui/resources/vector_ic...
File ui/resources/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2656463002/diff/240001/ui/resources/vector_ic...
ui/resources/vector_icons/BUILD.gn:7:
aggregate_vector_icons("ui_resources_vector_icons") {
I just added a target for this --- ui/vector_icons/BUILD.gn

https://codereview.chromium.org/2656463002/diff/240001/ui/resources/vector_ic...
ui/resources/vector_icons/BUILD.gn:11: "navigate_back.1x.icon",
seems these are no longer used exclusively for navigation. Maybe back_arrow or
go_back now makes more sense?

[mohsen, 2017-02-08 03:44:35]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 03:44:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-02-08 03:54:56]

https://codereview.chromium.org/2656463002/diff/240001/ui/resources/vector_ic...
File ui/resources/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2656463002/diff/240001/ui/resources/vector_ic...
ui/resources/vector_icons/BUILD.gn:7:
aggregate_vector_icons("ui_resources_vector_icons") {
On 2017/02/08 at 00:00:24, Evan Stade wrote:
> I just added a target for this --- ui/vector_icons/BUILD.gn

Switched to that target...

https://codereview.chromium.org/2656463002/diff/240001/ui/resources/vector_ic...
ui/resources/vector_icons/BUILD.gn:11: "navigate_back.1x.icon",
On 2017/02/08 at 00:00:24, Evan Stade wrote:
> seems these are no longer used exclusively for navigation. Maybe back_arrow or
go_back now makes more sense?

Sure. Done.

[commit-bot: I haz the power, 2017-02-08 04:33:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 04:33:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2017-02-08 16:07:59]

we should be able to remove IDR_BACK_ARROW now, no?

[Evan Stade, 2017-02-08 16:08:10]

https://codereview.chromium.org/2656463002/diff/260001/ui/vector_icons/BUILD.gn
File ui/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2656463002/diff/260001/ui/vector_icons/BUILD....
ui/vector_icons/BUILD.gn:13: "back_arrow.1x.icon",
nit: alphabetize

[mohsen, 2017-02-08 16:51:17]

Description was changed from

==========
Implement MD simple gesture nav

This CL implements Material Design version of simple gesture nav to
replace the existing overscroll effect.

As a part of this CL, back and forward navigation icons are moved to
ui/resources/ so it can be accessible in contents/, too.

BUG=668296
==========

to

==========
Implement MD simple gesture nav

This CL implements Material Design version of simple gesture nav to
replace the existing overscroll effect.

As a part of this CL, back and forward navigation icons are moved to
ui/vector_icons/ so it can be accessible in contents/, too.

BUG=668296
==========

[mohsen, 2017-02-08 17:37:32]

Patchset #11 (id:280001) has been deleted

[mohsen, 2017-02-08 18:38:31]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 18:39:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-02-08 19:00:52]

Patchset #11 (id:290001) has been deleted

[mohsen, 2017-02-08 19:01:36]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 19:02:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-02-08 19:19:13]

Patchset #11 (id:310001) has been deleted

[mohsen, 2017-02-08 19:21:59]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 19:23:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 20:42:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 20:42:09]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-02-08 20:52:04]

Removed IDR_BACK_ARROW, IDR_FORWARD_ARROW. PTAL.

https://codereview.chromium.org/2656463002/diff/260001/ui/vector_icons/BUILD.gn
File ui/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2656463002/diff/260001/ui/vector_icons/BUILD....
ui/vector_icons/BUILD.gn:13: "back_arrow.1x.icon",
On 2017/02/08 at 16:08:10, Evan Stade wrote:
> nit: alphabetize

Oops! Done.

[Evan Stade, 2017-02-08 21:17:11]

lgtm

[mohsen, 2017-02-08 21:31:17]

mohsen@chromium.org changed reviewers:
+ jam@chromium.org

[mohsen, 2017-02-08 21:31:18]

jam@: Can you please take an OWNERS look...

[jam, 2017-02-08 22:48:59]

On 2017/02/08 21:31:18, mohsen wrote:
> jam@: Can you please take an OWNERS look...

which files specifically?

[mohsen, 2017-02-08 23:16:10]

Mainly:
 - content/browser/DEPS,
 - content/browser/BUILD.gn.

Also, I'd apperciate if you rubber stamp changes in:
 - chrome/app/vector_icons/,
 - chrome/browser/ui,
 - ui/resources,
which are just moving around some resources.

If these changes are fine, then I'll ask an OWNER to review:
 - ui/base/cocoa/three_part_image_unittest.mm.

[mohsen, 2017-02-10 20:25:34]

On 2017/02/08 at 23:16:10, mohsen wrote:
> Mainly:
>  - content/browser/DEPS,
>  - content/browser/BUILD.gn.
> 
> Also, I'd apperciate if you rubber stamp changes in:
>  - chrome/app/vector_icons/,
>  - chrome/browser/ui,
>  - ui/resources,
> which are just moving around some resources.
> 
> If these changes are fine, then I'll ask an OWNER to review:
>  - ui/base/cocoa/three_part_image_unittest.mm.

jam@: ping...

[jam, 2017-02-13 21:05:28]

lgtm

[mohsen, 2017-02-13 21:16:44]

mohsen@chromium.org changed reviewers:
+ tapted@chromium.org

[mohsen, 2017-02-13 21:16:45]

tapted@: Can you please take a look at
ui/base/cocoa/three_part_image_unittest.mm?

[tapted, 2017-02-13 22:29:09]

https://codereview.chromium.org/2656463002/diff/330001/ui/base/cocoa/three_pa...
File ui/base/cocoa/three_part_image_unittest.mm (right):

https://codereview.chromium.org/2656463002/diff/330001/ui/base/cocoa/three_pa...
ui/base/cocoa/three_part_image_unittest.mm:55: // Create a bitmap with
transparent corners.
update - see below

https://codereview.chromium.org/2656463002/diff/330001/ui/base/cocoa/three_pa...
ui/base/cocoa/three_part_image_unittest.mm:57: const int corener_size = 8;
corener_size -> corner_size

https://codereview.chromium.org/2656463002/diff/330001/ui/base/cocoa/three_pa...
ui/base/cocoa/three_part_image_unittest.mm:78: base::scoped_nsobject<NSImage>
rightImage(part_image.CopyNSImage());
This can be simpler (and I think more compelling) by creating a bitmap that
looks like

+---+
|   |
+---+
|xxx|
+---+
|   |
+---+

and using the same image on both sides.

The code below this comment is good, but above becomes something like

  // Clear top and bottom.
  bitmap.erase(SK_ColorTRANSPARENT, SkIRect::MakeXYWH(0, 0, size, corner_size));
  bitmap.erase(SK_ColorTRANSPARENT,
               SkIRect::MakeXYWH(0, size - corner_size, size, corner_size));
  gfx::Image part_image = gfx::Image::CreateFrom1xBitmap(bitmap);

  // Create a three-part image.
  base::scoped_nsobject<NSImage> ns_image(part_image.CopyNSImage());
  ThreePartImage image(ns_image, nullptr, ns_image);


The description of the bitmap needs to be updated, but "corner_size" is still a
good name, since that's what they become once in the combined image.

(we could also move ThreePartImage under chrome/browser and use
IDR_TAB_ALPHA_LEFT/IDR_TAB_ACTIVE_CENTER/IDR_TAB_ALPHA_RIGHT but that's probably
a bigger yak to shave)

[mohsen, 2017-02-14 03:51:01]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 03:51:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2017-02-14 04:54:31]

https://codereview.chromium.org/2656463002/diff/330001/ui/base/cocoa/three_pa...
File ui/base/cocoa/three_part_image_unittest.mm (right):

https://codereview.chromium.org/2656463002/diff/330001/ui/base/cocoa/three_pa...
ui/base/cocoa/three_part_image_unittest.mm:55: // Create a bitmap with
transparent corners.
On 2017/02/13 at 22:29:09, tapted wrote:
> update - see below

Done.

https://codereview.chromium.org/2656463002/diff/330001/ui/base/cocoa/three_pa...
ui/base/cocoa/three_part_image_unittest.mm:57: const int corener_size = 8;
On 2017/02/13 at 22:29:09, tapted wrote:
> corener_size -> corner_size

Done.

https://codereview.chromium.org/2656463002/diff/330001/ui/base/cocoa/three_pa...
ui/base/cocoa/three_part_image_unittest.mm:78: base::scoped_nsobject<NSImage>
rightImage(part_image.CopyNSImage());
On 2017/02/13 at 22:29:09, tapted wrote:
> This can be simpler (and I think more compelling) by creating a bitmap that
looks like
> 
> +---+
> |   |
> +---+
> |xxx|
> +---+
> |   |
> +---+
> 
> and using the same image on both sides.
> 
> The code below this comment is good, but above becomes something like
> 
>   // Clear top and bottom.
>   bitmap.erase(SK_ColorTRANSPARENT, SkIRect::MakeXYWH(0, 0, size,
corner_size));
>   bitmap.erase(SK_ColorTRANSPARENT,
>                SkIRect::MakeXYWH(0, size - corner_size, size, corner_size));
>   gfx::Image part_image = gfx::Image::CreateFrom1xBitmap(bitmap);
> 
>   // Create a three-part image.
>   base::scoped_nsobject<NSImage> ns_image(part_image.CopyNSImage());
>   ThreePartImage image(ns_image, nullptr, ns_image);
> 
> 
> The description of the bitmap needs to be updated, but "corner_size" is still
a good name, since that's what they become once in the combined image.
> 
> (we could also move ThreePartImage under chrome/browser and use
IDR_TAB_ALPHA_LEFT/IDR_TAB_ACTIVE_CENTER/IDR_TAB_ALPHA_RIGHT but that's probably
a bigger yak to shave)

Done.

[commit-bot: I haz the power, 2017-02-14 05:21:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 05:21:21]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-02-14 05:23:59]

lgtm - thanks!

[mohsen, 2017-02-14 05:34:36]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2017-02-14 05:34:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
mfomitchev@chromium.org, jam@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2656463002/#ps350001
(title: "Simplified ThreePartImage test")

[commit-bot: I haz the power, 2017-02-14 05:34:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 05:39:46]

CQ is committing da patch.

Bot data: {"patchset_id": 350001, "attempt_start_ts": 1487050476735940,
"parent_rev": "496302a831fcd16eb152761df164daadf1d6ada6", "commit_rev":
"c8b9ca7e60bf607a91e1f73a9bc12e8cde0940e4"}

[commit-bot: I haz the power, 2017-02-14 05:40:20]

Description was changed from

==========
Implement MD simple gesture nav

This CL implements Material Design version of simple gesture nav to
replace the existing overscroll effect.

As a part of this CL, back and forward navigation icons are moved to
ui/vector_icons/ so it can be accessible in contents/, too.

BUG=668296
==========

to

==========
Implement MD simple gesture nav

This CL implements Material Design version of simple gesture nav to
replace the existing overscroll effect.

As a part of this CL, back and forward navigation icons are moved to
ui/vector_icons/ so it can be accessible in contents/, too.

BUG=668296

Review-Url: https://codereview.chromium.org/2656463002
Cr-Commit-Position: refs/heads/master@{#450272}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8b9ca7e60bf607a91e1f73a9bc1...
==========

[commit-bot: I haz the power, 2017-02-14 05:40:23]

Committed patchset #12 (id:350001) as
https://chromium.googlesource.com/chromium/src/+/c8b9ca7e60bf607a91e1f73a9bc1...