Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2816333002:
Add tests for TITLE_ONLY Custom Tab Toolbar (Closed)
DescriptionAdd tests for TITLE_ONLY Custom Tab Toolbar
Added test checking various conditions with TITLE_ONLY state. There
were conditions with prerender that were not covered by any integration
tests.
BUG=708957
Review-Url: https://codereview.chromium.org/2816333002
Cr-Commit-Position: refs/heads/master@{#467442}
Committed: https://chromium.googlesource.com/chromium/src/+/5febbe024bd044dc8ffb83cbe7acb58621d8f2c1
Patch Set 1 #
Total comments: 11
Patch Set 2 : Adding tests only now #
Total comments: 6
Patch Set 3 : tedchoc@ comments #Messages
Total messages: 17 (7 generated)
yusufo@chromium.org changed reviewers: + tedchoc@chromium.org
yusufo@chromium.org changed reviewers: + dfalcantara@chromium.org
https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:143: if (!TextUtils.isEmpty(tab.getTitle()) && !tab.getUrl().contains(tab.getTitle())) { what is the second statement doing? Why does the url containing the title matter? https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:147: LoadUrlParams params = new LoadUrlParams("about:blank"); s/about:blank/ContentUrlConstants.ABOUT_BLANK_URL https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:369: if (getCurrentTab().isLoading() || getCurrentTab().isHidden()) return; do we get signals for when these changes happen? I'm wondering if there is ever a chance we don't get setUrlToPageUrl called after these change to their final values and thus end up showing a nothing. https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:371: postDelayed(mNoTitleFallback, 1000); make 1000 a constant? https://codereview.chromium.org/2816333002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2816333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1082: * Tests that basic postMessage functionality works through sending a single postMessage I think your comments on the new tests need updating :-P Also, what do you think about moving this to CustomTabTitleTest. This file is getting...large. https://codereview.chromium.org/2816333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1128: fail(); should we include the exception here and below? https://codereview.chromium.org/2816333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1199: return "about:blank".equals(currentTab.getUrl()); So the page has navigated to about:blank, but the title bar will show the URL? Isn't that disconnect weird? Any reason we don't just kick them out of setHideDomainForSession for the remainder of the session?
Responding to the more debated decisions I had to make first, so that we can converge on a solution. I agree that some of these are highly debatable. https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:143: if (!TextUtils.isEmpty(tab.getTitle()) && !tab.getUrl().contains(tab.getTitle())) { On 2017/04/17 20:51:32, Ted C wrote: > what is the second statement doing? Why does the url containing the title > matter? Responding to this without uploading first, because I know it is vague and will probably require some debate: This is the best way I found to handle the "There is no title and getTitle fallback to the url" case. We get the title as the url through navigation_entry_impl.cc when the page has either has no title at all or when it is empty string. But we can't do equals, because the title doesn't contain the scheme, so is never exactly equal to url. This was my take on what we can check in that situation. It doesn't conflict with the TITLE_ONLY state because of the way, we write out the titles in this particular state. https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:369: if (getCurrentTab().isLoading() || getCurrentTab().isHidden()) return; On 2017/04/17 20:51:32, Ted C wrote: > do we get signals for when these changes happen? I'm wondering if there is ever > a chance we don't get setUrlToPageUrl called after these change to their final > values and thus end up showing a nothing. For loading, yes we do get it at every loading related update. We dont for onShow, but all except the first one comes after the tab has been shown. The isHidden check is there to bypass the very first force update after we initialize native. https://codereview.chromium.org/2816333002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2816333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1199: return "about:blank".equals(currentTab.getUrl()); On 2017/04/17 20:51:32, Ted C wrote: > So the page has navigated to about:blank, but the title bar will show the URL? > Isn't that disconnect weird? Any reason we don't just kick them out of > setHideDomainForSession for the remainder of the session? No we first make sure the title is showing the url and then we wait until we have navigated to about:blank. Going to about:blank is currently given as the necessary fallback in the linked bug.
https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:143: if (!TextUtils.isEmpty(tab.getTitle()) && !tab.getUrl().contains(tab.getTitle())) { On 2017/04/17 21:17:36, Yusuf wrote: > On 2017/04/17 20:51:32, Ted C wrote: > > what is the second statement doing? Why does the url containing the title > > matter? > > Responding to this without uploading first, because I know it is vague and will > probably require some debate: > > This is the best way I found to handle the "There is no title and getTitle > fallback to the url" case. We get the title as the url through > navigation_entry_impl.cc when the page has either has no title at all or when it > is empty string. > > But we can't do equals, because the title doesn't contain the scheme, so is > never exactly equal to url. This was my take on what we can check in that > situation. It doesn't conflict with the TITLE_ONLY state because of the way, we > write out the titles in this particular state. :-/ Hmm...that is indeed less than ideal. Ideally, we could use NavigationEntry#getTitle instead of getTitleForDisplay that web_contents_impl defers into. The problem is that GetTitle in WebContentsImpl does non-trivial logic to determine which NavigationEntry to use for titles. We "could" expose a new method from WebContentsImpl like GetNavigationEntryForTitle but that would certainly require more platform discussion. I'd argue it is probably "more" correct than this though. If we have to keep this logic as is, I think it might be worth pulling out a helper function that really describes this clearly (at least the title vs url comparison as it is used elsewhere and not immediately obvious). I guess the bigger question I have is that if the url contains the title, is that going to be flagged as a bad situation and set to about:blank or given more access? I think if we are more likely to treat things as errors than we can be a bit looser with those checks vs granted permission on loose checks.
Description was changed from ========== Navigate to about:blank if title only CCT toolbar doesn't get a title This is a fallback for the special case while CCT is not showing the domain but only a title, to navigate to about:blank as a fallback to showing a page with no title set. In this situation, the toolbar attempts to show the url as fallback, but this new addition navigates to about:blank. Added test checking various conditions with TITLE_ONLY state. BUG=708957 ========== to ========== Add tests for TITLE_ONLY Custom Tab Toolbar Added test checking various conditions with TITLE_ONLY state. There were conditions with prerender that were not covered by any integration tests. BUG=708957 ==========
On 2017/04/18 04:46:27, Ted C wrote: > https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java > (right): > > https://codereview.chromium.org/2816333002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:143: > if (!TextUtils.isEmpty(tab.getTitle()) && > !tab.getUrl().contains(tab.getTitle())) { > On 2017/04/17 21:17:36, Yusuf wrote: > > On 2017/04/17 20:51:32, Ted C wrote: > > > what is the second statement doing? Why does the url containing the title > > > matter? > > > > Responding to this without uploading first, because I know it is vague and > will > > probably require some debate: > > > > This is the best way I found to handle the "There is no title and getTitle > > fallback to the url" case. We get the title as the url through > > navigation_entry_impl.cc when the page has either has no title at all or when > it > > is empty string. > > > > But we can't do equals, because the title doesn't contain the scheme, so is > > never exactly equal to url. This was my take on what we can check in that > > situation. It doesn't conflict with the TITLE_ONLY state because of the way, > we > > write out the titles in this particular state. > > :-/ Hmm...that is indeed less than ideal. Ideally, we could use > NavigationEntry#getTitle instead of getTitleForDisplay that web_contents_impl > defers into. The problem is that GetTitle in WebContentsImpl does non-trivial > logic to determine which NavigationEntry to use for titles. We "could" expose a > new method from WebContentsImpl like GetNavigationEntryForTitle but that would > certainly require more platform discussion. I'd argue it is probably "more" > correct than this though. > > If we have to keep this logic as is, I think it might be worth pulling out a > helper function that really describes this clearly (at least the title vs url > comparison as it is used elsewhere and not immediately obvious). > > I guess the bigger question I have is that if the url contains the title, is > that going to be flagged as a bad situation and set to about:blank or given more > access? I think if we are more likely to treat things as errors than we can be > a bit looser with those checks vs granted permission on loose checks. The debate about the right fallback is not completely resolved. And seeing that there may be getTitle related cleanup to clarify logic, I split this CL into two. These initial tests checks the main regression that happening during the bug. So better to land these first and then handle the fallback separately.
lgtm https://codereview.chromium.org/2816333002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2816333002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1075: * Tests that basic postMessage functionality works through sending a single postMessage Update test description here and below https://codereview.chromium.org/2816333002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1104: private void hideDomainAndEnsureTitleIsSet(final String url, int speculation) { I'd probably have this pass expected title instead of hardcoding nytimes.com below https://codereview.chromium.org/2816333002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1130: CriteriaHelper.pollInstrumentationThread(new Criteria() { UiThread?
https://codereview.chromium.org/2816333002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2816333002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1075: * Tests that basic postMessage functionality works through sending a single postMessage On 2017/04/25 21:25:43, Ted C wrote: > Update test description here and below Done. https://codereview.chromium.org/2816333002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1104: private void hideDomainAndEnsureTitleIsSet(final String url, int speculation) { On 2017/04/25 21:25:43, Ted C wrote: > I'd probably have this pass expected title instead of hardcoding http://nytimes.com > below Done. https://codereview.chromium.org/2816333002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1130: CriteriaHelper.pollInstrumentationThread(new Criteria() { On 2017/04/25 21:25:43, Ted C wrote: > UiThread? Done.
The CQ bit was checked by yusufo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2816333002/#ps40001 (title: "tedchoc@ comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493234947349080,
"parent_rev": "ca0862600acc57baec238c114be2af70e889708f", "commit_rev":
"5febbe024bd044dc8ffb83cbe7acb58621d8f2c1"}
Message was sent while issue was closed.
Description was changed from ========== Add tests for TITLE_ONLY Custom Tab Toolbar Added test checking various conditions with TITLE_ONLY state. There were conditions with prerender that were not covered by any integration tests. BUG=708957 ========== to ========== Add tests for TITLE_ONLY Custom Tab Toolbar Added test checking various conditions with TITLE_ONLY state. There were conditions with prerender that were not covered by any integration tests. BUG=708957 Review-Url: https://codereview.chromium.org/2816333002 Cr-Commit-Position: refs/heads/master@{#467442} Committed: https://chromium.googlesource.com/chromium/src/+/5febbe024bd044dc8ffb83cbe7ac... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5febbe024bd044dc8ffb83cbe7ac... |
