Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
583583005:
[Contextual Search] Pass selection coordinate to ContentViewClient. (Closed)
|
Created:
6 years, 3 months ago by pedro (no code reviews) Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Contextual Search] Pass selection coordinate to ContentViewClient.
BUG=415656
Committed: https://crrev.com/f5164b9f8b4352fb326260ae2f901b17a000fd51
Cr-Commit-Position: refs/heads/master@{#295802}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding TODO #
Total comments: 2
Patch Set 3 : Addressing Donn's comment #
Total comments: 2
Patch Set 4 : Addressing Jared's comment #Patch Set 5 : Moving a TODO as per Ted's suggestion #
Messages
Total messages: 24 (8 generated)
pedrosimonetti@chromium.org changed reviewers: + jdduke@chromium.org
Hey Jared, please take a look at this change.
https://codereview.chromium.org/583583005/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/583583005/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:134: * TODO(donnd): Remove this and instead expose a ContextualSearchClient, crbug.com/403001. Please add a TODO here to remove this method once downstream code has been updated to use the other signature.
Jared, please take another look. https://codereview.chromium.org/583583005/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/583583005/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:134: * TODO(donnd): Remove this and instead expose a ContextualSearchClient, crbug.com/403001. On 2014/09/18 18:08:58, jdduke wrote: > Please add a TODO here to remove this method once downstream code has been > updated to use the other signature. Done.
donnd@google.com changed reviewers: + donnd@google.com
lgtm https://codereview.chromium.org/583583005/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/583583005/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:136: * the other signature Nit: end with a period.
PTAL https://codereview.chromium.org/583583005/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/583583005/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:136: * the other signature On 2014/09/18 18:46:29, Donn wrote: > Nit: end with a period. Done.
https://codereview.chromium.org/583583005/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/583583005/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:148: public void onSelectionEvent(int eventType, float posXDip, float posYDip) { Hmm, other coordinates in this class are in pixels, we should probably do the conversion to pixels before forwarding.
PTAL https://codereview.chromium.org/583583005/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/583583005/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:148: public void onSelectionEvent(int eventType, float posXDip, float posYDip) { On 2014/09/18 19:12:07, jdduke wrote: > Hmm, other coordinates in this class are in pixels, we should probably do the > conversion to pixels before forwarding. Done.
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
lgtm
The CQ bit was checked by pedrosimonetti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/583583005/80001
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/583583005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by pedrosimonetti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/583583005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 35dc83f4c4bb2427cfc59da52e5d59e1caf54a9d
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f5164b9f8b4352fb326260ae2f901b17a000fd51 Cr-Commit-Position: refs/heads/master@{#295802} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
