Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10913277:
Upstream ContentViewCore.pageUp/pageDown (Closed)
|
Created:
8 years, 3 months ago by Ramya Modified:
8 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpstream ContentViewCore.pageUp/pageDown
BUG=146001
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158348
Patch Set 1 #
Total comments: 23
Patch Set 2 : Apply feedback from comments #
Total comments: 8
Patch Set 3 : Updates based on feedback comments #
Messages
Total messages: 13 (0 generated)
https://chromiumcodereview.appspot.com/10913277/diff/1/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://chromiumcodereview.appspot.com/10913277/diff/1/content/browser/androi... content/browser/android/content_view_core_impl.h:144: virtual void UpdateContentSize(int width, int height) OVERRIDE; are these defined in content_view_core.h? If not, they shouldn't have the virtual or OVERRIDE tags. https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:567: * @see {@link android.webkit.WebView#getContentHeight()} the format should be: @see android.webkit.WebView#getContentHeight() Same everywhere else. The {@link ...} business is not needed in combination with the @see. https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:576: public int getContentWidth() { also, these are only used by WebView, so we don't really need to upstream them either. That being said, exposing just the member variables will probably be more work than leaving these methods here. https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:883: public boolean dispatchKeyEvent(KeyEvent event) { might want to loop in Oli to make sure he's not changing this bit of logic. https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:904: if (mNativeContentViewCore != 0) { I would do: if (mNativeContentViewCore == 0) return; Then reduce the indent of the following. https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1063: mSelectionHandleController = new SelectionHandleController(getContainerView()) { is this needed by pageUp/pageDown? If not, might want to leave this out of this change. https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1076: (x1 + mNativeScrollX) / mNativePageScaleFactor, +4 indent here (and below) https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1487: * @See {@link android.webkit.WebView#pageUp(boolean) these are both only used in WebView, so we probably should work on removing them instead of upstreaming it sadly. The comments apply to the above method. https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1490: if (mNativeScrollY == 0) { to get rid of referencing this directly, you can use computeVerticalScrollOffset() https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1496: scrollTo(mNativeScrollX, 0); s/mNativeScrollX/computeHorizontalScrollOffset() https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... File content/public/android/java/src/org/chromium/content/browser/SelectionHandleController.java (right): https://chromiumcodereview.appspot.com/10913277/diff/1/content/public/android... content/public/android/java/src/org/chromium/content/browser/SelectionHandleController.java:27: public void setStartHandlePosition(int x, int y) { Again, maybe leave out of this change?
Just chatted and we probably want to move the pageUp/pageDown to AwContents. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:896: && mImeAdapter .dispatchKeyEvent(event)) remove space before .
On 2012/09/20 18:20:33, Ted C wrote: > Just chatted and we probably want to move the pageUp/pageDown to AwContents. > > http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:896: > && mImeAdapter .dispatchKeyEvent(event)) > remove space before . joth said otherwise on http://code.google.com/p/chromium/issues/detail?id=146001 but I haven't looked at the specifics
On 2012/09/20 18:25:25, Yaron wrote: > On 2012/09/20 18:20:33, Ted C wrote: > > Just chatted and we probably want to move the pageUp/pageDown to AwContents. > > > > > http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... > > File > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > (right): > > > > > http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:896: > > && mImeAdapter .dispatchKeyEvent(event)) > > remove space before . > > joth said otherwise on http://code.google.com/p/chromium/issues/detail?id=146001 > but I haven't looked at the specifics Hmm...it looks like all the necessary bits needed for pageUp/pageDown are exposed (as computeVerticalScrollRange(), computeVerticalScrollOffset(), etc...). Like the canScrollUp would be: return computeVerticalScrollOffset() >= computeVerticalScrollRange() - getHeight(); But maybe that is just a bit too much internal knowledge. I won't object to leaving them in ContentView, but moving them doesn't look "too" difficult either.
PTAL Oli, Can you please take a look at methods like dispatchKeyEvent() in ContentViewCore.java and let me know if it ok to upstream these methods? http://codereview.chromium.org/10913277/diff/1/content/browser/android/conten... File content/browser/android/content_view_core_impl.h (right): http://codereview.chromium.org/10913277/diff/1/content/browser/android/conten... content/browser/android/content_view_core_impl.h:144: virtual void UpdateContentSize(int width, int height) OVERRIDE; On 2012/09/18 00:21:40, Ted C wrote: > are these defined in content_view_core.h? > > If not, they shouldn't have the virtual or OVERRIDE tags. Done. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:567: * @see {@link android.webkit.WebView#getContentHeight()} On 2012/09/18 00:21:40, Ted C wrote: > the format should be: > > @see android.webkit.WebView#getContentHeight() > > Same everywhere else. The {@link ...} business is not needed in combination > with the @see. Done. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:883: public boolean dispatchKeyEvent(KeyEvent event) { On 2012/09/18 00:21:40, Ted C wrote: > might want to loop in Oli to make sure he's not changing this bit of logic. Done. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:896: && mImeAdapter .dispatchKeyEvent(event)) On 2012/09/20 18:20:33, Ted C wrote: > remove space before . Done. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:904: if (mNativeContentViewCore != 0) { On 2012/09/18 00:21:40, Ted C wrote: > I would do: > if (mNativeContentViewCore == 0) return; > > Then reduce the indent of the following. Done. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1063: mSelectionHandleController = new SelectionHandleController(getContainerView()) { On 2012/09/18 00:21:40, Ted C wrote: > is this needed by pageUp/pageDown? If not, might want to leave this out of this > change. Done. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1076: (x1 + mNativeScrollX) / mNativePageScaleFactor, On 2012/09/18 00:21:40, Ted C wrote: > +4 indent here (and below) Done. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1487: * @See {@link android.webkit.WebView#pageUp(boolean) On 2012/09/18 00:21:40, Ted C wrote: > these are both only used in WebView, so we probably should work on removing them > instead of upstreaming it sadly. > > The comments apply to the above method. Keeping it here as per the bug. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1490: if (mNativeScrollY == 0) { On 2012/09/18 00:21:40, Ted C wrote: > to get rid of referencing this directly, you can use > computeVerticalScrollOffset() Done. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1496: scrollTo(mNativeScrollX, 0); On 2012/09/18 00:21:40, Ted C wrote: > s/mNativeScrollX/computeHorizontalScrollOffset() Done. http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... File content/public/android/java/src/org/chromium/content/browser/SelectionHandleController.java (right): http://codereview.chromium.org/10913277/diff/1/content/public/android/java/sr... content/public/android/java/src/org/chromium/content/browser/SelectionHandleController.java:27: public void setStartHandlePosition(int x, int y) { On 2012/09/18 00:21:40, Ted C wrote: > Again, maybe leave out of this change? Done.
http://codereview.chromium.org/10913277/diff/6002/content/browser/android/con... File content/browser/android/content_view_core_impl.h (right): http://codereview.chromium.org/10913277/diff/6002/content/browser/android/con... content/browser/android/content_view_core_impl.h:151: virtual void UpdateContentSize(int width, int height) OVERRIDE; if we remove these from content_view_core.h, we can remove the virtual and OVERRIDE bits. http://codereview.chromium.org/10913277/diff/6002/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): http://codereview.chromium.org/10913277/diff/6002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:191: private final PointF mStartHandleNormalizedPoint = new PointF(); I don't think you need these anymore. http://codereview.chromium.org/10913277/diff/6002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1065: // TODO(olilan): add specific method implementations. no need for this indent http://codereview.chromium.org/10913277/diff/6002/content/public/browser/andr... File content/public/browser/android/content_view_core.h (right): http://codereview.chromium.org/10913277/diff/6002/content/public/browser/andr... content/public/browser/android/content_view_core.h:43: virtual void UpdateContentSize(int width, int height) = 0; are these called from outside of the content package? I don't think they are, so we can probably not add these here.
PTAL http://codereview.chromium.org/10913277/diff/6002/content/browser/android/con... File content/browser/android/content_view_core_impl.h (right): http://codereview.chromium.org/10913277/diff/6002/content/browser/android/con... content/browser/android/content_view_core_impl.h:151: virtual void UpdateContentSize(int width, int height) OVERRIDE; On 2012/09/21 00:29:48, Ted C wrote: > if we remove these from content_view_core.h, we can remove the virtual and > OVERRIDE bits. Done. http://codereview.chromium.org/10913277/diff/6002/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): http://codereview.chromium.org/10913277/diff/6002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:191: private final PointF mStartHandleNormalizedPoint = new PointF(); On 2012/09/21 00:29:48, Ted C wrote: > I don't think you need these anymore. Done. http://codereview.chromium.org/10913277/diff/6002/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1065: // TODO(olilan): add specific method implementations. On 2012/09/21 00:29:48, Ted C wrote: > no need for this indent Done. http://codereview.chromium.org/10913277/diff/6002/content/public/browser/andr... File content/public/browser/android/content_view_core.h (right): http://codereview.chromium.org/10913277/diff/6002/content/public/browser/andr... content/public/browser/android/content_view_core.h:43: virtual void UpdateContentSize(int width, int height) = 0; On 2012/09/21 00:29:48, Ted C wrote: > are these called from outside of the content package? I don't think they are, > so we can probably not add these here. Done.
lgtm if ted's happy
On 2012/09/21 17:19:36, Yaron wrote: > lgtm if ted's happy lgtm for dispatchKeyEvent.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cramya@chromium.org/10913277/14001
Change committed as 158348 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
