[Android] Fix 'open in browser' in CCT does not work for images in Google image search

[Android] Fix 'open in browser' in CCT does not work for images in Google image search

'Open in browser', 'Open in new Chrome tab' and 'Open in incognito tab'
in CCT don't work for images in Google image search. This is because
these images are both images and anchor. Their sourceUrls are data://
scheme which cannot be handled for Chrome for a View intent.

To fix the problem, since those images are also anchor and their linkUrl is
a valid url for View Intent in Chrome, thus sending linkUrl instead.
This CL also redesigns the logic for deciding the valid url of a
ContextMenuParams, that if the param is an anchor and its linkUrl is not
null, returns the linkUrl, otherwise returns the srcUrl.

BUG=720532
Review-Url: https://codereview.chromium.org/2891583004
Cr-Commit-Position: refs/heads/master@{#473287}
Committed: https://chromium.googlesource.com/chromium/src/+/9c3c202169fe21903913e25f2a44f3353b72d4d6

[ltian, 2017-05-18 01:40:19]

ltian@chromium.org changed reviewers:
+ tedchoc@chromium.org

[ltian, 2017-05-18 01:40:19]

Can you also take a look of the changes in this CL? 

This CL should fix the bug but I am concerned that if an image is not an anchor
and this srcUrl is something like "data:image/jpeg...", current code will not
handle it. For this case, should I add a check in this populator to prevent it
from showing the menu items or should I allow this scheme to be handled by
Chrome through a VIEW intent?

I also check our Manifest file but cannot find anywhere allows
android:scheme="data", so I am also curious how does inside Chrome handle this
kind of url?

Thanks!

[Ted C, 2017-05-18 02:21:33]

https://codereview.chromium.org/2891583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/2891583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:625:
if (params.isAnchor() && params.getLinkUrl() != null) {
Should we use isEmptyUrl(params.getLinkUrl()) instead of != null?

[ltian, 2017-05-18 18:00:02]

https://codereview.chromium.org/2891583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/2891583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:625:
if (params.isAnchor() && params.getLinkUrl() != null) {
On 2017/05/18 02:21:33, Ted C wrote:
> Should we use isEmptyUrl(params.getLinkUrl()) instead of != null?

Done.

[Ted C, 2017-05-19 16:55:08]

https://codereview.chromium.org/2891583004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/2891583004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:625:
if (params.isAnchor() && !isEmptyUrl(params.getLinkUrl())) {
per offline discussion, let's remove the isEmptyUrl check at all.

[ltian, 2017-05-19 18:02:04]

https://codereview.chromium.org/2891583004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/2891583004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:625:
if (params.isAnchor() && !isEmptyUrl(params.getLinkUrl())) {
On 2017/05/19 16:55:07, Ted C wrote:
> per offline discussion, let's remove the isEmptyUrl check at all.

Done.

[Ted C, 2017-05-19 18:03:41]

lgtm, did you want to make the change to send to ChromeLauncherActivity as a
class instead of package in the helper as well?

[ltian, 2017-05-19 18:16:02]

On 2017/05/19 18:03:41, Ted C wrote:
> lgtm, did you want to make the change to send to ChromeLauncherActivity as a
> class instead of package in the helper as well?

Oh, yes! Sorry I forgot that.

[Ted C, 2017-05-19 18:42:36]

On 2017/05/19 18:17:48, ltian wrote:

still lgtm

[ltian, 2017-05-19 18:43:49]

The CQ bit was checked by ltian@chromium.org

[commit-bot: I haz the power, 2017-05-19 18:44:34]

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-05-19 19:47:53]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495219429233620,
"parent_rev": "462b7cae8824b4eb54e028b42da50f25ab4b0615", "commit_rev":
"9c3c202169fe21903913e25f2a44f3353b72d4d6"}

[commit-bot: I haz the power, 2017-05-19 19:48:08]

Description was changed from

==========
[Android] Fix 'open in browser' in CCT does not work for images in Google image
search

'Open in browser', 'Open in new Chrome tab' and 'Open in incognito tab'
in CCT don't work for images in Google image search. This is because
these images are both images and anchor. Their sourceUrls are data://
scheme which cannot be handled for Chrome for a View intent.

To fix the problem, since those images are also anchor and their linkUrl is
a valid url for View Intent in Chrome, thus sending linkUrl instead.
This CL also redesigns the logic for deciding the valid url of a
ContextMenuParams, that if the param is an anchor and its linkUrl is not
null, returns the linkUrl, otherwise returns the srcUrl.

BUG=720532
==========

to

==========
[Android] Fix 'open in browser' in CCT does not work for images in Google image
search

'Open in browser', 'Open in new Chrome tab' and 'Open in incognito tab'
in CCT don't work for images in Google image search. This is because
these images are both images and anchor. Their sourceUrls are data://
scheme which cannot be handled for Chrome for a View intent.

To fix the problem, since those images are also anchor and their linkUrl is
a valid url for View Intent in Chrome, thus sending linkUrl instead.
This CL also redesigns the logic for deciding the valid url of a
ContextMenuParams, that if the param is an anchor and its linkUrl is not
null, returns the linkUrl, otherwise returns the srcUrl.

BUG=720532

Review-Url: https://codereview.chromium.org/2891583004
Cr-Commit-Position: refs/heads/master@{#473287}
Committed:
https://chromium.googlesource.com/chromium/src/+/9c3c202169fe21903913e25f2a44...
==========

[commit-bot: I haz the power, 2017-05-19 19:48:09]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9c3c202169fe21903913e25f2a44...