Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1354763003:
[Contextual Search] Trigger the translation one-box. (Closed)
|
Created:
5 years, 3 months ago by Donn Denman Modified:
5 years, 1 month ago CC:
chromium-reviews, Theresa, joshuacooper_google.com, shmichael_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Contextual Search] Trigger the translation one-box.
Adds support for triggering a Translate one-box on the SERP in response to a tap gesture.
* Adds client recognition of the "lang" parameter returned in
the Search Term Resolution response.
* Updates unit tests to check for language.
* Adds a method to get the user's "target" languages.
* Adds a check to see if the "source" and "target" languages
are different enough to require translation.
* Adds parameters to the Search request when translation
is needed to trigger the Translate one-box. These
parameters still need to be revised to match an evolving
server-side API.
* Add a utility to UiUtils that produces a set of locales
known by the user by inspecting the Android input methods.
* Put this functionality behind a Finch flag.
BUG=413717
Committed: https://crrev.com/9959a95fc4fedd724decdd1a5b938803c4990a7d
Cr-Commit-Position: refs/heads/master@{#358101}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Added a method to get all locales by checking IME. Put translation behind a flag. #
Total comments: 9
Patch Set 3 : Updated UiUtils to address Ted's comments. #
Total comments: 4
Patch Set 4 : Addressed Teds comments: Changed to use LinkedHashSet to ensure unique and ordered results. #
Total comments: 2
Patch Set 5 : Return a Set instead of a List from the UiUtil. #Patch Set 6 : Added languages from accept-headers. #Patch Set 7 : Disable for non-English users, for now. #
Total comments: 26
Patch Set 8 : Addressed Pedros recent comments, including changing the caching of the target language. #
Total comments: 4
Patch Set 9 : Caching getTargetLanguages, as suggested by Pedro in his last review. #Patch Set 10 : Fix unit tests. #
Total comments: 2
Patch Set 11 : Rebase #Patch Set 12 : Rebase, and remove ContextualSearchPanelDelegate.java (already removed from repo). #Patch Set 13 : Add flag settings to new instrumentation tests. #Patch Set 14 : Rebased with Pedro's new test infrastructure, updated these instrumentation tests to use that. #
Total comments: 6
Patch Set 15 : Updated comment about test infrastructure needing to be updated when shifting to new API. #Messages
Total messages: 55 (21 generated)
donnd@chromium.org changed reviewers: + pedrosimonetti@chromium.org
Pedro, this isn't finished yet because we still have some issues with the Translate one-box triggering params. But I think it's far enough along that a review would be very useful. PTAL.
Hey Donn, sorry for the delay. Here are my first round of comments. Overall, things look okay. The only thing that I'd like to discuss with you is the concept of a "target language". I think we need a list of languages, and not a single one, otherwise we'll make the experience worse for multi-language users. For example, my phone is in english but I would never want to have translations for portuguese. In this case, it would make more sense showing a dictionary one-box in portuguese. https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:683: int selectionStartAdjust, int selectionEndAdjust, String language) { I would specify which language this is referring to. Is this the context language? Search term language? Also, if this is the search term language, shouldn't this parameter come right after the searchTerm parameter? https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:715: boolean needsTranslation = false; Aren't translations supposed to work with long press as well? https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:332: return !TextUtils.equals(source.substring(0, 2), target.substring(0, 2)); I think we should also take into consideration other factors, like languages that the user has set up on their phone or Android keyboard (you can choose multiple languages). https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java (right): https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:33: private static final String GWS_SEARCH_NO_SUGGESTIONS_PARAM = "s"; Isn't this supposed to be "sns"? https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:63: boolean isLowPriorityEnabled, boolean needsTranslation, @Nullable String sourceLanguage, Nit: Instead of passing the extra parameters here in the constructor, I think it would be clearer passing them through a new method, like forceTranslationResult(String sourceLanguage, String targetLanguage), since parameters in the constructor are usually the ones that are always used/needed for the purpose of the class. In this case, translation is not something that will always happen. https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:67: searchTerm, alternateTerm, false, needsTranslation, sourceLanguage, targetLanguage); Nit: Also, I think we should store the parameters in class properties and use them later in the private methods. This would prevent having to pass them over and over, but also, it feels right. It's kinda strange that this class doesn't remember what the search term was, for example. https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:239: System.out.println("ctxs Uri: " + result); Remove println() call https://codereview.chromium.org/1354763003/diff/1/chrome/browser/android/cont... File chrome/browser/android/contextualsearch/contextual_search_delegate.cc (right): https://codereview.chromium.org/1354763003/diff/1/chrome/browser/android/cont... chrome/browser/android/contextualsearch/contextual_search_delegate.cc:443: return target_language_; We should probably get a list of languages, and not just a single one. I would hate loosing Contextual Search just because I'm reading portuguese pages and Google thinks that I need to translate words, instead of getting actual results. That would make Contextual Search much less useful in portuguese for me (specially if we use the "translate" string as a trigger). https://codereview.chromium.org/1354763003/diff/1/chrome/browser/android/cont... File chrome/browser/android/contextualsearch/contextual_search_manager.cc (right): https://codereview.chromium.org/1354763003/diff/1/chrome/browser/android/cont... chrome/browser/android/contextualsearch/contextual_search_manager.cc:102: const ResolvedSearchTerm& resolved_search_term) { I don't see the point why we are storing this in a struct if we will end up having to split it up right after that, because we need to pass individual values to the java method. Is there a reason why we are storing it in a struct? https://codereview.chromium.org/1354763003/diff/1/chrome/browser/android/cont... File chrome/browser/android/contextualsearch/contextual_search_manager.h (right): https://codereview.chromium.org/1354763003/diff/1/chrome/browser/android/cont... chrome/browser/android/contextualsearch/contextual_search_manager.h:50: // TODO(donnd): Can we get rid of the jobject if it's not being used? The jobj is being implicitly used when you pass the pointer from java, so you need it in there.
donnd@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, PTAL just at UiUtils. Pedro, thanks for the review! PTAL. https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:683: int selectionStartAdjust, int selectionEndAdjust, String language) { On 2015/09/23 18:33:54, pedrosimonetti wrote: > I would specify which language this is referring to. Is this the context > language? Search term language? Also, if this is the search term language, > shouldn't this parameter come right after the searchTerm parameter? Done. https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:715: boolean needsTranslation = false; On 2015/09/23 18:33:54, pedrosimonetti wrote: > Aren't translations supposed to work with long press as well? Great question! Since we don't call the server for Search Term Resolution on long-press we have no language detection... but we could do some local language detection, since this is probably a strong use-case. Unfortunately all my attempts to call the local CDL have failed. So I've added a TODO for that! https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:332: return !TextUtils.equals(source.substring(0, 2), target.substring(0, 2)); On 2015/09/23 18:33:54, pedrosimonetti wrote: > I think we should also take into consideration other factors, like languages > that the user has set up on their phone or Android keyboard (you can choose > multiple languages). Done. https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:33: private static final String GWS_SEARCH_NO_SUGGESTIONS_PARAM = "s"; On 2015/09/23 18:33:54, pedrosimonetti wrote: > Isn't this supposed to be "sns"? Thanks for the catch! Done. https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:63: boolean isLowPriorityEnabled, boolean needsTranslation, @Nullable String sourceLanguage, On 2015/09/23 18:33:54, pedrosimonetti wrote: > Nit: Instead of passing the extra parameters here in the constructor, I think it > would be clearer passing them through a new method, like > forceTranslationResult(String sourceLanguage, String targetLanguage), since > parameters in the constructor are usually the ones that are always used/needed > for the purpose of the class. In this case, translation is not something that > will always happen. Done. https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:67: searchTerm, alternateTerm, false, needsTranslation, sourceLanguage, targetLanguage); On 2015/09/23 18:33:54, pedrosimonetti wrote: > Nit: Also, I think we should store the parameters in class properties and use > them later in the private methods. This would prevent having to pass them over > and over, but also, it feels right. It's kinda strange that this class doesn't > remember what the search term was, for example. You mean remember the parameters instead of modifying the existing URI, or building multiple URIs, one for low and one for normal priority? https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:239: System.out.println("ctxs Uri: " + result); On 2015/09/23 18:33:55, pedrosimonetti wrote: > Remove println() call Done. https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/browser/andro... File chrome/browser/android/contextualsearch/contextual_search_delegate.cc (right): https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate.cc:443: return target_language_; On 2015/09/23 18:33:55, pedrosimonetti wrote: > We should probably get a list of languages, and not just a single one. I would > hate loosing Contextual Search just because I'm reading portuguese pages and > Google thinks that I need to translate words, instead of getting actual results. > That would make Contextual Search much less useful in portuguese for me > (specially if we use the "translate" string as a trigger). Done. https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/browser/andro... File chrome/browser/android/contextualsearch/contextual_search_manager.cc (right): https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_manager.cc:102: const ResolvedSearchTerm& resolved_search_term) { On 2015/09/23 18:33:55, pedrosimonetti wrote: > I don't see the point why we are storing this in a struct if we will end up > having to split it up right after that, because we need to pass individual > values to the java method. Is there a reason why we are storing it in a struct? I think it makes this code and the tests more readable. I agree that the win is small, but it still moves some of the complication out of the bigger files that are already complicated. https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/browser/andro... File chrome/browser/android/contextualsearch/contextual_search_manager.h (right): https://chromiumcodereview.appspot.com/1354763003/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_manager.h:50: // TODO(donnd): Can we get rid of the jobject if it's not being used? On 2015/09/23 18:33:55, pedrosimonetti wrote: > The jobj is being implicitly used when you pass the pointer from java, so you > need it in there. Acknowledged.
https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:149: List<String> result = new ArrayList<String>(); I'd recommend returning a Set and using LinkedHashSet() then you at least ensure there are no duplicates https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:153: for (InputMethodInfo inputMethodInfo : enabledMethods) { Use for (int i = 0; i < ...) for iterating to avoid the allocation of the iterator. https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:155: imManager.getEnabledInputMethodSubtypeList(inputMethodInfo, true); this can't be null right? the docs don't say, but might want to poke around a bit. https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:158: if (!locale.isEmpty()) result.add(locale); TextUtils.isEmpty
Ted, thanks for the review! Soon I'll remember TextUtils.isEmpty. PTAL. https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:149: List<String> result = new ArrayList<String>(); On 2015/10/09 22:50:30, Ted C wrote: > I'd recommend returning a Set and using LinkedHashSet() then you at least ensure > there are no duplicates Actually I did that, but then switched back to using a list because I think these items are ordered, and that the order could be important, which is why getEnabledInputMethodList returns a list. :-) https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:153: for (InputMethodInfo inputMethodInfo : enabledMethods) { On 2015/10/09 22:50:30, Ted C wrote: > Use for (int i = 0; i < ...) for iterating to avoid the allocation of the > iterator. Done. https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:155: imManager.getEnabledInputMethodSubtypeList(inputMethodInfo, true); On 2015/10/09 22:50:30, Ted C wrote: > this can't be null right? the docs don't say, but might want to poke around a > bit. I wondered about all of these requests returning null, and concluded that none of them should (though that was just poking around). Double-checking this one I see it *can* be null when there's no IME, so added a check for that. https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:158: if (!locale.isEmpty()) result.add(locale); On 2015/10/09 22:50:30, Ted C wrote: > TextUtils.isEmpty Done.
lgtm https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/20001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:149: List<String> result = new ArrayList<String>(); On 2015/10/09 23:51:38, Donn Denman wrote: > On 2015/10/09 22:50:30, Ted C wrote: > > I'd recommend returning a Set and using LinkedHashSet() then you at least > ensure > > there are no duplicates > > Actually I did that, but then switched back to using a list because I think > these items are ordered, and that the order could be important, which is why > getEnabledInputMethodList returns a list. :-) Agreed, that's what I was suggesting LinkedHashMap. The iteration order of that is based on insertion order, so it would match the language list from the input methods but without dups. If it can contain dupes, then it would be best to document that in the javadoc. https://chromiumcodereview.appspot.com/1354763003/diff/40001/ui/android/java/... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/40001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:157: if (subtypes != null) { I would do: if (subtypes == null) continue; Then you can reduce the indent of the following. https://chromiumcodereview.appspot.com/1354763003/diff/40001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:164: System.out.println("ctxs getIMELocales returning " + result.toString()); debug print out left in
Ted, how does this look now? Thanks for educating me on the Linked Hash collections! https://chromiumcodereview.appspot.com/1354763003/diff/40001/ui/android/java/... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/40001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:157: if (subtypes != null) { On 2015/10/10 00:05:47, Ted C wrote: > I would do: > > if (subtypes == null) continue; > > Then you can reduce the indent of the following. Done. https://chromiumcodereview.appspot.com/1354763003/diff/40001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:164: System.out.println("ctxs getIMELocales returning " + result.toString()); On 2015/10/10 00:05:47, Ted C wrote: > debug print out left in Done.
lgtm https://chromiumcodereview.appspot.com/1354763003/diff/60001/ui/android/java/... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/60001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:164: return new ArrayList<String>(locales); I'd just return the Set here myself, but if the list makes the rest of the CL easier than that is fine.
Pedro, PTAL. Thanks Ted! https://chromiumcodereview.appspot.com/1354763003/diff/60001/ui/android/java/... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/60001/ui/android/java/... ui/android/java/src/org/chromium/ui/UiUtils.java:164: return new ArrayList<String>(locales); On 2015/10/13 01:15:13, Ted C wrote: > I'd just return the Set here myself, but if the list makes the rest of the CL > easier than that is fine. Done.
Description was changed from ========== [Contextual Search] Add language support for translation. Adds support for triggering a Translate one-box on the SERP. * Adds client recognition of the "lang" parameter returned in the Search Term Resolution response. * Updates unit tests to check for language. * Adds a method to get the user's "target" languages. * Adds a check to see if the "source" and "target" languages are different enough to require translation. * Adds parameters to the Search request when translation is needed to trigger the Translate one-box. These parameters still need to be revised to match an evolving server-side API. * Add a utility to UiUtils that produces a set of locales known by the user by inspecting the Android input methods. * Put this functionality behind a Finch flag. BUG=413717 ========== to ========== [Contextual Search] Trigger the translation one-box. Adds support for triggering a Translate one-box on the SERP in response to a tap gesture. * Adds client recognition of the "lang" parameter returned in the Search Term Resolution response. * Updates unit tests to check for language. * Adds a method to get the user's "target" languages. * Adds a check to see if the "source" and "target" languages are different enough to require translation. * Adds parameters to the Search request when translation is needed to trigger the Translate one-box. These parameters still need to be revised to match an evolving server-side API. * Add a utility to UiUtils that produces a set of locales known by the user by inspecting the Android input methods. * Put this functionality behind a Finch flag. BUG=413717 ==========
donnd@chromium.org changed reviewers: - tedchoc@chromium.org
Pedro, PTAL. I think this is in good shape to land for English users since it's behind a flag. Removing Ted since he approved the utils.
https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:715: boolean needsTranslation = false; On 2015/10/09 22:08:23, Donn Denman wrote: > On 2015/09/23 18:33:54, pedrosimonetti wrote: > > Aren't translations supposed to work with long press as well? > > Great question! Since we don't call the server for Search Term Resolution on > long-press we have no language detection... but we could do some local language > detection, since this is probably a strong use-case. Unfortunately all my > attempts to call the local CDL have failed. So I've added a TODO for that! Acknowledged. https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java (right): https://codereview.chromium.org/1354763003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:67: searchTerm, alternateTerm, false, needsTranslation, sourceLanguage, targetLanguage); On 2015/10/09 22:08:23, Donn Denman wrote: > On 2015/09/23 18:33:54, pedrosimonetti wrote: > > Nit: Also, I think we should store the parameters in class properties and use > > them later in the private methods. This would prevent having to pass them over > > and over, but also, it feels right. It's kinda strange that this class doesn't > > remember what the search term was, for example. > > You mean remember the parameters instead of modifying the existing URI, or > building multiple URIs, one for low and one for normal priority? I meant that it feels weird that this class doesn't remember what the search term was, given this class should be the authority about the search request. But it's OK, is just something to keep in mind. Maybe we can improve this in our refactoring. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:923: public Context getContext() { We might not need this (see comment in ContextualSearchManager). Also, it's in the wrong section, since this is not related to the Content. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java (right): https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:141: Context getContext(); FYI: The delegate has been removed. Users now access the Panel directly via public methods. You should sync and rebase your patch. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:750: List<String> targetLanguages = getTargetLanguages(); We should cache the targetLanguages to prevent calling getTargetLanguages() every time. I wouldn't cache it here either, since this method might be called during animations. Maybe cache during the Manager's initialize() method? We should also check mPolicy.isTranslationEnabled() and only cache it when necessary. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:811: private List<String> getTargetLanguages() { This method makes 2 JNI calls so we should cache its return value to avoid making those calls every time. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:820: Context context = mSearchPanelDelegate.getContext(); Can't we use mWindowAndroid.getApplicationContext() instead? https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:853: String trimmedLocale = locale.substring(0, 2); Do we really need to trim it? I think Locale.forLanguageTag(locale).getLanguage() should return just the language part, excluding the country/region part. Locale.forLanguageTag() expects the code with "-", not "_". If the locale string uses underscore, you would have to replace them. Still, I think it's better because it doesn't assume the size of the language part of the code (here assumed to be 2). http://docs.oracle.com/javase/7/docs/api/java/util/Locale.html#def_language https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:381: // For now, we just return the first language, unless it's English. Nit: Put a TODO here to improve this logic. If this language doesn't match your server preferences, you might see a page in one language and the one box translation in another, which might be confusing. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:382: if (targetLanguages.get(0) == "en" && targetLanguages.size() > 1) { Nit: Add constant for string. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java (right): https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:159: There should be a public boolean isTranlationForced() method that tests can use to assert it is in translation mode. See comment in the ContextualSearchManagerTest. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1354763003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2004: assertLoadedLowPriorityUrl(); Nit: We shouldn't care about the priority in this test case, right? https://codereview.chromium.org/1354763003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2006: mFakeServer.getLoadedUrl().contains(ContextualSearchRequest.TLITE_SOURCE_LANGUAGE_PARAM); We shouldn't know how the Request triggers the translation. Instead, the Request itself should tell if it's using the translation mode.
Pedro PTAL. Thanks for the review! https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:923: public Context getContext() { On 2015/10/23 08:50:36, pedrosimonetti wrote: > We might not need this (see comment in ContextualSearchManager). Also, it's in > the wrong section, since this is not related to the Content. Removed -- thanks for the pointer for getApplicationContext, somehow missed that. https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:141: Context getContext(); On 2015/10/23 08:50:36, pedrosimonetti wrote: > FYI: The delegate has been removed. Users now access the Panel directly via > public methods. You should sync and rebase your patch. Removed this too. https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:750: List<String> targetLanguages = getTargetLanguages(); On 2015/10/23 08:50:36, pedrosimonetti wrote: > We should cache the targetLanguages to prevent calling getTargetLanguages() > every time. I wouldn't cache it here either, since this method might be called > during animations. Maybe cache during the Manager's initialize() method? I think we should call getTargetLanguages every time, since the user can respond to an infobar at any time that says "don't ever translate" some language and that's reflected in the accept-headers and the target languages. > We should also check mPolicy.isTranslationEnabled() and only cache it when > necessary. Already done -- all isTranslationEnabled does is check the fieldtrial, which caches. https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:811: private List<String> getTargetLanguages() { On 2015/10/23 08:50:36, pedrosimonetti wrote: > This method makes 2 JNI calls so we should cache its return value to avoid > making those calls every time. I think caching the target language is fine since it seldom changes. So I added a method here to cache that, and removed the caching from the native code called. However, As I said above, I don't think we should cache the accept-languages, since the user can easily change this by responding to an infobar to never translate <x>. https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:820: Context context = mSearchPanelDelegate.getContext(); On 2015/10/23 08:50:36, pedrosimonetti wrote: > Can't we use mWindowAndroid.getApplicationContext() instead? Done. Thanks! https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:381: // For now, we just return the first language, unless it's English. On 2015/10/23 08:50:36, pedrosimonetti wrote: > Nit: Put a TODO here to improve this logic. If this language doesn't match your > server preferences, you might see a page in one language and the one box > translation in another, which might be confusing. Done. https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:382: if (targetLanguages.get(0) == "en" && targetLanguages.size() > 1) { On 2015/10/23 08:50:36, pedrosimonetti wrote: > Nit: Add constant for string. Done. https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRequest.java:159: On 2015/10/23 08:50:36, pedrosimonetti wrote: > There should be a public boolean isTranlationForced() method that tests can use > to assert it is in translation mode. See comment in the > ContextualSearchManagerTest. Done. https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2004: assertLoadedLowPriorityUrl(); On 2015/10/23 08:50:36, pedrosimonetti wrote: > Nit: We shouldn't care about the priority in this test case, right? Done. https://chromiumcodereview.appspot.com/1354763003/diff/120001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2006: mFakeServer.getLoadedUrl().contains(ContextualSearchRequest.TLITE_SOURCE_LANGUAGE_PARAM); On 2015/10/23 08:50:36, pedrosimonetti wrote: > We shouldn't know how the Request triggers the translation. Instead, the Request > itself should tell if it's using the translation mode. Good point! Done.
lgtm I still think caching the result of getTargetLanguages() is a good idea. What about caching it right after requesting a resolution? This way we would cache it while the resolution is happening, saving 2 JNI calls when multiples animations might be happening, while at same time working in the case where the user changes the translation preferences. Let me know what you think. Other than that, this CL is looking good. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:750: List<String> targetLanguages = getTargetLanguages(); On 2015/10/28 22:17:15, Donn Denman wrote: > On 2015/10/23 08:50:36, pedrosimonetti wrote: > > We should cache the targetLanguages to prevent calling getTargetLanguages() > > every time. I wouldn't cache it here either, since this method might be called > > during animations. Maybe cache during the Manager's initialize() method? > I think we should call getTargetLanguages every time, since the user can respond > to an infobar at any time that says "don't ever translate" some language and > that's reflected in the accept-headers and the target languages. > > > We should also check mPolicy.isTranslationEnabled() and only cache it when > > necessary. > Already done -- all isTranslationEnabled does is check the fieldtrial, which > caches. > My point is that getTargetLanguages() makes a 2 JNI calls and the user changing the translation preferences should happen only once per language the user speaks, which pretty rare. The mSearchPanelDelegate.onSearchTermResolutionResponse(message) call above will start the resolution animation and will most likely cause a view to be inflated (unless the bar was already peeking). Besides that, the icon and/or peeking animations might be happening at the same time, which makes things even worse (less time for each animation to process its changes). We could cache it when right after requesting a resolution, for example. This way when the resolution is done we would would have it ready. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:811: private List<String> getTargetLanguages() { On 2015/10/28 22:17:15, Donn Denman wrote: > On 2015/10/23 08:50:36, pedrosimonetti wrote: > > This method makes 2 JNI calls so we should cache its return value to avoid > > making those calls every time. > > I think caching the target language is fine since it seldom changes. So I added > a method here to cache that, and removed the caching from the native code > called. > > However, As I said above, I don't think we should cache the accept-languages, > since the user can easily change this by responding to an infobar to never > translate <x>. See comment above. https://codereview.chromium.org/1354763003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java (right): https://codereview.chromium.org/1354763003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:20: public interface ContextualSearchPanelDelegate { This file has been removed. It should not be added back. https://codereview.chromium.org/1354763003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1354763003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:314: ContextualSearchRequest getRequest() { Nit: Instead of exposing the request, why not making this renaming this method to isTranslationForced() and return that check instead? https://codereview.chromium.org/1354763003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:496: didRequestSurroundings = true; We could cache the result of getTargetLanguages() here, for example. https://codereview.chromium.org/1354763003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:749: List<String> targetLanguages = getTargetLanguages(); See comment on previous patch.
Thank you Pedro! https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:750: List<String> targetLanguages = getTargetLanguages(); On 2015/10/30 00:58:42, pedrosimonetti wrote: > On 2015/10/28 22:17:15, Donn Denman wrote: > > On 2015/10/23 08:50:36, pedrosimonetti wrote: > > > We should cache the targetLanguages to prevent calling getTargetLanguages() > > > every time. I wouldn't cache it here either, since this method might be > called > > > during animations. Maybe cache during the Manager's initialize() method? > > I think we should call getTargetLanguages every time, since the user can > respond > > to an infobar at any time that says "don't ever translate" some language and > > that's reflected in the accept-headers and the target languages. > > > > > We should also check mPolicy.isTranslationEnabled() and only cache it when > > > necessary. > > Already done -- all isTranslationEnabled does is check the fieldtrial, which > > caches. > > > > My point is that getTargetLanguages() makes a 2 JNI calls and the user changing > the translation preferences should happen only once per language the user > speaks, which pretty rare. > > The mSearchPanelDelegate.onSearchTermResolutionResponse(message) call above will > start the resolution animation and will most likely cause a view to be inflated > (unless the bar was already peeking). > > Besides that, the icon and/or peeking animations might be happening at the same > time, which makes things even worse (less time for each animation to process its > changes). > > We could cache it when right after requesting a resolution, for example. This > way when the resolution is done we would would have it ready. Done, but for the record, I think this is premature optimization. Having said that, I don't actually know how expensive those JNI calls are, and caching as you suggest doesn't seem too bad. I agree that the user doesn't choose new languages to not translate very often. However, to nit pick your comment, it could be more often than the languages they *do* speak (could be dozens). https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:811: private List<String> getTargetLanguages() { On 2015/10/30 00:58:42, pedrosimonetti wrote: > On 2015/10/28 22:17:15, Donn Denman wrote: > > On 2015/10/23 08:50:36, pedrosimonetti wrote: > > > This method makes 2 JNI calls so we should cache its return value to avoid > > > making those calls every time. > > > > I think caching the target language is fine since it seldom changes. So I > added > > a method here to cache that, and removed the caching from the native code > > called. > > > > However, As I said above, I don't think we should cache the accept-languages, > > since the user can easily change this by responding to an infobar to never > > translate <x>. > > See comment above. Done. https://codereview.chromium.org/1354763003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:853: String trimmedLocale = locale.substring(0, 2); On 2015/10/23 08:50:36, pedrosimonetti wrote: > Do we really need to trim it? I think > Locale.forLanguageTag(locale).getLanguage() should return just the language > part, excluding the country/region part. > > Locale.forLanguageTag() expects the code with "-", not "_". If the locale string > uses underscore, you would have to replace them. Still, I think it's better > because it doesn't assume the size of the language part of the code (here > assumed to be 2). > > http://docs.oracle.com/javase/7/docs/api/java/util/Locale.html#def_language Unfortunately this seems to use API 21, and we're using a lower API currently. I will need to land another CL when the translation API changes, so we should get someone more translation-savvy to review all my TODOs then, e.g. Newt.
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, pedrosimonetti@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1354763003/#ps160001 (title: "Caching getTargetLanguages, as suggested by Pedro in his last review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1354763003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1354763003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Pedro, PTAL: needed to fix the unit tests. I'd verified the instrumentation tests worked, but forgot that I'd updated the unit tests too.
Thanks for the changes Donn. I have just a single nit, otherwise this CL looks good to me. https://codereview.chromium.org/1354763003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1354763003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:125: private List<String> mTargetLanguages; Nit: Should it be static?
Thanks Pedro! https://codereview.chromium.org/1354763003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1354763003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:125: private List<String> mTargetLanguages; On 2015/11/03 19:36:19, pedrosimonetti wrote: > Nit: Should it be static? I wondered about that too, and concluded that it doesn't really matter since this class is essentially a singleton. I'm inclined to leave it as a class member.
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, pedrosimonetti@chromium.org Link to the patchset: https://codereview.chromium.org/1354763003/#ps180001 (title: "Fix unit tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1354763003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1354763003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, pedrosimonetti@chromium.org Link to the patchset: https://codereview.chromium.org/1354763003/#ps200001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1354763003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1354763003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, pedrosimonetti@chromium.org Link to the patchset: https://codereview.chromium.org/1354763003/#ps220001 (title: "Rebase, and remove ContextualSearchPanelDelegate.java (already removed from repo).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1354763003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1354763003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/11/04 02:55:37, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) Pedro, PTAL at my latest revision of ContextualSearchManagerTest. I forgot that my phone had a flag set, and needed to set it in the tests so they would work on the bots! I guess we know the feature's behind a flag though. :-)
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, pedrosimonetti@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1354763003/#ps240001 (title: "Add flag settings to new instrumentation tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1354763003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1354763003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Pedro, PTAL. Thanks!
lgtm Just a single nit to add a comment. Change is looking good! https://codereview.chromium.org/1354763003/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java (right): https://codereview.chromium.org/1354763003/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:395: * @return Whether onShow() was ever called for the current {@code ContentViewCore}. Oops. Sorry I forgot to document this. https://codereview.chromium.org/1354763003/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:482: "Resolution", "Resolution", "alternate-term", false, 0, 0, "")); I'm happy with how simple it is now to add new fake searches! :) https://codereview.chromium.org/1354763003/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1354763003/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:459: doesMatch = doesMatch || mFakeServer.getLoadedUrl().contains("tlitetxt=" + searchTerm); Nit: Please add a comment explaining what is happening here (that the translation is triggered by forcing with a certain query, therefore in that case we need to check for other parameter). Otherwise it will not be obvious to others not familiar with the translate API.
Thanks Pedro! https://chromiumcodereview.appspot.com/1354763003/diff/260001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/260001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:395: * @return Whether onShow() was ever called for the current {@code ContentViewCore}. On 2015/11/05 02:09:58, pedrosimonetti wrote: > Oops. Sorry I forgot to document this. Acknowledged. https://chromiumcodereview.appspot.com/1354763003/diff/260001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:482: "Resolution", "Resolution", "alternate-term", false, 0, 0, "")); On 2015/11/05 02:09:58, pedrosimonetti wrote: > I'm happy with how simple it is now to add new fake searches! :) Yep, this looks good. I especially like how brief the actual tests are! https://chromiumcodereview.appspot.com/1354763003/diff/260001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/1354763003/diff/260001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:459: doesMatch = doesMatch || mFakeServer.getLoadedUrl().contains("tlitetxt=" + searchTerm); On 2015/11/05 02:09:58, pedrosimonetti wrote: > Nit: Please add a comment explaining what is happening here (that the > translation is triggered by forcing with a certain query, therefore in that case > we need to check for other parameter). Otherwise it will not be obvious to > others not familiar with the translate API. Done.
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, pedrosimonetti@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1354763003/#ps280001 (title: "Updated comment about test infrastructure needing to be updated when shifting to new API.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1354763003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1354763003/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/9959a95fc4fedd724decdd1a5b938803c4990a7d Cr-Commit-Position: refs/heads/master@{#358101} |
