Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2682453002:
Changed keywords conflicts resolution for extensions search engines. (Closed)
|
Created:
3 years, 10 months ago by Alexander Yashkin Modified:
3 years, 9 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChanged keywords conflicts resolution for extensions search engines.
This is preparation CL for https://codereview.chromium.org/2639153002/.
This CL changes logic of keywords conflicts resolution for extension
controlled search engines. In previous logic last added keyword always
won. New logic does not change extension keywords at all, they become
hidden if more relevant keyword is installed. New priority of keywords is:
omnibox api extension keywords > extension search engines > user engines.
When there are multiple extensions of same type, the last-added wins.
BUG=450534
R=pkasting@chromium.org, vasilii@chromium.org, jochen@chromium.org
Review-Url: https://codereview.chromium.org/2682453002
Cr-Commit-Position: refs/heads/master@{#453100}
Committed: https://chromium.googlesource.com/chromium/src/+/f4eb4359fec12b0cd91dca5b0c8a9857bd55ef7c
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fixed after review, round 1 #Patch Set 3 : Rebase on master #
Total comments: 2
Patch Set 4 : Fixed after review round 2, changed keywords ranking #Patch Set 5 : Rebase on master #Patch Set 6 : Updated test, check removal of search engines. #
Total comments: 27
Patch Set 7 : Updated after review, round 3 #
Total comments: 8
Patch Set 8 : Fixed after review, round 4 #
Total comments: 22
Patch Set 9 : Fixed after review, round 5 #Patch Set 10 : Fixed TemplateURLServiceTest.RepairPrepopulatedSearchEngines #Patch Set 11 : Fixed auto variable type must not deduce to a raw pointer type #
Messages
Total messages: 54 (14 generated)
I have created separate CL with changes in how extension keywords conflicts resolved needed to implement solution in https://codereview.chromium.org/2639153002/.
https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1490: auto turl_data = GenerateDummyTemplateURLData("ext2"); Nit: "ext2" is pretty random, especially when the test doesn't use "ext" or "ext1". Just use "extension". https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1529: // Check that two extensions with same engine handled corrected. Nit: handled corrected -> are handled correctly https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1533: // TemplateURLData used for all extension. Nit: extension -> extensions https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1544: auto extension1 = model()->AddExtensionControlledTURL( Nit: This should not use auto; it's a raw pointer (so it'd be auto*), and the type of the object is not object from the function call, so specify the type explicitly. (2 places) https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1554: model()->AddExtensionControlledTURL(std::move(ext_dse), Nit: Shouldn't this store the result in a temp |extension_2|, and we check that that's DSE below? (Possibly this would obviate the need for some of the existing checks below like the IsExtensionCOntrolledDefaultSearch() and FindTemplateURLForExtension() calls) https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1567: // Wait for any saves to finish. Nit: Blank line above this https://codereview.chromium.org/2682453002/diff/1/components/search_engines/s... File components/search_engines/search_engines_test_util.h (right): https://codereview.chromium.org/2682453002/diff/1/components/search_engines/s... components/search_engines/search_engines_test_util.h:19: std::unique_ptr<TemplateURLData> GenerateDummyTemplateURLData( Are you going to eliminate the identically-named function in default_search_manager_unittest.cc in this CL, or will you be doing that separately? Similar question for ExpectSimilar() and any other overlap between these files. If we're just condensing/moving functionality it would be clearer to see that in this CL (or have that reorg happen in its own CL that lands before the logic change in this one). https://codereview.chromium.org/2682453002/diff/1/components/search_engines/t... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/1/components/search_engines/t... components/search_engines/template_url_service.cc:2048: // the OMNIBOX_API_EXTENSION. Nit: Might want to say "for engines of the same priority, the newly-added one wins." https://codereview.chromium.org/2682453002/diff/1/components/search_engines/t... components/search_engines/template_url_service.cc:2068: if (new_engine_lose) { Nit: If we just do this here: if (IsCreatedByExtension(existing_turl) && (!IsCreatedByExtension(template_url.get()) || (existing_turl->extension_info_->wants_to_be_default_engine && !template_url->extension_info_->wants_to_be_default_engine))) { ...then we can eliminate basically all the new code above. (But keep the comment at the top). Note that for better line wrapping, I renamed |existing_keyword_turl| to |existing_turl|, which doesn't seem less clear in context.
https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1490: auto turl_data = GenerateDummyTemplateURLData("ext2"); On 2017/02/07 at 00:42:45, Peter Kasting wrote: > Nit: "ext2" is pretty random, especially when the test doesn't use "ext" or "ext1". Just use "extension". Done https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1529: // Check that two extensions with same engine handled corrected. On 2017/02/07 at 00:42:45, Peter Kasting wrote: > Nit: handled corrected -> are handled correctly Done, thanks. https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1533: // TemplateURLData used for all extension. On 2017/02/07 at 00:42:45, Peter Kasting wrote: > Nit: extension -> extensions Done. https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1544: auto extension1 = model()->AddExtensionControlledTURL( On 2017/02/07 at 00:42:44, Peter Kasting wrote: > Nit: This should not use auto; it's a raw pointer (so it'd be auto*), and the type of the object is not object from the function call, so specify the type explicitly. (2 places) Done. https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1554: model()->AddExtensionControlledTURL(std::move(ext_dse), On 2017/02/07 at 00:42:45, Peter Kasting wrote: > Nit: Shouldn't this store the result in a temp |extension_2|, and we check that that's DSE below? (Possibly this would obviate the need for some of the existing checks below like the IsExtensionCOntrolledDefaultSearch() and FindTemplateURLForExtension() calls) Done. https://codereview.chromium.org/2682453002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_unittest.cc:1567: // Wait for any saves to finish. On 2017/02/07 at 00:42:45, Peter Kasting wrote: > Nit: Blank line above this Done https://codereview.chromium.org/2682453002/diff/1/components/search_engines/s... File components/search_engines/search_engines_test_util.h (right): https://codereview.chromium.org/2682453002/diff/1/components/search_engines/s... components/search_engines/search_engines_test_util.h:19: std::unique_ptr<TemplateURLData> GenerateDummyTemplateURLData( On 2017/02/07 at 00:42:45, Peter Kasting wrote: > Are you going to eliminate the identically-named function in default_search_manager_unittest.cc in this CL, or will you be doing that separately? > > Similar question for ExpectSimilar() and any other overlap between these files. > > If we're just condensing/moving functionality it would be clearer to see that in this CL (or have that reorg happen in its own CL that lands before the logic change in this one). Created https://codereview.chromium.org/2686453002 with test helpers extraction. https://codereview.chromium.org/2682453002/diff/1/components/search_engines/t... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/1/components/search_engines/t... components/search_engines/template_url_service.cc:2048: // the OMNIBOX_API_EXTENSION. On 2017/02/07 at 00:42:45, Peter Kasting wrote: > Nit: Might want to say "for engines of the same priority, the newly-added one wins." Done https://codereview.chromium.org/2682453002/diff/1/components/search_engines/t... components/search_engines/template_url_service.cc:2068: if (new_engine_lose) { On 2017/02/07 at 00:42:45, Peter Kasting wrote: > Nit: If we just do this here: > > if (IsCreatedByExtension(existing_turl) && > (!IsCreatedByExtension(template_url.get()) || > (existing_turl->extension_info_->wants_to_be_default_engine && > !template_url->extension_info_->wants_to_be_default_engine))) { > > ...then we can eliminate basically all the new code above. (But keep the comment at the top). > > Note that for better line wrapping, I renamed |existing_keyword_turl| to |existing_turl|, which doesn't seem less clear in context. Done, hope it would be clear to next generations :).
https://codereview.chromium.org/2682453002/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2049: // the OMNIBOX_API_EXTENSION. I'm confused by the algorithm because I thought we agreed on something different: Extension keywords are not replaced even between each other. The later one wins. Not sure why |wants_to_be_default_engine| matters. With the current approach you make FindMatchingExtensionTemplateURL from https://codereview.chromium.org/2639153002 more reliable but the failure is still possible (when you have two extension DSEs added in the reverse order here).
https://codereview.chromium.org/2682453002/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2049: // the OMNIBOX_API_EXTENSION. On 2017/02/07 at 16:27:21, vasilii wrote: > I'm confused by the algorithm because I thought we agreed on something different: Extension keywords are not replaced even between each other. The later one wins. Not sure why |wants_to_be_default_engine| matters. > I have understood differently, especially Peter's comment "Right, the conflict resolution algorithm here is clearly wrong. No extension keyword should be ever changed, and that goes double for an extension keyword set as default." It seems strange to me if non default extension could change keyword of default extension even if installed later. And what do you mean - "Extension keywords are not replaced even between each other." How its possible to have two extensions installed with same keywords? Do you mean to evict such engine from TemplateURLService? > With the current approach you make FindMatchingExtensionTemplateURL from https://codereview.chromium.org/2639153002 more reliable but the failure is still possible (when you have two extension DSEs added in the reverse order here). I am aware of it, as I already have written to Peter - "Also storing extension_id, IMHO, anyway is better and more clear way of choosing extension default search, because choosing engine by matching TemplateURLData can in exotic cases (2 extensions with same engines) match to engine from another extension or fail to match at all as I accidentally encountered in the case above." I will readily change my solution if you propose the algorithm that looks better to you.
It would be sorta nice if extensions "overlaid" the existing system, so that if you installed an extension, and then you uninstalled it, you were always guaranteed the system was as it was before you installed it to begin with. But this would require handling extension engines entirely differently, e.g. having a separate multimap of keyword-to-TemplateURL for installed extensions, and when we ask for "give me the engine for this keyword, we first look in that multimap and select the most-recently-installed matching extension (if any), otherwise fall back to the "real" map. Then extension keywords can never conflict with each other or with normal TemplateURLs. But I don't know whether it's necessary to do this. I'd like to hear vasilii's thoughts.
On 2017/02/07 at 20:36:53, pkasting wrote: > It would be sorta nice if extensions "overlaid" the existing system, so that if you installed an extension, and then you uninstalled it, you were always guaranteed the system was as it was before you installed it to begin with. > > But this would require handling extension engines entirely differently, e.g. having a separate multimap of keyword-to-TemplateURL for installed extensions, and when we ask for "give me the engine for this keyword, we first look in that multimap and select the most-recently-installed matching extension (if any), otherwise fall back to the "real" map. > > Then extension keywords can never conflict with each other or with normal TemplateURLs. > > But I don't know whether it's necessary to do this. I'd like to hear vasilii's thoughts. I like the whole idea - OMNIBOX_API_EXTENSION are already handled very differently and can have same keyword as normal engines. Also extensions engines are not synced and not persisted to DB, so the code for handling such cases would be simpler. We can create separate extension_template_urls_ vector and separate keyword to engine maps. Also there is question to what engine we will return for keyword if there is more than one candidate - latest installed, from extension or normal, from simple extension/default extension. What bothers me is that changes and impact would be large, current component code is very brittle, and I am afraid my original CL will be stalled for indefinite time :(. Can we somehow implement current, not so perfect solution and aftewards I could try to refactor whole extensions in TemplateURLService?
The algorithm for registering an extension keyword can be the following: - if there is no conflict - install. - if there is a conflict with a normal SE - hide it without obfuscating. Thus, deleting the extension will enable the previous SE without obfuscation. - if there if a conflict with an extension SE then the one installed later wins (you have AssociatedExtensionInfo::install_time). Your original CL isn't blocked on this because the source of flakiness was in SettingsOverridesAPI.
On 2017/02/08 at 10:49:30, vasilii wrote: > The algorithm for registering an extension keyword can be the following: > - if there is no conflict - install. > - if there is a conflict with a normal SE - hide it without obfuscating. Thus, deleting the extension will enable the previous SE without obfuscation. > - if there if a conflict with an extension SE then the one installed later wins (you have AssociatedExtensionInfo::install_time). > You describe the algorithm that is currently used for OMNIBOX_API_EXTENSION. It would not work for my CL, where we decided to store current default extension engine in pref - when we will try to match extension from pref to find current default engine we could choose later installed nondefault or could not match at all. 1. default extension is installed with keyword "keyword" it writes itself to default pref. 2. non default extension installed with keyword "keyword" later overrides keyword from extension installed on step 1 3. on TemplateURLService load we will try to find extension TemplateURL that matches data from default pref and could fail, because extension installed at step 1 will loose in conflict resolution and will have "keyword_" as keyword so we could not find it by matching. I propose to leave current approach from this CL which almost always will match default extension data from default pref to extension engine in TemplateURLService. Its not worse than previous solution IMHO, where latest added engine (normal or extensions) always won. Rewrite extensions support in TemplateURLService as Peter proposed above later. > Your original CL isn't blocked on this because the source of flakiness was in SettingsOverridesAPI. I had impression that extension keyword resolution must be fixed first from Peter comment: "> It sounds like you have found cases where the current keyword resolution algorithm doesn't do the right thing, so IMO, that should be fixed separately from, and in advance of, landing this change, at which point this could be landed without the changes to plumb the extension ID. > I realize that would take some time, but it seems like the original bug here is comparatively minor and taking a while to fix it would be OK. Maybe I'm misunderstanding the original bug." Also its blocked on https://codereview.chromium.org/2659353002/ which is needed so browsertests would pass. But that CL is moving better :).
You got my proposal wrong. Once more - an extension keyword is never modified. It may be hidden (=evicted from some map). But its TemplateURL is always in template_urls_ unmodified. Therefore, TemplateURLService::FindMatchingExtensionTemplateURL just works and the extension id isn't needed. Your original CL (https://chromium.googlesource.com/chromium/src/+/f5f1407adee2b18b59b33fb8d747...) didn't use the extension ID. But then you started digging into the unrelated problem (keyword conflicts), introduced the extension ID and got that comment from Peter. Once more, the reason for revert of https://codereview.chromium.org/2479113002 is unrelated to the keywords.
On 2017/02/08 at 12:36:10, vasilii wrote: > You got my proposal wrong. Once more - an extension keyword is never modified. It may be hidden (=evicted from some map). But its TemplateURL is always in template_urls_ unmodified. Therefore, TemplateURLService::FindMatchingExtensionTemplateURL just works and the extension id isn't needed. Thanks for clarifying, I missed that point, I' ll try to implement this approach, see if it works. > Your original CL (https://chromium.googlesource.com/chromium/src/+/f5f1407adee2b18b59b33fb8d747...) didn't use the extension ID. But then you started digging into the unrelated problem (keyword conflicts), introduced the extension ID and got that comment from Peter. > Once more, the reason for revert of https://codereview.chromium.org/2479113002 is unrelated to the keywords. Ok, good to hear, I will try to submit https://codereview.chromium.org/2479113002 as soon as https://codereview.chromium.org/2659353002/ will apply. What do you think of Peter proposal?
I'm not sure why the original https://codereview.chromium.org/2479113002 depends on https://codereview.chromium.org/2659353002/ Peter's proposal is good. I basically reiterated it above. The implementation can be different though.
On 2017/02/08 at 13:07:10, vasilii wrote: > I'm not sure why the original https://codereview.chromium.org/2479113002 depends on https://codereview.chromium.org/2659353002/ From https://codereview.chromium.org/2659353002/: "1. First and most important for me :) This CL is needed for my https://codereview.chromium.org/2639153002/ - "Make extensions DSE persistent in browser prefs" How I encountered this bug: when working on my CL for extensions provided search engine - I found that after my changes FindMatchingExtensionTemplateURL could not find extension installed engine from test extension. chrome/test/data/extensions/settings_override/manifest.json Test extension uses "prepopulated_id": 1 which corresponds to google engine so search_terms_replacement_key in extension engine has value kGoogleInstantExtendedEnabledKeyFull and is replaced inside TemplateURL constructor to google_util::kInstantExtendedAPIParam. After this replacement FindMatchingExtensionTemplateURL could not match TemplateURLData from extension DSE pref to extension engine stored in TemplateURLService. As a solution we could disable some extensions browser tests and hope that no one uses prepopulated_id=1 in extension manifest :) or rewrite FindMatchingExtensionTemplateURL to use extension_id for matching :) (sounds like this conversation already happened in another CL :))"
On 2017/02/08 13:27:31, Alexander Yashkin wrote: > On 2017/02/08 at 13:07:10, vasilii wrote: > > I'm not sure why the original https://codereview.chromium.org/2479113002 > depends on https://codereview.chromium.org/2659353002/ > > From https://codereview.chromium.org/2659353002/: > > "1. First and most important for me :) > This CL is needed for my https://codereview.chromium.org/2639153002/ - "Make > extensions DSE persistent in browser prefs" > How I encountered this bug: when working on my CL for extensions provided search > engine - I found that > after my changes FindMatchingExtensionTemplateURL could not find extension > installed engine from test extension. > chrome/test/data/extensions/settings_override/manifest.json > > Test extension uses "prepopulated_id": 1 which corresponds to google engine so > search_terms_replacement_key in extension engine has value > kGoogleInstantExtendedEnabledKeyFull and is replaced inside TemplateURL > constructor > to google_util::kInstantExtendedAPIParam. > After this replacement FindMatchingExtensionTemplateURL could not match > TemplateURLData > from extension DSE pref to extension engine stored in TemplateURLService. > > As a solution we could disable some extensions browser tests and hope that no > one uses prepopulated_id=1 in extension manifest :) > or rewrite FindMatchingExtensionTemplateURL to use extension_id for matching :) > (sounds like this conversation already happened in another CL :))" Got it. As "prepopulated_id": 1 is handled differently I think it's important that we test it and the test passes :-)
On 2017/02/09 at 13:57:35, vasilii wrote: > On 2017/02/08 13:27:31, Alexander Yashkin wrote: > > On 2017/02/08 at 13:07:10, vasilii wrote: > > > I'm not sure why the original https://codereview.chromium.org/2479113002 > > depends on https://codereview.chromium.org/2659353002/ > > > > From https://codereview.chromium.org/2659353002/: > > > > "1. First and most important for me :) > > This CL is needed for my https://codereview.chromium.org/2639153002/ - "Make > > extensions DSE persistent in browser prefs" > > How I encountered this bug: when working on my CL for extensions provided search > > engine - I found that > > after my changes FindMatchingExtensionTemplateURL could not find extension > > installed engine from test extension. > > chrome/test/data/extensions/settings_override/manifest.json > > > > Test extension uses "prepopulated_id": 1 which corresponds to google engine so > > search_terms_replacement_key in extension engine has value > > kGoogleInstantExtendedEnabledKeyFull and is replaced inside TemplateURL > > constructor > > to google_util::kInstantExtendedAPIParam. > > After this replacement FindMatchingExtensionTemplateURL could not match > > TemplateURLData > > from extension DSE pref to extension engine stored in TemplateURLService. > > > > As a solution we could disable some extensions browser tests and hope that no > > one uses prepopulated_id=1 in extension manifest :) > > or rewrite FindMatchingExtensionTemplateURL to use extension_id for matching :) > > (sounds like this conversation already happened in another CL :))" > > Got it. As "prepopulated_id": 1 is handled differently I think it's important that we test it and the test passes :-) Vasilii, I am implementing new keyword resolution, what about user-modified keywords in new ranking? Previous ranking was: user modified keyword > extension api keyword > replaceable keyword Currently I implemented in my draft code following ranking when choosing which keyword will win: extension installed search engine > user modified keyword > extension api keyword > replaceable keyword Is it valid approach or do you prefer to treat extension installed search engine on the same level of priority as extension api keyword?
Do you mean an omnibox extension (https://developer.chrome.com/extensions/omnibox)? I'd place like this omnibox extension -> extension search engines -> user modified keyword. The replaceable keyword are not in the list because they can be modified to avoid the conflict. I think the omnibox extension is higher priority because if the keyword is hidden then the extension is just useless. However, if it's simpler to put omnibox and search engine extensions on the same level then I don't mind.
Do you mean an omnibox extension (https://developer.chrome.com/extensions/omnibox)? I'd place like this omnibox extension -> extension search engines -> user modified keyword. The replaceable keyword are not in the list because they can be modified to avoid the conflict. I think the omnibox extension is higher priority because if the keyword is hidden then the extension is just useless. However, if it's simpler to put omnibox and search engine extensions on the same level then I don't mind.
On 2017/02/15 at 12:19:57, vasilii wrote: > Do you mean an omnibox extension (https://developer.chrome.com/extensions/omnibox)? Yes. > I'd place like this > omnibox extension -> extension search engines -> user modified keyword. > > The replaceable keyword are not in the list because they can be modified to avoid the conflict. > > I think the omnibox extension is higher priority because if the keyword is hidden then the extension is just useless. However, if it's simpler to put omnibox and search engine extensions on the same level then I don't mind. Previous ranking was: user modified keyword > extension api keyword > replaceable keyword New ranking will hide user modified keyword if any extension with same keyword is installed. I do not object, just wanted to make it clear. This change can be noticed by some users who can complain about "bugs in keywords".
On 2017/02/15 12:40:25, Alexander Yashkin wrote: > On 2017/02/15 at 12:19:57, vasilii wrote: > > Do you mean an omnibox extension > (https://developer.chrome.com/extensions/omnibox) > Yes. > > > I'd place like this > > omnibox extension -> extension search engines -> user modified keyword. > > > > The replaceable keyword are not in the list because they can be modified to > avoid the conflict. > > > > I think the omnibox extension is higher priority because if the keyword is > hidden then the extension is just useless. However, if it's simpler to put > omnibox and search engine extensions on the same level then I don't mind. > > Previous ranking was: > user modified keyword > extension api keyword > replaceable keyword > > New ranking will hide user modified keyword if any extension with same keyword > is installed. > I do not object, just wanted to make it clear. > This change can be noticed by some users who can complain about "bugs in > keywords". That's fine because a user can edit the keyword again or uninstall the extension. The extension search engines have never been editable. Uninstalling an extension instead of having it with 'keyword_' seems a better choice.
On 2017/02/15 at 12:45:39, vasilii wrote: > On 2017/02/15 12:40:25, Alexander Yashkin wrote: > > On 2017/02/15 at 12:19:57, vasilii wrote: > > > Do you mean an omnibox extension > > (https://developer.chrome.com/extensions/omnibox) > > Yes. > > > > > I'd place like this > > > omnibox extension -> extension search engines -> user modified keyword. > > > > > > The replaceable keyword are not in the list because they can be modified to > > avoid the conflict. > > > > > > I think the omnibox extension is higher priority because if the keyword is > > hidden then the extension is just useless. However, if it's simpler to put > > omnibox and search engine extensions on the same level then I don't mind. > > > > Previous ranking was: > > user modified keyword > extension api keyword > replaceable keyword > > > > New ranking will hide user modified keyword if any extension with same keyword > > is installed. > > I do not object, just wanted to make it clear. > > This change can be noticed by some users who can complain about "bugs in > > keywords". > > That's fine because a user can edit the keyword again or uninstall the extension. The extension search engines have never been editable. Uninstalling an extension instead of having it with 'keyword_' seems a better choice. So be it.
vasilii, PTAL.
On 2017/02/18 at 20:28:24, Alexander Yashkin wrote: > vasilii, PTAL. Peter, PTAL as well.
Mostly good https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:179: TemplateURL* AddExtensionSearchEngine(const std::string& keyword, A comment? https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:264: auto turl_data = GenerateDummyTemplateURLData(keyword); The explicit type improves readabilty here. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:489: TEST_F(TemplateURLServiceTest, AddExtensionKeyword) { I'd rename the test to AddOmniboxExtensionKeyword https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:524: base::Time::Now()); I see a source of flakiness here. base::Time::Now() may return the same as in #499 and then the outcome isn't clear. I'd prefer to be explicit. Create a local variable with base::Time::Now() in the begin of the test. Later use |time + FromDays(1)| where it's important. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:532: TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) { AddSameKeywordWithOmniboxExtensionPresent https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1686: // Check that default extension engine and user engines with same keywords are It checks the priority between the omnibox extension, search engine extension and user search engines. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1707: base::RunLoop().RunUntilIdle(); Why? https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1715: model()->RegisterOmniboxKeyword("omnibox_api", "extension_name", Can you use "extension_id" or similar as the first parameter? https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1770: AddExtensionSearchEngine("common_keyword", "extension_id2", true); I think here is the same flakiness as above regarding the time. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1783: base::RunLoop().RunUntilIdle(); Why? https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:1469: } else if (new_engine->type() == You don't need else as the previous block always returns something. https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:2076: if (CanReplace(existing_turl)) {} here and below. https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... components/search_engines/template_url_service.h:491: // When there are multiple extensions, the last-added wins. The comment leaves a question. Is it possible that BestEngineForKeyword(a, b) returns a different result from BestEngineForKeyword(b, a)?
https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:179: TemplateURL* AddExtensionSearchEngine(const std::string& keyword, On 2017/02/20 at 15:30:08, vasilii wrote: > A comment? Done. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:264: auto turl_data = GenerateDummyTemplateURLData(keyword); On 2017/02/20 at 15:30:08, vasilii wrote: > The explicit type improves readabilty here. Done. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:524: base::Time::Now()); On 2017/02/20 at 15:30:08, vasilii wrote: > I see a source of flakiness here. base::Time::Now() may return the same as in #499 and then the outcome isn't clear. > > I'd prefer to be explicit. Create a local variable with base::Time::Now() in the begin of the test. Later use |time + FromDays(1)| where it's important. Agree, changed to base::Time::FromDoubleT() in cases where time is compared. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1686: // Check that default extension engine and user engines with same keywords are On 2017/02/20 at 15:30:08, vasilii wrote: > It checks the priority between the omnibox extension, search engine extension and user search engines. Reworded. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1707: base::RunLoop().RunUntilIdle(); On 2017/02/20 at 15:30:08, vasilii wrote: > Why? Copied from another test. I think its unnecessary, so removed this call. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1715: model()->RegisterOmniboxKeyword("omnibox_api", "extension_name", On 2017/02/20 at 15:30:08, vasilii wrote: > Can you use "extension_id" or similar as the first parameter? Changed to "omnibox_api_extension_id". Did I understood you right? https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1770: AddExtensionSearchEngine("common_keyword", "extension_id2", true); On 2017/02/20 at 15:30:08, vasilii wrote: > I think here is the same flakiness as above regarding the time. Changed, now pass explicit time to AddExtensionSearchEngine. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1783: base::RunLoop().RunUntilIdle(); On 2017/02/20 at 15:30:08, vasilii wrote: > Why? Removed. https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:1469: } else if (new_engine->type() == On 2017/02/20 at 15:30:08, vasilii wrote: > You don't need else as the previous block always returns something. Done. https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:2076: if (CanReplace(existing_turl)) On 2017/02/20 at 15:30:08, vasilii wrote: > {} here and below. Done. https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2682453002/diff/100001/components/search_engi... components/search_engines/template_url_service.h:491: // When there are multiple extensions, the last-added wins. On 2017/02/20 at 15:30:08, vasilii wrote: > The comment leaves a question. Is it possible that BestEngineForKeyword(a, b) returns a different result from BestEngineForKeyword(b, a)? Changed comment and updated function. New variant expected to conform to BestEngineForKeyword(b, a) == BestEngineForKeyword(a, )
Description was changed from ========== Changed keywords conflicts resolution for extensions search engines. This is preparation CL for https://codereview.chromium.org/2639153002/. This CL changes logic of keywords conflicts resolution for extension controlled search engines. In previous logic last added keyword always won. New logic tries to preserve extension keywords over user keywords and default extensions keywords over non default. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org ========== to ========== Changed keywords conflicts resolution for extensions search engines. This is preparation CL for https://codereview.chromium.org/2639153002/. This CL changes logic of keywords conflicts resolution for extension controlled search engines. In previous logic last added keyword always won. New logic tries to preserve extension keywords over user keywords and default extensions keywords over non default. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org, jochen@chromium.org ==========
jochen@chromium.org, PTAL at omnibox_api.cc changes.
Description was changed from ========== Changed keywords conflicts resolution for extensions search engines. This is preparation CL for https://codereview.chromium.org/2639153002/. This CL changes logic of keywords conflicts resolution for extension controlled search engines. In previous logic last added keyword always won. New logic tries to preserve extension keywords over user keywords and default extensions keywords over non default. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org, jochen@chromium.org ========== to ========== Changed keywords conflicts resolution for extensions search engines. This is preparation CL for https://codereview.chromium.org/2639153002/. This CL changes logic of keywords conflicts resolution for extension controlled search engines. In previous logic last added keyword always won. New logic does not change extension keywords at all, they become hidden if more relevant keyword is installed. New priority of keywords is: omnibox api extension keywords > extension search engines > user engines. When there are multiple extensions of same type, the last-added wins. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org, jochen@chromium.org ==========
a-v-y@yandex-team.ru changed reviewers: + jochen@chromium.org
please make sure the CL description is wrapped at 80c omnibox_api.cc lgtm
Description was changed from ========== Changed keywords conflicts resolution for extensions search engines. This is preparation CL for https://codereview.chromium.org/2639153002/. This CL changes logic of keywords conflicts resolution for extension controlled search engines. In previous logic last added keyword always won. New logic does not change extension keywords at all, they become hidden if more relevant keyword is installed. New priority of keywords is: omnibox api extension keywords > extension search engines > user engines. When there are multiple extensions of same type, the last-added wins. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org, jochen@chromium.org ========== to ========== Changed keywords conflicts resolution for extensions search engines. This is preparation CL for https://codereview.chromium.org/2639153002/. This CL changes logic of keywords conflicts resolution for extension controlled search engines. In previous logic last added keyword always won. New logic does not change extension keywords at all, they become hidden if more relevant keyword is installed. New priority of keywords is: omnibox api extension keywords > extension search engines > user engines. When there are multiple extensions of same type, the last-added wins. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org, jochen@chromium.org ==========
https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:489: TEST_F(TemplateURLServiceTest, AddExtensionKeyword) { On 2017/02/20 15:30:08, vasilii wrote: > I'd rename the test to AddOmniboxExtensionKeyword If you do not address a comment you should explain why. https://codereview.chromium.org/2682453002/diff/120001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:543: "http://test2", base::Time::Now()); If we don't care about the time I'd use Time(). Same in NotPersistOmniboxExtensionKeyword. https://codereview.chromium.org/2682453002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1706: "common_keyword", "extension_id", true, base::Time::Now()); Again here we want to exclude the possibility that time matters when resolving a conflict between different types. I'd use an explicit timestamp here later than in #1720. https://codereview.chromium.org/2682453002/diff/120001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/120001/components/search_engi... components/search_engines/template_url_service.cc:1465: if (engine2->type() == engine1->type()) Add {}, same below for the multiline statements https://codereview.chromium.org/2682453002/diff/120001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2682453002/diff/120001/components/search_engi... components/search_engines/template_url_service.h:489: // by extension. One of arguments allowed to be nullptr, in this case another is allowed
https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:489: TEST_F(TemplateURLServiceTest, AddExtensionKeyword) { On 2017/02/21 13:17:51, vasilii wrote: > On 2017/02/20 15:30:08, vasilii wrote: > > I'd rename the test to AddOmniboxExtensionKeyword > > If you do not address a comment you should explain why. Sorry missed it. Where is a bug in codereview tool I think. I see this comment only if I turn on "Deprecated UI" setting in my account settings and switch to "View side-by-side diff with in-line comments" mode. If I turn off "Deprecated UI" or switch to "View unified diff" mode this comment and another one below with "AddSameKeywordWithOmniboxExtensionPresent" text is not shown at all. Thats very embarassing. Test name fixed. https://codereview.chromium.org/2682453002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:532: TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) { On 2017/02/20 15:30:08, vasilii wrote: > AddSameKeywordWithOmniboxExtensionPresent Done. https://codereview.chromium.org/2682453002/diff/120001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:543: "http://test2", base::Time::Now()); On 2017/02/21 at 13:17:51, vasilii wrote: > If we don't care about the time I'd use Time(). Same in NotPersistOmniboxExtensionKeyword. Done. https://codereview.chromium.org/2682453002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1706: "common_keyword", "extension_id", true, base::Time::Now()); On 2017/02/21 at 13:17:51, vasilii wrote: > Again here we want to exclude the possibility that time matters when resolving a conflict between different types. > I'd use an explicit timestamp here later than in #1720. Done. https://codereview.chromium.org/2682453002/diff/120001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/120001/components/search_engi... components/search_engines/template_url_service.cc:1465: if (engine2->type() == engine1->type()) On 2017/02/21 at 13:17:51, vasilii wrote: > Add {}, same below for the multiline statements Done. https://codereview.chromium.org/2682453002/diff/120001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2682453002/diff/120001/components/search_engi... components/search_engines/template_url_service.h:489: // by extension. One of arguments allowed to be nullptr, in this case another On 2017/02/21 at 13:17:51, vasilii wrote: > is allowed Done.
LGTM but wait for Peter's comments. There was a public holiday yesterday.
On 2017/02/21 at 14:11:58, vasilii wrote: > LGTM > > but wait for Peter's comments. There was a public holiday yesterday. Created https://bugs.chromium.org/p/chromium/issues/detail?id=694592 for codereview issue with comments.
I will try to get to this tomorrow.
LGTM I did not think deeply about the TemplateURLService to ask if there were any places you didn't touch that you should have; I assume you got them all. I also didn't think hard about ensuring that sync/saving to disk of extension-related keywords is handled correctly here (it seems like it should be). https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/omnibox/omnibox_api.cc:251: for (const auto i : pending_extensions_) { Nit: Should (I think) be const auto* https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:510: Time::FromDoubleT(2)); Nit: Will anything crash if we just pass Time() in this case and the next one? That avoids implying that these time values matter. https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:566: // keyword. It seems like the test no longer checks that the replaceable keyword has been removed (underneath the extension). We should probably use GetTemplateURLForGUID() or GetTemplateURLForHost() to check that as well. https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1762: // Check that two extensions with same engine are handled corrected. Nit: same engine -> the same keyword; corrected -> correctly https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:1464: // If both extensions are of same type, latest installed wins. Nit: The header already says this, so I'd remove this. I'd remove the comments below too. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:2058: // Check for conflicts between non extension engines keywords. Nit: Replace this with "If |template_url| is not created by an extension, its keyword must not conflict with any already in the model." Then follow my next two comments below. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:2061: // model. Nit: Remove this comment, and move the statement below down underneath the subsequent comment. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:2065: // Check whether |template_url|'s keyword conflicts with any already in the Nit: Remove this first sentence, which is now redundant with the comment above. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:2083: // Replace keyword for existing engine. Nit: "Neither engine can be replaced. Uniquify the existing keyword." Then move this comment up above the statement above it. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.h:492: // When there are multiple extensions, the last-added wins. Why do we let either argument be null? It doesn't make sense as a call (a null engine doesn't have the same keyword as another engine), and the two callers don't seem to need this (it doesn't make sense to me that either call site could actually pass a null pointer). I think we should ban this and DCHECK that the arguments are both non-null. Nit: Minor grammar issues; how about: Given two engines with the same keyword, returns which should take precedence. While normal engines must all have distinct keywords, extension-controlled and omnibox API engines may have the same keywords as each other or as normal engines. In these cases, omnibox API engines override extension-controlled engines, which override normal engines; if there is still a conflict after this, the most recently-added extension wins. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.h:720: // Finds the extension-supplied TemplateURL of type Nit: "Finds any NORMAL_CONTROLLED_BY_EXTENSION engine that matches..."
https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/omnibox/omnibox_api.cc:251: for (const auto i : pending_extensions_) { On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Nit: Should (I think) be const auto* Done. https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:510: Time::FromDoubleT(2)); On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Nit: Will anything crash if we just pass Time() in this case and the next one? That avoids implying that these time values matter. Removed FromDoubleT in this two cases, time value is relevant only when comparing two omnibox extensions keywords, in this test these are engines with keyword "keyword3". https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:566: // keyword. On 2017/02/25 at 04:26:53, Peter Kasting wrote: > It seems like the test no longer checks that the replaceable keyword has been removed (underneath the extension). We should probably use GetTemplateURLForGUID() or GetTemplateURLForHost() to check that as well. Done. https://codereview.chromium.org/2682453002/diff/140001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1762: // Check that two extensions with same engine are handled corrected. On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Nit: same engine -> the same keyword; corrected -> correctly Done, thanks. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:1464: // If both extensions are of same type, latest installed wins. On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Nit: The header already says this, so I'd remove this. I'd remove the comments below too. Easy, done. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:2058: // Check for conflicts between non extension engines keywords. On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Nit: Replace this with "If |template_url| is not created by an extension, its keyword must not conflict with any already in the model." > > Then follow my next two comments below. Done. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:2061: // model. On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Nit: Remove this comment, and move the statement below down underneath the subsequent comment. Done. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:2065: // Check whether |template_url|'s keyword conflicts with any already in the On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Nit: Remove this first sentence, which is now redundant with the comment above. Done. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:2083: // Replace keyword for existing engine. On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Nit: "Neither engine can be replaced. Uniquify the existing keyword." Then move this comment up above the statement above it. Done. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.h:492: // When there are multiple extensions, the last-added wins. On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Why do we let either argument be null? It doesn't make sense as a call (a null engine doesn't have the same keyword as another engine), and the two callers don't seem to need this (it doesn't make sense to me that either call site could actually pass a null pointer). > I have added nullptr support to args to support call from RemoveFromMaps: TemplateURL* best_fallback = nullptr; for (const auto& turl : template_urls_) { if ((turl.get() != template_url) && (turl->keyword() == keyword)) best_fallback = BestEngineForKeyword(best_fallback, turl.get()); Removed nullptr support, added check outside of BestEngineForKeyword call. > I think we should ban this and DCHECK that the arguments are both non-null. Done. > > Nit: Minor grammar issues; how about: > > Given two engines with the same keyword, returns which should take precedence. While normal engines must all have distinct keywords, extension-controlled and omnibox API engines may have the same keywords as each other or as normal engines. In these cases, omnibox API engines override extension-controlled engines, which override normal engines; if there is still a conflict after this, the most recently-added extension wins. Done, thanks. https://codereview.chromium.org/2682453002/diff/140001/components/search_engi... components/search_engines/template_url_service.h:720: // Finds the extension-supplied TemplateURL of type On 2017/02/25 at 04:26:53, Peter Kasting wrote: > Nit: "Finds any NORMAL_CONTROLLED_BY_EXTENSION engine that matches..." Done
The CQ bit was checked by a-v-y@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, pkasting@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2682453002/#ps160001 (title: "Fixed after review, round 5")
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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by a-v-y@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, pkasting@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2682453002/#ps180001 (title: "Fixed TemplateURLServiceTest.RepairPrepopulatedSearchEngines")
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_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by a-v-y@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, pkasting@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2682453002/#ps200001 (title: "Fixed auto variable type must not deduce to a raw pointer type")
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": 200001, "attempt_start_ts": 1488045180759840,
"parent_rev": "bab27699add969a853c0c73eb16a60d2fb1b178e", "commit_rev":
"f4eb4359fec12b0cd91dca5b0c8a9857bd55ef7c"}
Message was sent while issue was closed.
Description was changed from ========== Changed keywords conflicts resolution for extensions search engines. This is preparation CL for https://codereview.chromium.org/2639153002/. This CL changes logic of keywords conflicts resolution for extension controlled search engines. In previous logic last added keyword always won. New logic does not change extension keywords at all, they become hidden if more relevant keyword is installed. New priority of keywords is: omnibox api extension keywords > extension search engines > user engines. When there are multiple extensions of same type, the last-added wins. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org, jochen@chromium.org ========== to ========== Changed keywords conflicts resolution for extensions search engines. This is preparation CL for https://codereview.chromium.org/2639153002/. This CL changes logic of keywords conflicts resolution for extension controlled search engines. In previous logic last added keyword always won. New logic does not change extension keywords at all, they become hidden if more relevant keyword is installed. New priority of keywords is: omnibox api extension keywords > extension search engines > user engines. When there are multiple extensions of same type, the last-added wins. BUG=450534 R=pkasting@chromium.org, vasilii@chromium.org, jochen@chromium.org Review-Url: https://codereview.chromium.org/2682453002 Cr-Commit-Position: refs/heads/master@{#453100} Committed: https://chromium.googlesource.com/chromium/src/+/f4eb4359fec12b0cd91dca5b0c8a... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/f4eb4359fec12b0cd91dca5b0c8a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
