Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2891583004: [Android] Fix 'open in browser' in CCT does not work for images in Google image search (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by ltian
Modified:
1 month, 1 week ago
Reviewers:
Ted C
CC:
chromium-reviews, agrieve+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 2

Patch Set 2 : Update based on Ted's comment. #

Total comments: 2

Patch Set 3 : Update based on Ted's comment and rebase. #

Patch Set 4 : Set Intent directly sent to ChromeLaucherActivity. #

Messages

Total messages: 15 (4 generated)
ltian
Can you also take a look of the changes in this CL? This CL should ...
1 month, 1 week ago (2017-05-18 01:40:19 UTC) #2
Ted C
https://codereview.chromium.org/2891583004/diff/1/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java 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/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java#newcode625 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:625: if (params.isAnchor() && params.getLinkUrl() != null) { Should we ...
1 month, 1 week ago (2017-05-18 02:21:33 UTC) #3
ltian
https://codereview.chromium.org/2891583004/diff/1/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java 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/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java#newcode625 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:625: if (params.isAnchor() && params.getLinkUrl() != null) { On 2017/05/18 ...
1 month, 1 week ago (2017-05-18 18:00:02 UTC) #4
Ted C
https://codereview.chromium.org/2891583004/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2891583004/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java#newcode625 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:625: if (params.isAnchor() && !isEmptyUrl(params.getLinkUrl())) { per offline discussion, let's ...
1 month, 1 week ago (2017-05-19 16:55:08 UTC) #5
ltian
https://codereview.chromium.org/2891583004/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2891583004/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java#newcode625 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 ...
1 month, 1 week ago (2017-05-19 18:02:04 UTC) #6
Ted C
lgtm, did you want to make the change to send to ChromeLauncherActivity as a class ...
1 month, 1 week ago (2017-05-19 18:03:41 UTC) #7
ltian
On 2017/05/19 18:03:41, Ted C wrote: > lgtm, did you want to make the change ...
1 month, 1 week ago (2017-05-19 18:16:02 UTC) #8
ltian
1 month, 1 week ago (2017-05-19 18:17:48 UTC) #9
Ted C
On 2017/05/19 18:17:48, ltian wrote: still lgtm
1 month, 1 week ago (2017-05-19 18:42:36 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2891583004/60001
1 month, 1 week ago (2017-05-19 18:44:34 UTC) #12
commit-bot: I haz the power
1 month, 1 week ago (2017-05-19 19:48:09 UTC) #15
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9c3c202169fe21903913e25f2a44...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318