Upstream Android implementation of WebContentsViewDelegate.

Upstream Android implementation of WebContentsViewDelegate.

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

[shashi, 2012-09-28 20:39:03]

Oli, PTAL, I want to make sure I am not upstreaming something which will be
removed by refactoring.

[nilesh, 2012-09-29 01:04:01]

Some style nits.

[nilesh, 2012-09-29 01:04:07]

https://chromiumcodereview.appspot.com/10993089/diff/2001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/10993089/diff/2001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1351:
int selectionStartY) {
line break before "int selectionStartX "

https://chromiumcodereview.appspot.com/10993089/diff/2001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1354:
selectionStartY - mNativeScrollY);
will this fit on line 1354?

https://chromiumcodereview.appspot.com/10993089/diff/2001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/InsertionHandleController.java
(right):

https://chromiumcodereview.appspot.com/10993089/diff/2001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/InsertionHandleController.java:28:
// TODO(olilan): This is just a stub, upstream implementation for this method.
Fix indentation.

https://chromiumcodereview.appspot.com/10993089/diff/2001/content/public/brow...
File content/public/browser/android/content_view_core.h (right):

https://chromiumcodereview.appspot.com/10993089/diff/2001/content/public/brow...
content/public/browser/android/content_view_core.h:35: virtual void
SelectText(const string16& text,
Move this after GetCurrentRenderPRocessId. Making consistent with
content_view_core_impl.h

[olilan, 2012-10-01 13:01:30]

I know this code comes from downstream master, but the context menu handling
code isn't correct. Looking at the history it's been wrong since the context
menu handling was refactored out of ChromeView. The comment and method names
suggest it's doing selection, but actually it's showing the paste popup (but
isn't doing it under the right conditions).

http://codereview.chromium.org/10993089/diff/9007/chrome/browser/ui/android/t...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

http://codereview.chromium.org/10993089/diff/9007/chrome/browser/ui/android/t...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:33:
// So we instruct the ContentView to select the text directly.
This comment doesn't match what the method does. This implies that this code is
to select text on longpress, but that actually happens in WebKit. What this code
does is apparently show the paste popup, but if so it should be done when the
selected text is empty, not when it is non-empty.

http://codereview.chromium.org/10993089/diff/9007/chrome/browser/ui/android/t...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:34:
if (params.is_editable && !params.selection_text.empty()) {
Should be checking for params.selection_text.empty() with no negation.

http://codereview.chromium.org/10993089/diff/9007/content/browser/android/con...
File content/browser/android/content_view_core_impl.cc (right):

http://codereview.chromium.org/10993089/diff/9007/content/browser/android/con...
content/browser/android/content_view_core_impl.cc:334: void
ContentViewCoreImpl::SelectText(const string16& selected_text,
This method doesn't select any text - ShowPastePopup would be a better name.

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

http://codereview.chromium.org/10993089/diff/9007/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1368:
private void onTextSelected(String selected_text, int selectionStartX, int
selectionStartY) {
We already have onSelectionChanged and onSelectionBoundsChanged as selected
events. Also, this is currently used to show the paste pop up, but that should
only happen when text isn't selected.

[shashi, 2012-10-02 02:30:35]

Thanks a lot, I am cleaning this up, looks like this has been broken for some
time.
On 2012/10/01 13:01:30, olilan wrote:
> I know this code comes from downstream master, but the context menu handling
> code isn't correct. Looking at the history it's been wrong since the context
> menu handling was refactored out of ChromeView. The comment and method names
> suggest it's doing selection, but actually it's showing the paste popup (but
> isn't doing it under the right conditions).
> 
>
http://codereview.chromium.org/10993089/diff/9007/chrome/browser/ui/android/t...
> File
>
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
> (right):
> 
>
http://codereview.chromium.org/10993089/diff/9007/chrome/browser/ui/android/t...
>
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:33:
> // So we instruct the ContentView to select the text directly.
> This comment doesn't match what the method does. This implies that this code
is
> to select text on longpress, but that actually happens in WebKit. What this
code
> does is apparently show the paste popup, but if so it should be done when the
> selected text is empty, not when it is non-empty.
> 
>
http://codereview.chromium.org/10993089/diff/9007/chrome/browser/ui/android/t...
>
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:34:
> if (params.is_editable && !params.selection_text.empty()) {
> Should be checking for params.selection_text.empty() with no negation.
> 
>
http://codereview.chromium.org/10993089/diff/9007/content/browser/android/con...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
http://codereview.chromium.org/10993089/diff/9007/content/browser/android/con...
> content/browser/android/content_view_core_impl.cc:334: void
> ContentViewCoreImpl::SelectText(const string16& selected_text,
> This method doesn't select any text - ShowPastePopup would be a better name.
> 
>
http://codereview.chromium.org/10993089/diff/9007/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
http://codereview.chromium.org/10993089/diff/9007/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1368:
> private void onTextSelected(String selected_text, int selectionStartX, int
> selectionStartY) {
> We already have onSelectionChanged and onSelectionBoundsChanged as selected
> events. Also, this is currently used to show the paste pop up, but that should
> only happen when text isn't selected.

[shashi, 2012-10-10 20:53:25]

Patch after making changes downstream.

[nilesh, 2012-10-10 21:18:45]

Some nits.

[nilesh, 2012-10-10 21:18:51]

https://codereview.chromium.org/10993089/diff/16001/content/browser/android/c...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/10993089/diff/16001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:149: virtual jint
GetCurrentRenderProcessId(JNIEnv* env, jobject obj) OVERRIDE;
Why are you moving this?

https://codereview.chromium.org/10993089/diff/16001/content/public/browser/an...
File content/public/browser/android/content_view_core.h (right):

https://codereview.chromium.org/10993089/diff/16001/content/public/browser/an...
content/public/browser/android/content_view_core.h:35: virtual void
ShowPastePopup(int x, int y) = 0;
Please add a comment now that you understand it fully :)
Also add it at the end, or maintain order in the interface and implementation.

[shashi, 2012-10-11 01:59:57]

https://codereview.chromium.org/10993089/diff/16001/content/browser/android/c...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/10993089/diff/16001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:149: virtual jint
GetCurrentRenderProcessId(JNIEnv* env, jobject obj) OVERRIDE;
I was moving this because it was called by JNI, but this method is also
overridden so original place is correct.
On 2012/10/10 21:18:52, nileshagrawal1 wrote:
> Why are you moving this?

https://codereview.chromium.org/10993089/diff/16001/content/public/browser/an...
File content/public/browser/android/content_view_core.h (right):

https://codereview.chromium.org/10993089/diff/16001/content/public/browser/an...
content/public/browser/android/content_view_core.h:35: virtual void
ShowPastePopup(int x, int y) = 0;
Moved it to the end. 
This method just shows a paste popup, so I think no need for comment, have a
corresponding comment in ShowContextMenu.
On 2012/10/10 21:18:52, nileshagrawal1 wrote:
> Please add a comment now that you understand it fully :)
> Also add it at the end, or maintain order in the interface and implementation.

[shashi, 2012-10-12 19:22:55]

PTAL,
Once I have lgtm from both of you, I will add owners.

[nilesh, 2012-10-12 20:39:56]

LGTM

[shashi, 2012-10-15 21:49:43]

On 2012/10/12 20:39:56, nileshagrawal1 wrote:
> LGTM

Just tested after rebase of husky's
change(https://chromiumcodereview.appspot.com/11068010)
paste now works.

[commit-bot: I haz the power, 2012-10-15 21:50:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/10993089/32001

[commit-bot: I haz the power, 2012-10-15 21:50:26]

Presubmit check for 10993089-32001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/android

Presubmit checks took 1.0s to calculate.

[shashi, 2012-10-15 21:54:36]

+yfriedman for owners approval.

[shashi, 2012-10-15 21:58:07]

On 2012/10/15 21:54:36, shashi wrote:
> +yfriedman for owners approval.
Forgot to mention the directory
yfriedman: chrome/browser/ui/android

[Yaron, 2012-10-15 22:03:23]

On 2012/10/15 21:58:07, shashi wrote:
> On 2012/10/15 21:54:36, shashi wrote:
> > +yfriedman for owners approval.
> Forgot to mention the directory
> yfriedman: chrome/browser/ui/android

lgtm

[commit-bot: I haz the power, 2012-10-15 22:05:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/10993089/32001

[commit-bot: I haz the power, 2012-10-16 01:40:04]

Change committed as 162031