[Athena] Simple pull to refresh implementation

[Athena] Simple pull to refresh implementation

BUG=408964
TEST=Manual

Committed: https://crrev.com/9f334b9eb935190e4246fc4b3a85a5114013033b
Cr-Commit-Position: refs/heads/master@{#295905}

[pkotwicz, 2014-09-10 22:56:21]

pkotwicz@chromium.org changed reviewers:
+ sadrul@chromium.org

[pkotwicz, 2014-09-10 22:56:21]

Sadrul, PTAL

I have asked Albert, and we want a basic implementation of pull to refresh to
work for the demo.

I have added WebContentsDelegate::OverscrollComplete()
We cannot assume that an overscroll action completes when delta_y in
OverscrollUpdate() drops to zero. This occurs, for instance, when a user
switches from scrolling vertically to scrolling horizontally

[sadrul, 2014-09-11 04:33:14]

https://codereview.chromium.org/554393008/diff/1/athena/content/web_activity.cc
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/554393008/diff/1/athena/content/web_activity....
athena/content/web_activity.cc:256:
reload_message_->GetLayer()->SetOpacity(std::min(1.0f, opacity));
opacity will be negative when overscroll_y_ < kDistanceReload.

Try using gfx::Tween::FloatValueBetween() instead.

https://codereview.chromium.org/554393008/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/554393008/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_view_aura.cc:1306:
web_contents_->GetDelegate()->OverscrollComplete();
Since we send updates to the WebContentsDelegate only for vertical scrolls,
should we call this complete for vertical scrolls too (i.e. only when mode ==
NORTH || SOUTH)?

https://codereview.chromium.org/554393008/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/554393008/diff/1/content/public/browser/web_c...
content/public/browser/web_contents_delegate.h:167: virtual void
OverscrollUpdate(int delta_y) {}
Can you add a comment here that |delta_y| is the total amount of overscroll, and
not just the incremental updates?

https://codereview.chromium.org/554393008/diff/1/content/public/browser/web_c...
content/public/browser/web_contents_delegate.h:169: // Invoked when overscroll
completes.
+vertical

[pkotwicz, 2014-09-11 22:45:16]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2014-09-11 22:50:27]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2014-09-11 22:51:40]

Sadrul, can you please take another look?

https://codereview.chromium.org/554393008/diff/1/athena/content/web_activity.cc
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/554393008/diff/1/athena/content/web_activity....
athena/content/web_activity.cc:256:
reload_message_->GetLayer()->SetOpacity(std::min(1.0f, opacity));
Thanks for catching this.

I cannot use gfx::Tween::FloatValueBetween().
- gfx::Tween::FloatValueBetween() does not guarantee the value's range.
- gfx::Tween::FloatValueBetween() is used to convert a value in the range [0,1]
to a value in a range [start, end] (unfortunately not the other way around)

https://codereview.chromium.org/554393008/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/554393008/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_view_aura.cc:1306:
web_contents_->GetDelegate()->OverscrollComplete();
I have changed the code to only send updates when a user is scrolling
vertically. Previously, we would call OverscrollUpdateForWebContentsDelegate(0)
when a user was scrolling horizontally as well

I should probably rename WebContentsDelegate::OverscrollComplete() to
WebContentsDelegate::VerticalOverscrollComplete() and
WebContentsDelegate::OverscrollUpdate() to
WebContentsDelegate::VerticalOverscrollUpdate() ?

[pkotwicz, 2014-09-11 22:56:56]

I will add a test (probably to WebContentsViewAuraTest) once you are happy with
the code

[sadrul, 2014-09-12 04:46:43]

LGTM

[pkotwicz, 2014-09-15 20:21:49]

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

[pkotwicz, 2014-09-15 20:21:50]

mfomitchev@, can you please take a look at the changes in
web_contents_view_aura_browsertest.cc

[pkotwicz, 2014-09-17 00:37:10]

mfomitchev@ Ping?

[mfomitchev, 2014-09-17 21:57:22]

https://codereview.chromium.org/554393008/diff/80001/athena/content/web_activ...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/554393008/diff/80001/athena/content/web_activ...
athena/content/web_activity.cc:271: reload_message_->Hide();
Shouldn't we hide the widget and reset overscroll_y_ to 0 regardless of whether
it passed the reload threshold?

[pkotwicz, 2014-09-18 14:11:43]

Patchset #4 (id:100001) has been deleted

[pkotwicz, 2014-09-18 14:18:23]

mfomitchev@ can you please take another look?

[pkotwicz, 2014-09-18 14:18:32]

https://codereview.chromium.org/554393008/diff/80001/athena/content/web_activ...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/554393008/diff/80001/athena/content/web_activ...
athena/content/web_activity.cc:271: reload_message_->Hide();
Thanks for pointing this out. Fixed!

[mfomitchev, 2014-09-18 17:21:22]

LGTM

[pkotwicz, 2014-09-18 17:24:56]

pkotwicz@chromium.org changed reviewers:
+ jam@chromium.org, oshima@chromium.org

[pkotwicz, 2014-09-18 17:24:57]

jam@ for content OWNERS
oshima@ for athena OWNERS

[oshima, 2014-09-18 18:01:03]

lgtm

https://codereview.chromium.org/554393008/diff/120001/athena/content/web_acti...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/554393008/diff/120001/athena/content/web_acti...
athena/content/web_activity.cc:268:
reload_message_->GetLayer()->SetOpacity(opacity);
I wonder if we should animation opacity change.

https://codereview.chromium.org/554393008/diff/120001/athena/content/web_acti...
athena/content/web_activity.cc:374: views::Label* label = new
views::Label(base::UTF8ToUTF16("Reload"));
I think it's time to stop hard coding strings and use resources instead. Can you
create grd file for strings? (you may do it in separate CL)

[pkotwicz, 2014-09-18 18:47:28]

https://codereview.chromium.org/554393008/diff/120001/athena/content/web_acti...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/554393008/diff/120001/athena/content/web_acti...
athena/content/web_activity.cc:268:
reload_message_->GetLayer()->SetOpacity(opacity);
We get gesture events often enough that I do not think that it is necessary

https://codereview.chromium.org/554393008/diff/120001/athena/content/web_acti...
athena/content/web_activity.cc:374: views::Label* label = new
views::Label(base::UTF8ToUTF16("Reload"));
I will do this in a separate CL

[jam, 2014-09-19 18:43:37]

lgtm

[pkotwicz, 2014-09-21 15:24:35]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-21 15:25:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/554393008/140001

[commit-bot: I haz the power, 2014-09-21 16:23:07]

Committed patchset #5 (id:140001) as 97f728d68d40e80d9359f0073526d19ee34e1601

[commit-bot: I haz the power, 2014-09-21 16:23:33]

Patchset 5 (id:??) landed as
https://crrev.com/9f334b9eb935190e4246fc4b3a85a5114013033b
Cr-Commit-Position: refs/heads/master@{#295905}