Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
490383002:
Loosen DCHECK for Omnibox Extensions (Closed)
|
Created:
6 years, 4 months ago by Mark P Modified:
6 years, 4 months ago CC:
chromium-reviews, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionLoosen DCHECK for Omnibox Extensions
Omnibox extensions don't need a default match from SearchProvider, and
indeed they no longer get them following some refactoring I did last week
( https://codereview.chromium.org/476263002/ ).
BUG=406026
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291461
Patch Set 1 #
Total comments: 4
Patch Set 2 : Peter + Mike's comments #
Total comments: 10
Patch Set 3 : Mike's polish #
Total comments: 4
Patch Set 4 : more polish #Messages
Total messages: 19 (0 generated)
Hey guys, please take a look at your convenience. thanks, mark
RSLGTM https://codereview.chromium.org/490383002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/490383002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:438: // when in an extension-based keyword mode). The omnibox always needs Nit: Remove "an"
LGTM with a nit. Too bad SearchProvider behavior depends on the extension KeywordProvider behavior, but the provider logic is pretty interdependent in general... </whining> https://codereview.chromium.org/490383002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/490383002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:414: (keyword_url->GetType() != TemplateURL::OMNIBOX_API_EXTENSION) && nit: maybe make a local bool is_extension_keyword?
https://codereview.chromium.org/490383002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/490383002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:414: (keyword_url->GetType() != TemplateURL::OMNIBOX_API_EXTENSION) && On 2014/08/22 00:48:00, msw wrote: > nit: maybe make a local bool is_extension_keyword? Good idea. Done. https://codereview.chromium.org/490383002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:438: // when in an extension-based keyword mode). The omnibox always needs On 2014/08/22 00:21:09, Peter Kasting wrote: > Nit: Remove "an" Done.
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/490383002/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:414: (keyword_url->GetType() == TemplateURL::OMNIBOX_API_EXTENSION); This should be keyword_url != NULL && ... https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:415: if ((keyword_url != NULL) && !is_extension_keyword && fyi: you'll still need this NULL check too. https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:436: ((keyword_url == NULL) || !is_extension_keyword)) { nit: nix (keyword_url == NULL), use |is_extension_keyword|'s NULL check. https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:438: // when in n extension-based keyword mode). The omnibox always needs Remove stray 'n'. https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:447: ((keyword_url != NULL) && is_extension_keyword)); nit: nix (keyword_url == NULL), use |is_extension_keyword|'s NULL check.
Thanks for the help polishing this small changelist. I think my brain is not yet awake. --mark https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:414: (keyword_url->GetType() == TemplateURL::OMNIBOX_API_EXTENSION); On 2014/08/22 17:45:56, msw wrote: > This should be keyword_url != NULL && ... No wonder the tests are failing. (I hadn't looked yet.) https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:415: if ((keyword_url != NULL) && !is_extension_keyword && On 2014/08/22 17:45:56, msw wrote: > fyi: you'll still need this NULL check too. Acknowledged. https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:436: ((keyword_url == NULL) || !is_extension_keyword)) { On 2014/08/22 17:45:56, msw wrote: > nit: nix (keyword_url == NULL), use |is_extension_keyword|'s NULL check. Done. https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:438: // when in n extension-based keyword mode). The omnibox always needs On 2014/08/22 17:45:56, msw wrote: > Remove stray 'n'. Done. https://codereview.chromium.org/490383002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:447: ((keyword_url != NULL) && is_extension_keyword)); On 2014/08/22 17:45:56, msw wrote: > nit: nix (keyword_url == NULL), use |is_extension_keyword|'s NULL check. Done.
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/490383002/40001
lgtm with optional nits. https://codereview.chromium.org/490383002/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/490383002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:435: if ((FindTopMatch() == matches_.end()) && !is_extension_keyword) { optional nit: reverse conditions to match 415/416 and check the bool first. https://codereview.chromium.org/490383002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:445: DCHECK((FindTopMatch() != matches_.end()) || is_extension_keyword); ditto nit: reverse conditions.
https://codereview.chromium.org/490383002/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/490383002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:435: if ((FindTopMatch() == matches_.end()) && !is_extension_keyword) { On 2014/08/22 17:57:35, msw wrote: > optional nit: reverse conditions to match 415/416 and check the bool first. Done. https://codereview.chromium.org/490383002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:445: DCHECK((FindTopMatch() != matches_.end()) || is_extension_keyword); On 2014/08/22 17:57:35, msw wrote: > ditto nit: reverse conditions. Done.
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/490383002/60001
Message was sent while issue was closed.
Committed patchset #4 (60001) as 291461 |
