Calculator Omnibox Suggestions Should Use Calculator-Specific Icon |
|||||||||
Issue description
Steps to repro:
0. run in debug
1. tap omnibox
2. type "2+2"
expected results: no NOTREACHED()
actual results: NOTREACHED() in GetIconForAutocompleteMatchType (line 47)
int GetIconForAutocompleteMatchType(AutocompleteMatchType::Type type,
bool is_starred,
bool is_incognito) {
if (is_starred)
return is_incognito ? IDR_IOS_OMNIBOX_STAR_INCOGNITO : IDR_IOS_OMNIBOX_STAR;
switch (type) {
case AutocompleteMatchType::BOOKMARK_TITLE:
/* more cases */
case AutocompleteMatchType::VOICE_SUGGEST:
return is_incognito ? IDR_IOS_OMNIBOX_SEARCH_INCOGNITO
: IDR_IOS_OMNIBOX_SEARCH;
case AutocompleteMatchType::CALCULATOR:
case AutocompleteMatchType::EXTENSION_APP:
case AutocompleteMatchType::NUM_TYPES:
NOTREACHED(); // <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< this is hit
return IDR_IOS_OMNIBOX_HTTP;
}
}
What is the reason for this NOTREACHED?
,
Aug 11 2017
I can't imagine why we wouldn't want calculator results on iOS. It needs to have a different icon, though, (omnibox::kCalculatorIcon, looks like '='). I can make this change if you like but why is it RBS if it only happens in debug mode?
,
Aug 14 2017
If this only happens in debug mode, then it should not be an RBS. Please confirm and remove RBS label accordingly.
,
Aug 14 2017
Yup, NOTREACHED is defined as DCHECK which is debug-only. (Also, the reporter's repro instructions include running a debug build.)
,
Sep 22 2017
Hi, This is marked P1 for M62. Please provide updates with the fix plan or blockers. Thanks,
,
Sep 22 2017
Since this is a debug-mode-only failure, re-prioritizing as P3.
,
Sep 23 2017
This is not debug-only. It is a user-visible bug. It means that calculator suggestions are displayed with the wrong icon (the HTML page icon). Yet, using a calculator icon is not a normal page navigation: it's a search. It should either have the query icon or the calculator icon. I'll change the summary for clarity.
,
Oct 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f75f3241a16cbee7f5c72054cc954b343a34cfb commit 7f75f3241a16cbee7f5c72054cc954b343a34cfb Author: Justin Donnelly <jdonnelly@google.com> Date: Wed Oct 04 14:06:16 2017 Use the search icon for iOS omnibox calculator matches. This matches the behavior on Android and avoids a DCHECK when calculator matches are produced. It would be better if both platforms used the calculator-specific icon (looks like an equal sign) but that's only available as a vector graphic. If we can get a png or start using vector icons on Android and iOS, we should switch. But for now this is an improvement. Bug: 754221 Change-Id: Ie589349e6507b7e0b21896df07b4354e4bc37714 Reviewed-on: https://chromium-review.googlesource.com/699315 Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org> Commit-Queue: Justin Donnelly <jdonnelly@chromium.org> Cr-Commit-Position: refs/heads/master@{#506382} [modify] https://crrev.com/7f75f3241a16cbee7f5c72054cc954b343a34cfb/ios/chrome/browser/ui/omnibox/omnibox_util.cc
,
Oct 4 2017
Revising title to clarify remaining work, and demoting priority.
,
Oct 5 2017
Note that suggestion type icons are only shown on tablets. On phones, we should prefix calculator suggestions with '= ' text instead. Filed https://crbug.com/772015 to track this.
,
Oct 5 2017
,
Oct 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/07a87367c2a3ccd34d4a149628b765fb4fb9fab6 commit 07a87367c2a3ccd34d4a149628b765fb4fb9fab6 Author: Justin Donnelly <jdonnelly@google.com> Date: Fri Oct 06 16:41:02 2017 Restore the png version of the omnibox calculator icon for use on iOS. These were removed when vector icons were introduced to replace them but as far as I know, we don't support vector graphics images on iOS. Bug: 754221 Change-Id: I356aa9c4f8c3bc4040909f8d26530a404edb4509 Reviewed-on: https://chromium-review.googlesource.com/702723 Commit-Queue: Justin Donnelly <jdonnelly@chromium.org> Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org> Cr-Commit-Position: refs/heads/master@{#507096} [add] https://crrev.com/07a87367c2a3ccd34d4a149628b765fb4fb9fab6/ios/chrome/app/theme/default_100_percent/omnibox_calculator.png [add] https://crrev.com/07a87367c2a3ccd34d4a149628b765fb4fb9fab6/ios/chrome/app/theme/default_200_percent/omnibox_calculator.png [add] https://crrev.com/07a87367c2a3ccd34d4a149628b765fb4fb9fab6/ios/chrome/app/theme/default_300_percent/omnibox_calculator.png [modify] https://crrev.com/07a87367c2a3ccd34d4a149628b765fb4fb9fab6/ios/chrome/app/theme/ios_theme_resources.grd [modify] https://crrev.com/07a87367c2a3ccd34d4a149628b765fb4fb9fab6/ios/chrome/browser/ui/omnibox/omnibox_util.cc
,
Oct 12 2017
Jdonnelly is this issue considered fixed now? Also this cl broke test as described in issue 772746
,
Oct 16 2017
Yes, this is fixed. Sorry about the test breakage, responded on the other bug.
,
Oct 17 2017
np!
,
Jan 9 2018
The NextAction date has arrived: 2018-01-09 |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by justincohen@chromium.org
, Aug 10 2017Owner: jdonnelly@chromium.org