Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2351283002:
[TTS] Decode Now on Tap results for v1 integration. (Closed)
|
Created:
4 years, 3 months ago by Donn Denman Modified:
4 years, 3 months ago Reviewers:
Theresa CC:
chromium-reviews, twellington+watch_chromium.org, donnd+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Decode Now on Tap results for v1 integration.
Decodes the results from the Contextual Cards section of the response to set
caption and thumbnail.
BUG=647041
Committed: https://crrev.com/d60e1f0ccb9666bfecc31b3547658f379a991d28
Cr-Commit-Position: refs/heads/master@{#419918}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Used command-line switch for test, rollback changes needed for the field trial subclass. #
Messages
Total messages: 19 (9 generated)
donnd@chromium.org changed reviewers: + twellington@chromium.org
Theresa, PTAL. I'm not sure whether we need to fake data class in there or not, so let me know if you'd like me to rip it out. Certainly don't spend time checking it, since it's throw-away code. The server integration seems to be moving pretty quickly so we may not need it. Unfortunately we have some terminology mismatches -- the Coca folks like using "Contextual Cards", so that's what we're thinking the server will return in the JSON field for all their data. We've been using "Now on Tap" integration so there is some confusion. I'm thinking the boundary is client/server -- server returns contextual cards, we in the client treat everything as Now on Tap unless it came from the server as a contextual card response.
On 2016/09/20 19:14:48, Donn Denman wrote: > Theresa, PTAL. > > I'm not sure whether we need to fake data class in there or not, so let me know > if you'd like me to rip it out. Certainly don't spend time checking it, since > it's throw-away code. The server integration seems to be moving pretty quickly > so we may not need it. > > Unfortunately we have some terminology mismatches -- the Coca folks like using > "Contextual Cards", so that's what we're thinking the server will return in the > JSON field for all their data. We've been using "Now on Tap" integration so > there is some confusion. I'm thinking the boundary is client/server -- server > returns contextual cards, we in the client treat everything as Now on Tap unless > it came from the server as a contextual card response. I think we should just change the terminology client side. We would need to change the field trial flags and some method names, but probably not anything else, right?
On 2016/09/20 19:16:14, Theresa Wellington wrote: > On 2016/09/20 19:14:48, Donn Denman wrote: > > Theresa, PTAL. > > > > I'm not sure whether we need to fake data class in there or not, so let me > know > > if you'd like me to rip it out. Certainly don't spend time checking it, since > > it's throw-away code. The server integration seems to be moving pretty > quickly > > so we may not need it. > > > > Unfortunately we have some terminology mismatches -- the Coca folks like using > > "Contextual Cards", so that's what we're thinking the server will return in > the > > JSON field for all their data. We've been using "Now on Tap" integration so > > there is some confusion. I'm thinking the boundary is client/server -- server > > returns contextual cards, we in the client treat everything as Now on Tap > unless > > it came from the server as a contextual card response. > > I think we should just change the terminology client side. We would need to > change the field trial flags and some method names, but probably not anything > else, right? Unfortunately this term gets pushed down into TemplateURL too. See https://chromiumcodereview.appspot.com/2343523002/ But I'm OK doing that as long as it seems like the right thing to do.
I think that at the very least we should use "contextual cards" for new things. "Now on tap" is a feature; we used it because we couldn't think of a better name but now we have one :)
https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/FakeNowOnTapResults.java (right): https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/FakeNowOnTapResults.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. s/2015/2016 https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/FakeNowOnTapResults.java:17: * TODO(donnd): remove this throw-away code! The server code is getting close, right? How long will this throw away code be used? Also, can the faked data be landed separately from the v1 parsing so that it's easy to revert the CL? https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... File chrome/browser/android/contextualsearch/contextual_search_delegate.cc (right): https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate.cc:507: DecodeNowOnTapResponse(*dict.get(), caption, thumbnail_url); nit: I think it makes sense to change "caption" to "subtitle" everywhere up to the contextual_search_manager.cc call to onSetCaption(). https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate.cc:561: DCHECK(contextual_cards_dict); I don't think this DCHECK is necessary since we return above if contextual_cards_dict doesn't exist. https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... File chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:46: bool IsNowOnTapBarIntegrationEnabled() override { return true; } I don't think this is necessary. There should be a way to set command line flags for unit tests just like do for Java. Maybe something like: base::CommandLine::ForCurrentProcess()->AppendSwitch(...); https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:597: "{\"search_term\":\"obama\"," Can we use escapeBarQuoted() here too? https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:611: // Same test using helpers and "|" quoting (for readability). Does duplicating the test give us any extra code coverage? If not, can we remove the one above?
Description was changed from ========== [TTS] Decode Now on Tap results for v1 integration. Decodes the results from the Now on Tap section of the response to set caption and thumbnail. Adds simple dependency injection of the Field Trial in ContextualSearchDelegate. Updates unit tests for FieldTrial (which were misspelled FieldTrail). Also adds fake Now on Tap data for testing. BUG=647041 ========== to ========== [TTS] Decode Now on Tap results for v1 integration. Decodes the results from the Contextual Cards section of the response to set caption and thumbnail. BUG=647041 ==========
Thanks for the review Theresa! Using the command-line switch simplified things a lot. Also I decided to put the renaming of that fieldtrial test class into a separate CL, since it's unrelated. https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/FakeNowOnTapResults.java (right): https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/FakeNowOnTapResults.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/09/20 19:51:39, Theresa Wellington wrote: > s/2015/2016 Doh! https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/FakeNowOnTapResults.java:17: * TODO(donnd): remove this throw-away code! On 2016/09/20 19:51:39, Theresa Wellington wrote: > The server code is getting close, right? How long will this throw away code be > used? Also, can the faked data be landed separately from the v1 parsing so that > it's easy to revert the CL? Great idea, I'll hopefully use this in the future! For now I think we can just skip it altogether -- the server is getting close to being ready and now that this code has been uploaded it will be easy to grab a CL for just the fake server response. https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... File chrome/browser/android/contextualsearch/contextual_search_delegate.cc (right): https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate.cc:507: DecodeNowOnTapResponse(*dict.get(), caption, thumbnail_url); On 2016/09/20 19:51:39, Theresa Wellington wrote: > nit: I think it makes sense to change "caption" to "subtitle" everywhere up to > the contextual_search_manager.cc call to onSetCaption(). Mostly done. The ContextualSearchContext knows of this as the caption, and I think that's appropriate, since it's more generic than a subtitle (suitable for translations, etc). But I agree that when it comes from Contextual Cards it should be a subtitle. https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate.cc:561: DCHECK(contextual_cards_dict); On 2016/09/20 19:51:40, Theresa Wellington wrote: > I don't think this DCHECK is necessary since we return above if > contextual_cards_dict doesn't exist. In that case we should remove all of these DCHECKS. I'm inclined to leave them in to help in case of a crash. Compromise: Added a TODO to remove them all once this code has been exercised, and removed the one you flagged. https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... File chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:46: bool IsNowOnTapBarIntegrationEnabled() override { return true; } On 2016/09/20 19:51:40, Theresa Wellington wrote: > I don't think this is necessary. There should be a way to set command line flags > for unit tests just like do for Java. > > Maybe something like: > base::CommandLine::ForCurrentProcess()->AppendSwitch(...); Oh, I feel bad that I didn't figure this out on my own. That simplifies this CL a lot. THANKS! https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:597: "{\"search_term\":\"obama\"," On 2016/09/20 19:51:40, Theresa Wellington wrote: > Can we use escapeBarQuoted() here too? I'll just remove this one. https://chromiumcodereview.appspot.com/2351283002/diff/1/chrome/browser/andro... chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:611: // Same test using helpers and "|" quoting (for readability). On 2016/09/20 19:51:40, Theresa Wellington wrote: > Does duplicating the test give us any extra code coverage? If not, can we remove > the one above? Done.
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [TTS] Decode Now on Tap results for v1 integration. Decodes the results from the Contextual Cards section of the response to set caption and thumbnail. BUG=647041 ========== to ========== [TTS] Decode Now on Tap results for v1 integration. Decodes the results from the Contextual Cards section of the response to set caption and thumbnail. BUG=647041 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [TTS] Decode Now on Tap results for v1 integration. Decodes the results from the Contextual Cards section of the response to set caption and thumbnail. BUG=647041 ========== to ========== [TTS] Decode Now on Tap results for v1 integration. Decodes the results from the Contextual Cards section of the response to set caption and thumbnail. BUG=647041 Committed: https://crrev.com/d60e1f0ccb9666bfecc31b3547658f379a991d28 Cr-Commit-Position: refs/heads/master@{#419918} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d60e1f0ccb9666bfecc31b3547658f379a991d28 Cr-Commit-Position: refs/heads/master@{#419918} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
