Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
426653002:
Import keywords for search engines imported from Firefox (Closed)
DescriptionThis patch parses search-metadata.json that is used to store
keywords for search engines in Firefox.
GetSearchEnginesXMLDataFromJSON() is modified to parse search-metadata.json
also and include keyword information in xml string.
BUG=52344
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290518
Patch Set 1 #
Total comments: 31
Patch Set 2 : Review comments #
Total comments: 10
Patch Set 3 : Review comment (track keyword in TemplateURLParsingContext) #
Total comments: 7
Patch Set 4 : Added test case #
Total comments: 16
Patch Set 5 : Review comment (modified test case) #Patch Set 6 : Review comment (modified test case, reduced search.json) #
Total comments: 8
Patch Set 7 : Review feedback (modified test cases - both new and old) #
Total comments: 14
Patch Set 8 : Review feedback (use const refs) #Patch Set 9 : Review feedback (expand KeywordInfo struct) #
Total comments: 15
Patch Set 10 : Review feedback (nit fixes) #
Total comments: 2
Patch Set 11 : Review feedback (two nit fixes) #
Messages
Total messages: 34 (0 generated)
PTAL at my prosposed solution to import keywords for search engines imported from Firefox (v3.5 onwards). If you've any review comments, then please let me know. I'll incorporate them. Thanks!
Thanks, Nikhil. Could you please add test coverage? Also, +pkasting for the changes to search_engines code -- I'm familiar with the import code in general, but not with the nitty gritty details of the search engine code. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:484: // file exists only if user has set keywords for search engines. nit: "exists only if user has set" -> "exists only if the user has set" (i.e. add "the") https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:489: search_metadata_available = true; nit: These three lines can be condensed to bool search_metadata_available = base::PathExists(search_metadata_json_file); https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:491: const base::DictionaryValue* search_metadata_root = NULL; nit: Please move this down a few lines, to be just above the if-stmt, so as to match the formatting of lines 503-507. (If you really prefer, you could instead update those lines to match your new formatting). https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:492: JSONFileValueSerializer metadata_serializer(search_metadata_json_file); Why do this if search_metadata_available is false? Alternately, if it's safe to do this when that's false, why have the search_metadata_available variable at all? https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:577: // it in xml string as <Alias> element and use this updated nit: "add it in xml string as <Alias> element" -> "add it to the XML string as an <Alias> element" https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:594: continue; IMO it would be better to simply update |file_data| here, and drop the other two lines, since they'll be handled by the natural flow of the loop. This also means that if the loop is ever updated, it's less likely that this code path would fail to be updated.
https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:491: const base::DictionaryValue* search_metadata_root = NULL; On 2014/07/28 22:18:45, Ilya Sherman wrote: > nit: Please move this down a few lines, to be just above the if-stmt, so as to > match the formatting of lines 503-507. (If you really prefer, you could instead > update those lines to match your new formatting). FWIW I prefer the line 503-507 formatting, as the style guide explicitly says to declare variables as close to their first use as possible. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:576: // If keyword is mentioned for this search engine, then add Nit: keyword -> a keyword https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:581: // Keyword is stored in field named as "alias". Nit: This comment is redundant. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:584: file_path, &search_xml_path)) { Nit: Indent 4, not 8 https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:588: // Add it as last child element. Nit: it -> the alias element; last -> the last https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... File components/search_engines/template_url_parser.cc (right): https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... components/search_engines/template_url_parser.cc:119: ALIAS Nit: Add trailing comma (means future additions won't have to touch this line) https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... components/search_engines/template_url_parser.cc:245: case TemplateURLParsingContext::SHORT_NAME: Nit: The order of cases in this switch seems random -- can you change it to match the enum order? https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... components/search_engines/template_url_parser.cc:308: // that case. Else, we use alias mentioned in XML file. Nit: Clearer and shorter: // Generate a keyword for this search engine if a custom one was not present // in the imported data. https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... components/search_engines/template_url_parser.cc:309: if (base::UTF16ToUTF8(data_.keyword()) == "dummy") I'm not a big fan of checking against "dummy", as in theory a user could have manually set a keyword to that. Can we instead actually track directly whether a keyword has been set in the TemplateURLParsingContext?
Thanks for taking a look at this. I've made changes as per review comments. PTAL. Also, I'll add the test case soon. Thanks! https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:484: // file exists only if user has set keywords for search engines. On 2014/07/28 22:18:45, Ilya Sherman wrote: > nit: "exists only if user has set" -> "exists only if the user has set" (i.e. > add "the") Done. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:489: search_metadata_available = true; On 2014/07/28 22:18:45, Ilya Sherman wrote: > nit: These three lines can be condensed to > > bool search_metadata_available = base::PathExists(search_metadata_json_file); I've removed this boolean as check for search_metadata_root is sufficient. I tested this by removing search-metadata.json as well. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:491: const base::DictionaryValue* search_metadata_root = NULL; On 2014/07/28 22:18:45, Ilya Sherman wrote: > nit: Please move this down a few lines, to be just above the if-stmt, so as to > match the formatting of lines 503-507. (If you really prefer, you could instead > update those lines to match your new formatting). Done. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:492: JSONFileValueSerializer metadata_serializer(search_metadata_json_file); On 2014/07/28 22:18:45, Ilya Sherman wrote: > Why do this if search_metadata_available is false? Alternately, if it's safe to > do this when that's false, why have the search_metadata_available variable at > all? Removed search_metadata_available as it's not required. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:576: // If keyword is mentioned for this search engine, then add On 2014/07/28 22:48:31, Peter Kasting wrote: > Nit: keyword -> a keyword Done. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:577: // it in xml string as <Alias> element and use this updated On 2014/07/28 22:18:45, Ilya Sherman wrote: > nit: "add it in xml string as <Alias> element" -> "add it to the XML string as > an <Alias> element" Done. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:581: // Keyword is stored in field named as "alias". On 2014/07/28 22:48:31, Peter Kasting wrote: > Nit: This comment is redundant. Done. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:584: file_path, &search_xml_path)) { On 2014/07/28 22:48:31, Peter Kasting wrote: > Nit: Indent 4, not 8 This was actually done by clang-format. I've now manually changed it to 4. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:588: // Add it as last child element. On 2014/07/28 22:48:31, Peter Kasting wrote: > Nit: it -> the alias element; last -> the last Done. https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:594: continue; On 2014/07/28 22:18:45, Ilya Sherman wrote: > IMO it would be better to simply update |file_data| here, and drop the other two > lines, since they'll be handled by the natural flow of the loop. This also > means that if the loop is ever updated, it's less likely that this code path > would fail to be updated. Done. https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... File components/search_engines/template_url_parser.cc (right): https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... components/search_engines/template_url_parser.cc:119: ALIAS On 2014/07/28 22:48:31, Peter Kasting wrote: > Nit: Add trailing comma (means future additions won't have to touch this line) Done. https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... components/search_engines/template_url_parser.cc:245: case TemplateURLParsingContext::SHORT_NAME: On 2014/07/28 22:48:31, Peter Kasting wrote: > Nit: The order of cases in this switch seems random -- can you change it to > match the enum order? Done. https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... components/search_engines/template_url_parser.cc:308: // that case. Else, we use alias mentioned in XML file. On 2014/07/28 22:48:31, Peter Kasting wrote: > Nit: Clearer and shorter: > > // Generate a keyword for this search engine if a custom one was not present > // in the imported data. Done. https://codereview.chromium.org/426653002/diff/1/components/search_engines/te... components/search_engines/template_url_parser.cc:309: if (base::UTF16ToUTF8(data_.keyword()) == "dummy") On 2014/07/28 22:48:31, Peter Kasting wrote: > I'm not a big fan of checking against "dummy", as in theory a user could have > manually set a keyword to that. Can we instead actually track directly whether > a keyword has been set in the TemplateURLParsingContext? Noted, it could very well be possible. To track this, I've introduced a boolean in TemplateURLData. I'm setting it while extracting keyword from <Alias> element. PTAL.
Thanks! This looks great, other than the missing tests :) https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:584: file_path, &search_xml_path)) { On 2014/07/29 08:29:21, Nikhil wrote: > On 2014/07/28 22:48:31, Peter Kasting wrote: > > Nit: Indent 4, not 8 > > This was actually done by clang-format. I've now manually changed it to 4. FWIW, I agree with clang-format, and disagree with Peter. IMO it's not worth locally overriding the clang-format output here, as the codebase is moving toward general clang-format conformance. However, if Peter chimes back in disagreeing with me, feel free to ignore my comment and go with his request -- it's not worth losing tons of time on this :) https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:576: const base::DictionaryValue* search_xml_path = NULL; Optional nit: It might be cleaner to move this variable up before line 575, and then combine the two if-stmts into one. https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:584: if (end_of_parent != std::string::npos) nit: You should still use curly braces, since the body of the if-stmt spans two lines of text. (That is, the constraint is visual, rather than having to do with what the compiler sees.) https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:590: // No keyword is mentioned for this search-engine. nit: This comment is no longer correct.
https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:584: file_path, &search_xml_path)) { On 2014/07/29 17:24:55, Ilya Sherman wrote: > On 2014/07/29 08:29:21, Nikhil wrote: > > On 2014/07/28 22:48:31, Peter Kasting wrote: > > > Nit: Indent 4, not 8 > > > > This was actually done by clang-format. I've now manually changed it to 4. > > FWIW, I agree with clang-format, and disagree with Peter. IMO it's not worth > locally overriding the clang-format output here, as the codebase is moving > toward general clang-format conformance. I'm not convinced the codebase is, in fact, moving that way :) > However, if Peter chimes back in > disagreeing with me, feel free to ignore my comment and go with his request -- > it's not worth losing tons of time on this :) This particular one has been debated in a few places and there's never really been consensus. The style I'm advocating is classically what Chrome did everywhere. The style here is not terribly unreasonable, so I don't care too much either way, but in general I think we should stay with tradition until we make an explicit decision to change it, and clang-format is not (yet) a reason to violate that. Do what you want. https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:586: "<Alias>" + alias + "</Alias> \n"); Why did you pick "alias" anyway? Wouldn't "keyword" make more sense? Does some browser already use "alias" to mean this? Is "keyword" already taken? https://codereview.chromium.org/426653002/diff/20001/components/search_engine... File components/search_engines/template_url_data.h (right): https://codereview.chromium.org/426653002/diff/20001/components/search_engine... components/search_engines/template_url_data.h:31: bool has_custom_keyword; I'd like to not introduce this to TemplateURLData. It doesn't make sense outside the very narrow context of the importer. Instead, track this in the TemplateURLParsingContext.
Made changes as per review comments. PTAL. Thanks! https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:576: const base::DictionaryValue* search_xml_path = NULL; On 2014/07/29 17:24:55, Ilya Sherman wrote: > Optional nit: It might be cleaner to move this variable up before line 575, and > then combine the two if-stmts into one. Done. https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:584: if (end_of_parent != std::string::npos) On 2014/07/29 17:24:56, Ilya Sherman wrote: > nit: You should still use curly braces, since the body of the if-stmt spans two > lines of text. (That is, the constraint is visual, rather than having to do > with what the compiler sees.) Done. https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:586: "<Alias>" + alias + "</Alias> \n"); On 2014/07/29 18:33:31, Peter Kasting wrote: > Why did you pick "alias" anyway? Wouldn't "keyword" make more sense? Does some > browser already use "alias" to mean this? Is "keyword" already taken? search-metadata.json uses "alias" as key to store keyword. I thought it would be consistent to use same as element in XML string. If you think "keyword" makes more sense, then please let me know and I'll update the patch. I've no preference, either one works for me. https://codereview.chromium.org/426653002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:590: // No keyword is mentioned for this search-engine. On 2014/07/29 17:24:56, Ilya Sherman wrote: > nit: This comment is no longer correct. Done. https://codereview.chromium.org/426653002/diff/20001/components/search_engine... File components/search_engines/template_url_data.h (right): https://codereview.chromium.org/426653002/diff/20001/components/search_engine... components/search_engines/template_url_data.h:31: bool has_custom_keyword; On 2014/07/29 18:33:31, Peter Kasting wrote: > I'd like to not introduce this to TemplateURLData. It doesn't make sense > outside the very narrow context of the importer. Instead, track this in the > TemplateURLParsingContext. Done.
Is it possible to write a unittest for this? If so, please do. LGTM otherwise. https://codereview.chromium.org/426653002/diff/40001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/40001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:584: if (end_of_parent != std::string::npos) { Nit: No {} https://codereview.chromium.org/426653002/diff/40001/components/search_engine... File components/search_engines/template_url_parser.cc (right): https://codereview.chromium.org/426653002/diff/40001/components/search_engine... components/search_engines/template_url_parser.cc:185: // If true, the user has mentioned keyword and we should use it. Otherwise, Nit: mentioned -> set an https://codereview.chromium.org/426653002/diff/40001/components/search_engine... components/search_engines/template_url_parser.cc:186: // we generate keyword based on URL. Nit: keyword -> a keyword; URL -> the URL
Yes, please do add test coverage for this code. Chrome code is frequently refactored, and tests are the tool that prevent large refactorings from accidentally breaking your code. (Tests also have lots of other nice benefits, but that's one of the most salient for the importer code, which frequently sees regressions in functionality.) https://codereview.chromium.org/426653002/diff/40001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/40001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:584: if (end_of_parent != std::string::npos) { On 2014/07/30 19:48:59, Peter Kasting wrote: > Nit: No {} (To clarify, the curly braces are no longer needed because the code now fits on a single line of text.)
First attempt at test case. PTAL. [1] I introduced new elements, since AddKeywords() is common, to avoid updating search.sqlite every time we add a new search engine or modify an existing search engine. This can probably be improved further. Please let me know WDYT. [2] I feel we can add more test cases after checking certain Firefox versions and corresponding JSON files. If it's alright then maybe we can do this in separate CL (If so, I'll take care of it). [3] Maybe I should remove ImportCustomKeywords test as it is exactly same as FirefoxImporter? https://codereview.chromium.org/426653002/diff/40001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/40001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:584: if (end_of_parent != std::string::npos) { On 2014/07/30 19:48:59, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/426653002/diff/40001/components/search_engine... File components/search_engines/template_url_parser.cc (right): https://codereview.chromium.org/426653002/diff/40001/components/search_engine... components/search_engines/template_url_parser.cc:185: // If true, the user has mentioned keyword and we should use it. Otherwise, On 2014/07/30 19:48:59, Peter Kasting wrote: > Nit: mentioned -> set an Done. https://codereview.chromium.org/426653002/diff/40001/components/search_engine... components/search_engines/template_url_parser.cc:186: // we generate keyword based on URL. On 2014/07/30 19:48:59, Peter Kasting wrote: > Nit: keyword -> a keyword; URL -> the URL Done.
I just noticed your CL says BUG=none. This should be associated with some bug. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:97: const KeywordInfo kFirefoxSearchEnginesWithCustomKeywords[] = { This looks like a duplicate of the list above plus one element. Don't do this. You can construct vectors at runtime based on appending one list to another if you want to do something like this. Or, if you follow my suggestion in the .json files to not add a new Twitter entry, you can verify that the same number of entries are present, but some of them have custom keywords set. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:132: has_custom_keyword_(false) {} This isn't a good variable name for this. This name says that the FirefoxObserver object has a custom keyword. It seems like instead you want to communicate that the observer should check that custom keywords are set properly, or something. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:209: for (size_t j = 0; j < arraysize(kFirefoxKeywords); ++j) { Indenting (unless Rietveld isn't properly showing the diff) https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:219: for (size_t j = 0; Again, don't copy this code. Find a way to write just a single loop here. For example, you could pass in the vector of keywords to compare against. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:241: void SetHasCustomKeyword(bool has_custom_keyword) { Cheap inline accessors should be named unix_hacker()-style. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:356: MAYBE_IMPORTER(ImportCustomKeywords)) { It's not clear why you added a test that is identical to the previous test. What are you trying to test in each case? https://codereview.chromium.org/426653002/diff/80001/chrome/test/data/firefox... File chrome/test/data/firefox_profile/search.json (right): https://codereview.chromium.org/426653002/diff/80001/chrome/test/data/firefox... chrome/test/data/firefox_profile/search.json:77: "_id":"./chrome/test/data/firefox_searchplugins/default/twitter.xml", Any particular reason to add this? I'm not seeing a way that it differs from the entries above and thus would lead to greater test coverage. Why can't you just add an alias in search-metadata.json for one of the existing entries, instead of adding this new one so you can add to it? https://codereview.chromium.org/426653002/diff/80001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/80001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:584: if (end_of_parent != std::string::npos && !alias.empty()) Make sure you have a testcase with an empty alias.
I've slightly changed the approach for test case. PTAL. Also, I identified this issue while checking Issue 52344. I've mentioned same in "Bug=" but if you think separate bug should be created for this then please let me know. Thanks! https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:97: const KeywordInfo kFirefoxSearchEnginesWithCustomKeywords[] = { On 2014/07/31 18:28:21, Peter Kasting wrote: > This looks like a duplicate of the list above plus one element. Don't do this. > You can construct vectors at runtime based on appending one list to another if > you want to do something like this. Or, if you follow my suggestion in the > .json files to not add a new Twitter entry, you can verify that the same number > of entries are present, but some of them have custom keywords set. I've now slightly changed the approach based on your suggestion. PTAL. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:132: has_custom_keyword_(false) {} On 2014/07/31 18:28:21, Peter Kasting wrote: > This isn't a good variable name for this. This name says that the > FirefoxObserver object has a custom keyword. It seems like instead you want to > communicate that the observer should check that custom keywords are set > properly, or something. Done. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:209: for (size_t j = 0; j < arraysize(kFirefoxKeywords); ++j) { On 2014/07/31 18:28:21, Peter Kasting wrote: > Indenting (unless Rietveld isn't properly showing the diff) Done. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:219: for (size_t j = 0; On 2014/07/31 18:28:21, Peter Kasting wrote: > Again, don't copy this code. Find a way to write just a single loop here. For > example, you could pass in the vector of keywords to compare against. Done. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:241: void SetHasCustomKeyword(bool has_custom_keyword) { On 2014/07/31 18:28:21, Peter Kasting wrote: > Cheap inline accessors should be named unix_hacker()-style. Done. https://codereview.chromium.org/426653002/diff/80001/chrome/browser/importer/... chrome/browser/importer/firefox_importer_browsertest.cc:356: MAYBE_IMPORTER(ImportCustomKeywords)) { On 2014/07/31 18:28:21, Peter Kasting wrote: > It's not clear why you added a test that is identical to the previous test. > What are you trying to test in each case? Removed it. https://codereview.chromium.org/426653002/diff/80001/chrome/test/data/firefox... File chrome/test/data/firefox_profile/search.json (right): https://codereview.chromium.org/426653002/diff/80001/chrome/test/data/firefox... chrome/test/data/firefox_profile/search.json:77: "_id":"./chrome/test/data/firefox_searchplugins/default/twitter.xml", On 2014/07/31 18:28:21, Peter Kasting wrote: > Any particular reason to add this? I'm not seeing a way that it differs from > the entries above and thus would lead to greater test coverage. Why can't you > just add an alias in search-metadata.json for one of the existing entries, > instead of adding this new one so you can add to it? I added new entry in .json files since tests for older versions of Firefox compared against keywords set in kFirefoxKeywords. I couldn't get all tests to pass if I changed an existing entry. But I've now slightly changed the approach based on your suggestion. PTAL. https://codereview.chromium.org/426653002/diff/80001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/426653002/diff/80001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:584: if (end_of_parent != std::string::npos && !alias.empty()) On 2014/07/31 18:28:21, Peter Kasting wrote: > Make sure you have a testcase with an empty alias. Done.
This whole test implementation is messy and fragile. There are ways to fix it, but it's not worth trying to go through the time to explain them. Just use two totally different sets of profile data (as opposed to a single set which, in the newer profile version, overrides some entries to have custom keywords) to test the different importer versions. Then the test can be exactly like it was before, with initializer lists of the expected data, but the lists can be local to the testcases that test the different versions of importing, and they won't duplicate each other's data. You can also make the data lists much shorter -- just enough to test the various different control flow branches in the import code. That way your code won't have ginormous long initializer lists.
On 2014/08/04 21:36:12, Peter Kasting wrote: > This whole test implementation is messy and fragile. There are ways to fix it, > but it's not worth trying to go through the time to explain them. > > Just use two totally different sets of profile data (as opposed to a single set > which, in the newer profile version, overrides some entries to have custom > keywords) to test the different importer versions. Then the test can be exactly > like it was before, with initializer lists of the expected data, but the lists > can be local to the testcases that test the different versions of importing, and > they won't duplicate each other's data. You can also make the data lists much > shorter -- just enough to test the various different control flow branches in > the import code. That way your code won't have ginormous long initializer > lists. I completely agree that the implementation is messy. I'll try to separate out the profile data and then update the patch.
pkasting@ isherman@ - PTAL. This test implementation is still not perfect, but if you think approach looks okay then please let me know and I'll update other tests accordingly. [*] I've reduced initializer list for current Firefox version as it uses search.json. But same cannot be done for older versions unless we change search.sqlite binary currently being used for tests (it has all 11 entries, some of which are not required). PTAL, and let me know review comments. I'll incorporate them. Thanks!
You're welcome to change search.sqlite if doing so would be useful. https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:146: for (size_t i = 0; i < template_urls.size(); ++i) { Nit: Prefer iterators to indexes when the actual index value isn't used elswhere (3 places) https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:149: item.url = template_urls[i]->url(); You added a 2-arg constructor for KeywordInfo, so why not use it here? https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:281: std::vector<KeywordInfo*> expected_data; Why does this take objects by pointer instead of value? This results in the test leaking all added objects. https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:304: expected_data.clear(); This statement isn't necessary.
Modified test case as per review comments. PTAL. I verified browser tests with gtest_filter=FirefoxProfileImporterBrowserTest.* and all tests passed. Thanks! https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:146: for (size_t i = 0; i < template_urls.size(); ++i) { On 2014/08/11 22:25:05, Peter Kasting wrote: > Nit: Prefer iterators to indexes when the actual index value isn't used elswhere > (3 places) Done. https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:149: item.url = template_urls[i]->url(); On 2014/08/11 22:25:05, Peter Kasting wrote: > You added a 2-arg constructor for KeywordInfo, so why not use it here? Done. https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:281: std::vector<KeywordInfo*> expected_data; On 2014/08/11 22:25:05, Peter Kasting wrote: > Why does this take objects by pointer instead of value? This results in the > test leaking all added objects. Done. https://codereview.chromium.org/426653002/diff/120001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:304: expected_data.clear(); On 2014/08/11 22:25:05, Peter Kasting wrote: > This statement isn't necessary. Done.
Why don't we just have the KeywordInfo object contain both the <4.0 and >=4.0 keywords, and have the verify function take an argument about which field to check? Then we could make all these functions use the same list of search engines, which would return us to only needing to specify the whole list once in a constant array. I'm sorry if you feel like this is going in circles, but it seems like every iteration keeps resulting in massive data/code duplication. We shouldn't be listing the same data more than once in the file. https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:49: KeywordInfo(std::string keyword, std::string url) Nit: Const refs https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:51: KeywordInfo() {} Nit: Is this constructor necessary? Maybe for the compiler to instantiate vector<KeywordInfo>? If not, remove. https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:144: virtual void AddKeywords(ScopedVector<TemplateURL> template_urls, Nit: While here: Make this a const ref (requires changing the parent class) https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:187: void VerifyImportedSearchEngines(std::vector<KeywordInfo>& expected_data) { Nit: const arg https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:188: EXPECT_EQ(expected_data.size(), search_list.size()); Nit: (expected, actual) https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:360: // Expected data for search engine list and keywords. Nit: These comments don't seem very necessary (and aren't present above), I'd remove them.
https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:144: virtual void AddKeywords(ScopedVector<TemplateURL> template_urls, On 2014/08/12 18:16:45, Peter Kasting wrote: > Nit: While here: Make this a const ref (requires changing the parent class) Wait, don't do that. This is trying to pass ownership, changing to a const ref wouldn't be correct.
https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:49: KeywordInfo(std::string keyword, std::string url) On 2014/08/12 18:16:45, Peter Kasting wrote: > Nit: Const refs Done. https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:51: KeywordInfo() {} On 2014/08/12 18:16:45, Peter Kasting wrote: > Nit: Is this constructor necessary? Maybe for the compiler to instantiate > vector<KeywordInfo>? If not, remove. Done. https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:144: virtual void AddKeywords(ScopedVector<TemplateURL> template_urls, On 2014/08/12 18:23:06, Peter Kasting wrote: > On 2014/08/12 18:16:45, Peter Kasting wrote: > > Nit: While here: Make this a const ref (requires changing the parent class) > > Wait, don't do that. This is trying to pass ownership, changing to a const ref > wouldn't be correct. Acknowledged. https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:187: void VerifyImportedSearchEngines(std::vector<KeywordInfo>& expected_data) { On 2014/08/12 18:16:45, Peter Kasting wrote: > Nit: const arg Done. https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:188: EXPECT_EQ(expected_data.size(), search_list.size()); On 2014/08/12 18:16:45, Peter Kasting wrote: > Nit: (expected, actual) Are you suggesting to change variable name for vectors? expected_data is initializer list against whom search_list is compared. search_list is list of imported search engines. So, (expected, actual) is maintained for comparison. https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:360: // Expected data for search engine list and keywords. On 2014/08/12 18:16:45, Peter Kasting wrote: > Nit: These comments don't seem very necessary (and aren't present above), I'd > remove them. Done.
This hasn't yet addressed the primary complaint I made about the previous patch. https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:188: EXPECT_EQ(expected_data.size(), search_list.size()); On 2014/08/13 05:52:41, Nikhil wrote: > On 2014/08/12 18:16:45, Peter Kasting wrote: > > Nit: (expected, actual) > > Are you suggesting to change variable name for vectors? expected_data is > initializer list against whom search_list is compared. search_list is list of > imported search engines. So, (expected, actual) is maintained for comparison. OK.
On 2014/08/12 18:16:46, Peter Kasting wrote: > Why don't we just have the KeywordInfo object contain both the <4.0 and >=4.0 > keywords, and have the verify function take an argument about which field to > check? > > Then we could make all these functions use the same list of search engines, > which would return us to only needing to specify the whole list once in a > constant array. > I'm sorry but I don't fully understand this suggestion. Could you please explain more? The only difference between the lists is of keyword for one of the search engines. Maybe we can - [1] Update search.sqlite to have fewer entries. [2] Maintain vector of expected keywords and associated urls in constant array. Use this in verify function for older versions that use search.sqlite. [3] Introduce new entry in search.json and search-metadata.json. [4] Append this new entry to vector of [2] in FirefoxProfileImporterBrowserTest.FirefoxImporter. Use updated list in verify function. Still not a very clean implementation, but avoids code duplication.
On 2014/08/13 06:21:07, pkasting (away thru Aug 20) wrote: > This hasn't yet addressed the primary complaint I made about the previous patch. > Yes, I was just writing my response to it. I thought I'll discuss first since I wasn't very clear on what to do, and pushed the patch _assuming_ timezone difference. Didn't mean to ignore it. > https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... > File chrome/browser/importer/firefox_importer_browsertest.cc (right): > > https://codereview.chromium.org/426653002/diff/140001/chrome/browser/importer... > chrome/browser/importer/firefox_importer_browsertest.cc:188: > EXPECT_EQ(expected_data.size(), search_list.size()); > On 2014/08/13 05:52:41, Nikhil wrote: > > On 2014/08/12 18:16:45, Peter Kasting wrote: > > > Nit: (expected, actual) > > > > Are you suggesting to change variable name for vectors? expected_data is > > initializer list against whom search_list is compared. search_list is list of > > imported search engines. So, (expected, actual) is maintained for comparison. > > OK.
I'm suggesting we drop the vector idea entirely, go back to something like the original test code, and simply add a third field to each entry that's the "keyword for Firefox 4+", which would be the same as the existing keyword in most (but not all) cases. Then the pre-existing verify function can just take an extra arg that specifies which of these two keywords to pay attention to.
Introduced third field in KeywordInfo struct and modified the test cases accordingly. PTAL. Thanks!
LGTM https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:179: (is_sqlite_version_) Nit: Don't parenthesize an expression with only one token. https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:180: ? base::WideToUTF16(kFirefoxKeywords[j].keyword_in_sqlite) Nit: clang-format error: Wrap after '?' (and if necessary after ':'), not before You can also shorten things a bit by putting the ternary expression inside the WideToUTF16() call. https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:182: if (expected_keyword == imported_keyword) { Nit: Maybe it's just me, but I expected this to read "if (imported_keyword == expected_keyword)" https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:197: void set_sqlite_version(bool is_sqlite_version) { Nit: Instead of adding a member function to set this, pass it as a constructor arg. https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:212: bool is_sqlite_version_; Nit: How about this: // Newer versions of Firefox can store custom keyword names in json, which // override the sqlite values. To be able to test both older and newer // versions, tests set this variable to indicate whether to expect the // |keyword_in_sqlite| or |keyword_in_json| values from the reference data. bool use_keyword_in_json_;
LGTM with Peter's comments addressed. Thanks! https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:178: const base::string16& expected_keyword = nit: Hmm, you're defining a const-reference to an object returned by value from base::WideToUTF16. I'm a little surprised that this compiles. I'd drop the reference part at least for clarity.
https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:178: const base::string16& expected_keyword = On 2014/08/13 20:14:17, Ilya Sherman wrote: > nit: Hmm, you're defining a const-reference to an object returned by value from > base::WideToUTF16. I'm a little surprised that this compiles. I'd drop the > reference part at least for clarity. FWIW, apparently this is indeed explicitly legal in C++: [ http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-c... ]. I'm not actually sure whether it's considered good style or not, but I found it surprising.
This looks ready now :) PTAL. Thanks! https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:178: const base::string16& expected_keyword = On 2014/08/13 20:18:43, Ilya Sherman wrote: > On 2014/08/13 20:14:17, Ilya Sherman wrote: > > nit: Hmm, you're defining a const-reference to an object returned by value > from > > base::WideToUTF16. I'm a little surprised that this compiles. I'd drop the > > reference part at least for clarity. > > FWIW, apparently this is indeed explicitly legal in C++: [ > http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-c... > ]. I'm not actually sure whether it's considered good style or not, but I found > it surprising. Dropped reference. https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:179: (is_sqlite_version_) On 2014/08/13 17:32:01, pkasting (away thru Aug 20) wrote: > Nit: Don't parenthesize an expression with only one token. Done. https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:180: ? base::WideToUTF16(kFirefoxKeywords[j].keyword_in_sqlite) On 2014/08/13 17:32:01, pkasting (away thru Aug 20) wrote: > Nit: clang-format error: Wrap after '?' (and if necessary after ':'), not before > > You can also shorten things a bit by putting the ternary expression inside the > WideToUTF16() call. Done. https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:182: if (expected_keyword == imported_keyword) { On 2014/08/13 17:32:01, pkasting (away thru Aug 20) wrote: > Nit: Maybe it's just me, but I expected this to read "if (imported_keyword == > expected_keyword)" I've seen constants being used on left side of comparion in codebase. But I can change this, if required. https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:197: void set_sqlite_version(bool is_sqlite_version) { On 2014/08/13 17:32:01, pkasting (away thru Aug 20) wrote: > Nit: Instead of adding a member function to set this, pass it as a constructor > arg. Done. https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:212: bool is_sqlite_version_; On 2014/08/13 17:32:01, pkasting (away thru Aug 20) wrote: > Nit: How about this: > > // Newer versions of Firefox can store custom keyword names in json, which > // override the sqlite values. To be able to test both older and newer > // versions, tests set this variable to indicate whether to expect the > // |keyword_in_sqlite| or |keyword_in_json| values from the reference data. > bool use_keyword_in_json_; Done.
LGTM % a final two nits. Thanks! (Apologies for the delay -- I hadn't realized you were waiting on further review from us.) https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:182: if (expected_keyword == imported_keyword) { On 2014/08/14 09:13:08, Nikhil wrote: > On 2014/08/13 17:32:01, pkasting (away thru Aug 20) wrote: > > Nit: Maybe it's just me, but I expected this to read "if (imported_keyword == > > expected_keyword)" > > I've seen constants being used on left side of comparion in codebase. But I can > change this, if required. Constants on the left is an older style, which was used to work around insufficiently good compiler warnings. Constants on the right is more common now. https://codereview.chromium.org/426653002/diff/200001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/200001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:180: kFirefoxKeywords[j].keyword_in_sqlite); nit: Unless this is how clang-format formatted these lines (which would surprise me), please either run clang-format or format the lines like so: const base::string16 expected_keyword = base::WideToUTF16( use_keyword_in_json_ ? kFirefoxKeywords[j].keyword_in_json : kFirefoxKeywords[j].keyword_in_sqlite);
Final two nit fixes addressed! Thanks Peter and Ilya for reviewing this. I'm going to try to submit this now :) https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/180001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:182: if (expected_keyword == imported_keyword) { On 2014/08/19 00:13:28, Ilya Sherman wrote: > On 2014/08/14 09:13:08, Nikhil wrote: > > On 2014/08/13 17:32:01, pkasting (away thru Aug 20) wrote: > > > Nit: Maybe it's just me, but I expected this to read "if (imported_keyword > == > > > expected_keyword)" > > > > I've seen constants being used on left side of comparion in codebase. But I > can > > change this, if required. > > Constants on the left is an older style, which was used to work around > insufficiently good compiler warnings. Constants on the right is more common > now. Done. https://codereview.chromium.org/426653002/diff/200001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/426653002/diff/200001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:180: kFirefoxKeywords[j].keyword_in_sqlite); On 2014/08/19 00:13:28, Ilya Sherman wrote: > nit: Unless this is how clang-format formatted these lines (which would surprise > me), please either run clang-format or format the lines like so: > > const base::string16 expected_keyword = base::WideToUTF16( > use_keyword_in_json_ ? > kFirefoxKeywords[j].keyword_in_json : > kFirefoxKeywords[j].keyword_in_sqlite); clang-format wrapped before ? and :, which was incorrect. I made the change manually to wrap after. But missed the indentation. I've now fixed it as per your suggestion. Thanks for catching this.
The CQ bit was checked by n.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/n.bansal@samsung.com/426653002/220001
Message was sent while issue was closed.
Committed patchset #11 (220001) as 290518 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
