Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319033006:
Include a page title in the Physical Web omnibox overflow item (Closed)
|
Created:
4 years, 3 months ago by mattreynolds Modified:
4 years, 3 months ago Reviewers:
Mark P CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInclude a page title in the Physical Web omnibox overflow item
Instead of only listing the number of nearby Physical Web URLs, also include
the page title of the top URL. The title will be truncated if it is too long
to fit in the omnibox popup.
BUG=630769
Committed: https://crrev.com/08c7909beaede13cc84f7ae081274ce7ae10f789
Cr-Commit-Position: refs/heads/master@{#418689}
Patch Set 1 #
Total comments: 28
Patch Set 2 : changes for mpearson@, refactor unit tests #
Total comments: 12
Patch Set 3 : nits #
Total comments: 2
Patch Set 4 : move kPhysicalWebMaxMatches declaration #
Messages
Total messages: 18 (6 generated)
mattreynolds@chromium.org changed reviewers: + mpearson@chromium.org
Hi Mark, here's the CL to adjust the Physical Web content and description strings, PTAL.
https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:39: // The maximum length of the page title in the overflow item. Longer titles will nit: consider: "page title in the overflow item" -> "page title's part of the overflow item's description" https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:41: static const size_t kMaxTitleLength = 15; Where did you get this number from? Calculations of the length of the rest of the overflow item contents (as translated into a set of common languages) combined with typical phone screen sizes? (I don't expect that much, but I would like to make sure you thought about how this number works internationally and across devices.) https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:172: if (title.length() > kMaxTitleLength) { This is slightly wrong if indeed as you say above "Longer titles will be truncated to this length." After all, you're adding three characters to the title, so using this comparison (without taking that into account) means that titles could be up to two characters longer than kMaxTitleLength. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:173: contents_title = title.substr(0, kMaxTitleLength) + "... "; You probably should be using one of the standard constants here, perhaps gfx::kEllipsisUTF16. Then you probably want to append the " " unconditionally. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:176: } Please consider what happens with empty titles. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:176: } Please also consider what happens with non-latin characters. For example, can titles be in a RTL language? Also, there's probably call AutocompleteMatch::SanitizeString() on the title somewhere in this code path too (or in some common place so you don't call it twice on the same string). https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.h:44: const std::string& title); nit #1: the function comment should mention what |title| refers to / what it does. nit #2: spacing of parameters (style) https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:25: namespace { didn't notice this the first time: should everything really be in an anonymous namespace? Why bother for a test file? https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:138: static void ValidateMatch(const AutocompleteMatch& match, nit: comment. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:239: const size_t additional_url_count = metadata_count - match_count + 1; This is an unusual placement. Either comment these are put them in the loop below. (The optimizing compiler is smart.) Even putting them inline in the ValidateMatch function call is fine with me. I kept trying to figure out where these variables were used outside the loop block. ditto below https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:242: additional_url_count - 1); Also, I'm confused by all these +1 -1 here. Is there a way you can write this in a more straightforward manner? For example, if you put the number calculation inline here, you can simply do metadata_count - match_count with an appropriate comment right? Or perhaps just comment the declaration of additional_url_count better. Right now it's confusing, especially the +2 thing below. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:312: TEST_F(PhysicalWebProviderTest, TestLongPageTitleIsTruncated) { This test is only applicable for whether there's a lot of matches. Either the name or a comment should reflect that putting a lot of matches here is critical. Perhaps TestLongPageTitleIsTruncatedInOverflowItem? You probably also want a comment somewhere that you don't truncate titles when not in the overflow item -- you trust the omnibox display mechanism for truncation that context. You only worry about truncating when you preferentially want to truncate something in the middle of the content string, keeping the rest.
https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:39: // The maximum length of the page title in the overflow item. Longer titles will On 2016/09/08 18:49:50, Mark P wrote: > nit: consider: > "page title in the overflow item" > -> > "page title's part of the overflow item's description" Done. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:41: static const size_t kMaxTitleLength = 15; On 2016/09/08 18:49:49, Mark P wrote: > Where did you get this number from? Calculations of the length of the rest of > the overflow item contents (as translated into a set of common languages) > combined with typical phone screen sizes? > > (I don't expect that much, but I would like to make sure you thought about how > this number works internationally and across devices.) > I found the longest string of "m"s that would fit on a simulated iPhone SE in portrait. I didn't try other languages. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:172: if (title.length() > kMaxTitleLength) { On 2016/09/08 18:49:49, Mark P wrote: > This is slightly wrong if indeed as you say above "Longer titles will be > truncated to this length." After all, you're adding three characters to the > title, so using this comparison (without taking that into account) means that > titles could be up to two characters longer than kMaxTitleLength. I'm now using gfx::TruncateString which ensures the returned string's length is <= kMaxTitleLength, including the ellipsis character. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:173: contents_title = title.substr(0, kMaxTitleLength) + "... "; On 2016/09/08 18:49:49, Mark P wrote: > You probably should be using one of the standard constants here, perhaps > gfx::kEllipsisUTF16. > > Then you probably want to append the " " unconditionally. I switched to using gfx::TruncateString, which automatically appends the ellipsis . https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:176: } On 2016/09/08 18:49:49, Mark P wrote: > Please consider what happens with empty titles. I added an alternate string to use when the title is empty. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:176: } On 2016/09/08 18:49:49, Mark P wrote: > Please also consider what happens with non-latin characters. > For example, can titles be in a RTL language? > Also, there's probably call AutocompleteMatch::SanitizeString() on the title > somewhere in this code path too (or in some common place so you don't call it > twice on the same string). I'm not sure what the best practices are for RTL in this situation, but currently it displays: and 2 more web pages ...[insert RTL page title] This looks wrong to my LTR-accustomed eyes but seems to be consistent with the behavior in the omnibox when composing queries with mixed LTR and RTL. I also moved the SanitizeString call up to avoid needing to call it twice. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.h:44: const std::string& title); On 2016/09/08 18:49:50, Mark P wrote: > nit #1: the function comment should mention what |title| refers to / what it > does. > nit #2: spacing of parameters (style) Done. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:25: namespace { On 2016/09/08 18:49:50, Mark P wrote: > didn't notice this the first time: should everything really be in an anonymous > namespace? Why bother for a test file? Fixed so only the mocks are in the anonymous namespace. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:138: static void ValidateMatch(const AutocompleteMatch& match, On 2016/09/08 18:49:50, Mark P wrote: > nit: comment. Done. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:239: const size_t additional_url_count = metadata_count - match_count + 1; On 2016/09/08 18:49:50, Mark P wrote: > This is an unusual placement. Either comment these are put them in the loop > below. (The optimizing compiler is smart.) Even putting them inline in the > ValidateMatch function call is fine with me. I kept trying to figure out where > these variables were used outside the loop block. > > ditto below Done. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:242: additional_url_count - 1); On 2016/09/08 18:49:50, Mark P wrote: > Also, I'm confused by all these +1 -1 here. Is there a way you can write this > in a more straightforward manner? For example, if you put the number > calculation inline here, you can simply do > metadata_count - match_count > with an appropriate comment > right? > > Or perhaps just comment the declaration of additional_url_count better. Right > now it's confusing, especially the +2 thing below. Yeah, this was pretty confusing. I added some comments and gave names to the intermediate values to make it clearer. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider_unittest.cc:312: TEST_F(PhysicalWebProviderTest, TestLongPageTitleIsTruncated) { On 2016/09/08 18:49:50, Mark P wrote: > This test is only applicable for whether there's a lot of matches. Either the > name or a comment should reflect that putting a lot of matches here is critical. > Perhaps TestLongPageTitleIsTruncatedInOverflowItem? > > You probably also want a comment somewhere that you don't truncate titles when > not in the overflow item -- you trust the omnibox display mechanism for > truncation that context. You only worry about truncating when you > preferentially want to truncate something in the middle of the content string, > keeping the rest. Done. I added a note about truncation behavior in the kMaxTitleLength comment.
https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:41: static const size_t kMaxTitleLength = 15; On 2016/09/09 21:24:54, mattreynolds wrote: > On 2016/09/08 18:49:49, Mark P wrote: > > Where did you get this number from? Calculations of the length of the rest of > > the overflow item contents (as translated into a set of common languages) > > combined with typical phone screen sizes? > > > > (I don't expect that much, but I would like to make sure you thought about how > > this number works internationally and across devices.) > > > > I found the longest string of "m"s that would fit on a simulated iPhone SE in > portrait. I didn't try other languages. Is this the number of "m"s that would fit, or the number of "m" that would fit once you add in the rest of the overflow item's text ("and 1 more web page" for example)? https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:176: } On 2016/09/09 21:24:54, mattreynolds wrote: > On 2016/09/08 18:49:49, Mark P wrote: > > Please also consider what happens with non-latin characters. > > For example, can titles be in a RTL language? > > Also, there's probably call AutocompleteMatch::SanitizeString() on the title > > somewhere in this code path too (or in some common place so you don't call it > > twice on the same string). > > I'm not sure what the best practices are for RTL in this situation, but > currently it displays: > > and 2 more web pages ...[insert RTL page title] > > This looks wrong to my LTR-accustomed eyes but seems to be consistent with the > behavior in the omnibox when composing queries with mixed LTR and RTL. > > I also moved the SanitizeString call up to avoid needing to call it twice. Good enough for now I think. You may want to bring this up when as part of the launch review (internationalization section) for the full rollout. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:44: static const size_t kMaxTitleLength = 15; Please put overflow in the name somewhere here. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:126: base::string16 title = nit: This goes best right after line 120. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:177: base::string16 contents; nit: consider setting match.contents directly in the two passes rather than using a temporary variable. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider_unittest.cc:135: return AutocompleteInput(base::UTF8ToUTF16(url), base::string16::npos, side comment: URLs are in ASCII as far as I know. This change wasn't necessary. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider_unittest.cc:155: // Construct the contents string for an overflow item. Use |truncated_title| nit: Constructs -> Returns https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider_unittest.cc:187: void OverflowItemTestCase(const AutocompleteInput& input, I'm a bit nervous about this function. Rather than testing the output of the match directly, it duplicates some of the logic (in particular the number of item calculations) in the code to calculate what the output should look like. I think that code is correct, as do you, but if we're wrong, then this test case is wrong and won't catch it.
https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:41: static const size_t kMaxTitleLength = 15; On 2016/09/13 23:34:42, Mark P wrote: > On 2016/09/09 21:24:54, mattreynolds wrote: > > On 2016/09/08 18:49:49, Mark P wrote: > > > Where did you get this number from? Calculations of the length of the rest > of > > > the overflow item contents (as translated into a set of common languages) > > > combined with typical phone screen sizes? > > > > > > (I don't expect that much, but I would like to make sure you thought about > how > > > this number works internationally and across devices.) > > > > > > > I found the longest string of "m"s that would fit on a simulated iPhone SE in > > portrait. I didn't try other languages. > > Is this the number of "m"s that would fit, or the number of "m" that would fit > once you add in the rest of the overflow item's text ("and 1 more web page" for > example)? It's the number of "m"s that would fit once the rest of the text is added. https://codereview.chromium.org/2319033006/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:176: } On 2016/09/13 23:34:42, Mark P wrote: > On 2016/09/09 21:24:54, mattreynolds wrote: > > On 2016/09/08 18:49:49, Mark P wrote: > > > Please also consider what happens with non-latin characters. > > > For example, can titles be in a RTL language? > > > Also, there's probably call AutocompleteMatch::SanitizeString() on the title > > > somewhere in this code path too (or in some common place so you don't call > it > > > twice on the same string). > > > > I'm not sure what the best practices are for RTL in this situation, but > > currently it displays: > > > > and 2 more web pages ...[insert RTL page title] > > > > This looks wrong to my LTR-accustomed eyes but seems to be consistent with the > > behavior in the omnibox when composing queries with mixed LTR and RTL. > > > > I also moved the SanitizeString call up to avoid needing to call it twice. > > Good enough for now I think. You may want to bring this up when as part of the > launch review (internationalization section) for the full rollout. Acknowledged. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:44: static const size_t kMaxTitleLength = 15; On 2016/09/13 23:34:42, Mark P wrote: > Please put overflow in the name somewhere here. Done. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:126: base::string16 title = On 2016/09/13 23:34:43, Mark P wrote: > nit: This goes best right after line 120. Done. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:177: base::string16 contents; On 2016/09/13 23:34:42, Mark P wrote: > nit: consider setting match.contents directly in the two passes rather than > using a temporary variable. Done. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider_unittest.cc:135: return AutocompleteInput(base::UTF8ToUTF16(url), base::string16::npos, On 2016/09/13 23:34:43, Mark P wrote: > side comment: URLs are in ASCII as far as I know. This change wasn't necessary. Acknowledged. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider_unittest.cc:155: // Construct the contents string for an overflow item. Use |truncated_title| On 2016/09/13 23:34:43, Mark P wrote: > nit: Constructs -> Returns Done. https://codereview.chromium.org/2319033006/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider_unittest.cc:187: void OverflowItemTestCase(const AutocompleteInput& input, On 2016/09/13 23:34:43, Mark P wrote: > I'm a bit nervous about this function. Rather than testing the output of the > match directly, it duplicates some of the logic (in particular the number of > item calculations) in the code to calculate what the output should look like. I > think that code is correct, as do you, but if we're wrong, then this test case > is wrong and won't catch it. I removed the logic for computing the number of match items and now pass it as a parameter.
lgtm with a trivial comment below --mark https://codereview.chromium.org/2319033006/diff/40001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2319033006/diff/40001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:28: // The maximum number of match results to provide. If the number of nearby nit: Should got at the top of the class definition. https://google.github.io/styleguide/cppguide.html#Declaration_Order
Thanks Mark! https://codereview.chromium.org/2319033006/diff/40001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2319033006/diff/40001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:28: // The maximum number of match results to provide. If the number of nearby On 2016/09/14 19:33:11, Mark P wrote: > nit: Should got at the top of the class definition. > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done.
The CQ bit was checked by mattreynolds@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2319033006/#ps60001 (title: "move kPhysicalWebMaxMatches declaration")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mattreynolds@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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Include a page title in the Physical Web omnibox overflow item Instead of only listing the number of nearby Physical Web URLs, also include the page title of the top URL. The title will be truncated if it is too long to fit in the omnibox popup. BUG=630769 ========== to ========== Include a page title in the Physical Web omnibox overflow item Instead of only listing the number of nearby Physical Web URLs, also include the page title of the top URL. The title will be truncated if it is too long to fit in the omnibox popup. BUG=630769 Committed: https://crrev.com/08c7909beaede13cc84f7ae081274ce7ae10f789 Cr-Commit-Position: refs/heads/master@{#418689} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/08c7909beaede13cc84f7ae081274ce7ae10f789 Cr-Commit-Position: refs/heads/master@{#418689} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
