Add a view to show magnified link preview on Andrdoid.

Add a view to show magnified link preview on Andrdoid.

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

[nilesh, 2012-08-21 23:03:41]

Adding Yaron (for android OWNERS)
Adding brettw (for content/content_tests.gypi)

[Yaron, 2012-08-22 00:56:09]

as mentioend offline, please add usage from ContentViewCore

[nilesh, 2012-08-22 22:12:52]

I have added code to init PopupZoomer in ContentVIewCore init.
There is more code in ContentViewCore to show the zoomer but I will put it in
another CL. PTAL.

[Yaron, 2012-08-22 23:30:02]

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:129:
// Copied from android.util.MathUtils.
I'd remove this whole comment as it's a pretty trivial routine and doesn't look
to be available.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:143:
// here after fixing crbug.com/136704
Can you fulfill this TODO?

[brettw, 2012-08-22 23:32:14]

owners lgtm rubberstam, I didn't read any code.

[nilesh, 2012-08-23 00:11:37]

PTAL.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:129:
// Copied from android.util.MathUtils.
On 2012/08/22 23:30:02, Yaron wrote:
> I'd remove this whole comment as it's a pretty trivial routine and doesn't
look
> to be available.

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:143:
// here after fixing crbug.com/136704
On 2012/08/22 23:30:02, Yaron wrote:
> Can you fulfill this TODO?

Modified the constructor to accept a resource id specifying the overlay radius.

[Yaron, 2012-08-23 00:16:36]

lgtm

[Ted C, 2012-08-23 00:19:08]

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:312:
nativeSingleTap(mNativeContentViewCore, e.getEventTime(), (int)e.getX(),
space after the casts (same for below)

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:32:
* It handles manipulation of the canvas and touch events to display
This comment looks like it's capped at 80 chars and we allow 100, maybe it can
be collapsed a bit?  Same for other comments in the file.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:44:
public static interface OnTapListener {
simple javadoc for this.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:47:
}
blank line after the definition.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:210:
public void setBitmap(Bitmap b) {
all public methods should have javadoc

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:218:
Canvas c = new Canvas(mZoomedBitmap);
s/c/canvas

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:219:
Path p = new Path();
s/p/path

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:222:
sOverlayCornerRadius,
java indenting of 8 chars instead of c++, although it looks like it might all
fit on one line?  If not, I would put all params on following line.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:232:
private boolean scroll(float x, float y) {
no one uses this return value so it can be removed.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:368:
((float)ANIMATION_DURATION);
space after the cast

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:392:
canvas.drawARGB((int)(80 * fractionAnimation), 0, 0, 0);
space between (int) and (

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
File
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
(right):

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java:62:
PopupZoomer popup = new CustomCanvasPopupZoomer(
you can just return here without keeping a reference to popup

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java:133:
((CustomCanvasPopupZoomer)mPopupZoomer).finishPendingDraws();
space after cast (a couple places below as well)

actually, I would probably just change the type of mPopupZoomer to be
CustomCanvasPopupZoomer to avoid this here and below

[nilesh, 2012-08-23 17:04:30]

Ted, Please take another look.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:312:
nativeSingleTap(mNativeContentViewCore, e.getEventTime(), (int)e.getX(),
On 2012/08/23 00:19:08, Ted C wrote:
> space after the casts (same for below)

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:32:
* It handles manipulation of the canvas and touch events to display
On 2012/08/23 00:19:08, Ted C wrote:
> This comment looks like it's capped at 80 chars and we allow 100, maybe it can
> be collapsed a bit?  Same for other comments in the file.

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:44:
public static interface OnTapListener {
On 2012/08/23 00:19:08, Ted C wrote:
> simple javadoc for this.

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:47:
}
On 2012/08/23 00:19:08, Ted C wrote:
> blank line after the definition.

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:210:
public void setBitmap(Bitmap b) {
On 2012/08/23 00:19:08, Ted C wrote:
> all public methods should have javadoc

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:218:
Canvas c = new Canvas(mZoomedBitmap);
On 2012/08/23 00:19:08, Ted C wrote:
> s/c/canvas

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:219:
Path p = new Path();
On 2012/08/23 00:19:08, Ted C wrote:
> s/p/path

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:222:
sOverlayCornerRadius,
On 2012/08/23 00:19:08, Ted C wrote:
> java indenting of 8 chars instead of c++, although it looks like it might all
> fit on one line?  If not, I would put all params on following line.

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:232:
private boolean scroll(float x, float y) {
On 2012/08/23 00:19:08, Ted C wrote:
> no one uses this return value so it can be removed.

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:368:
((float)ANIMATION_DURATION);
On 2012/08/23 00:19:08, Ted C wrote:
> space after the cast

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:392:
canvas.drawARGB((int)(80 * fractionAnimation), 0, 0, 0);
On 2012/08/23 00:19:08, Ted C wrote:
> space between (int) and (

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
File
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
(right):

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java:62:
PopupZoomer popup = new CustomCanvasPopupZoomer(
On 2012/08/23 00:19:08, Ted C wrote:
> you can just return here without keeping a reference to popup

Done.

http://codereview.chromium.org/10828427/diff/4001/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java:133:
((CustomCanvasPopupZoomer)mPopupZoomer).finishPendingDraws();
On 2012/08/23 00:19:08, Ted C wrote:
> space after cast (a couple places below as well)
> 
> actually, I would probably just change the type of mPopupZoomer to be
> CustomCanvasPopupZoomer to avoid this here and below

Done.

[Ted C, 2012-08-23 17:18:42]

lgtm w/ tiny nit :-)

http://codereview.chromium.org/10828427/diff/11003/content/public/android/jav...
File
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
(right):

http://codereview.chromium.org/10828427/diff/11003/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java:62:
return  new CustomCanvasPopupZoomer(
looks like there are two spaces after return

[nilesh, 2012-08-23 17:25:32]

On 2012/08/23 17:18:42, Ted C wrote:
> lgtm w/ tiny nit :-)

What version of lint do you have installed in your eyes? I need to upgrade :)

Thanks for the review.

[nilesh, 2012-08-23 17:26:26]

http://codereview.chromium.org/10828427/diff/11003/content/public/android/jav...
File
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
(right):

http://codereview.chromium.org/10828427/diff/11003/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java:62:
return  new CustomCanvasPopupZoomer(
On 2012/08/23 17:18:43, Ted C wrote:
> looks like there are two spaces after return

Fixed.

[commit-bot: I haz the power, 2012-08-23 17:45:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/10828427/8003

[commit-bot: I haz the power, 2012-08-23 22:23:35]

Change committed as 153084