Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
616763002:
Importing certain bookmarks from firefox and HTML file as search engines. (Closed)
|
Created:
6 years, 2 months ago by Tapu Ghose Modified:
5 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImporting certain bookmarks from firefox and HTML file as search engines.
BUG=52344
Committed: https://crrev.com/4183aa2da39c20f9e49f7e9bd51f08544c36af83
Cr-Commit-Position: refs/heads/master@{#311834}
Patch Set 1 #
Total comments: 54
Patch Set 2 : Addressed comments #
Total comments: 45
Patch Set 3 : Renamed URLKeywordInfo to SearchEngineInfo and addressed comments. #Patch Set 4 : Replacing custom code by existing function for checking if an url supports replacement terms. #
Total comments: 42
Patch Set 5 : Addressed feedback. #
Total comments: 36
Patch Set 6 : Addressed more feedback. #
Total comments: 12
Patch Set 7 : Handled potential NULL pointer exception that could have resulted from CreateTemplateURL. #
Total comments: 18
Patch Set 8 : Skipped autogenerating keyword for bookmark import. #Patch Set 9 : Removed keyword autogeneration logic from CreateTemplateURL. #
Total comments: 6
Patch Set 10 : Minor changes. #
Total comments: 8
Patch Set 11 : Updated repo and made necessary changes to IE importer. #Patch Set 12 : Re-ordered dependency list in GN based on GYP. #
Total comments: 2
Patch Set 13 : Addressed dependency issue. #Messages
Total messages: 78 (15 generated)
ghose.tapu@gmail.com changed reviewers: + isherman@chromium.org
ghose.tapu@gmail.com changed reviewers: + pkasting@chromium.org
Added code to import bookmark from firefox/HTML files as search engines if 1) url of the bookmark is invalid 2) url contains replacement term and 3) resulted url after substituting the replacement term is a valid one.
https://codereview.chromium.org/616763002/diff/1/chrome/browser/importer/in_p... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/browser/importer/in_p... chrome/browser/importer/in_process_importer_bridge.cc:106: // imported as search engine. |title| and |raw_url| may be empty. nit: "as search engine" -> "as a search engine" https://codereview.chromium.org/616763002/diff/1/chrome/browser/importer/in_p... chrome/browser/importer/in_process_importer_bridge.cc:111: const base::string16& raw_url) { Peter understands the TemplateURL code better than I do; but my guess is that it would be better to pass in a single parameter to this method, rather than passing both a |url| and a |raw_url|. That would, I think, make the code easier to understand. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:164: || CanImportURLAsSearchEngine(url, &decoded_url))) { nit: The "||" should be on the previous line. ("git cl format" can help you find and fix formatting errors) https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:270: url::RawCanonOutputW<1024> canon_output; Hmm, how do you know that 1024 characters is enough? Can you use an automatically resizing class instead? https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:288: } Please use a utility function from base/strings/string_util.h rather than writing your own version here. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:41: // |url_keywords| is a pointer toa vector, which is filled with the imported nit: "toa" -> "to a" https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:57: // the resulted url is a valid one. nit: "resulted" -> "resulting" https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader_unittest.cc:336: } // namespace bookmark_html_reader Please add test coverage for your changes. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmarks_file_importer_unittest.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmarks_file_importer_unittest.cc:14: } // namespace namespace bookmark_html_reader nit: Why is this needed? Can you #include the appropriate header instead? https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmarks_file_importer_unittest.cc:22: nit: Please revert this diff. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmarks_file_importer_unittest.cc:48: internal::CanImportURL(GURL(test_cases[i].url))); nit: Please revert this diff. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmarks_file_importer_unittest.cc:66: } This test code belongs in the bookmark_html_reader_unittest.cc file. Also, you should test the public API, rather than testing just this helper method. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:272: } Please add test coverage for these changes
https://codereview.chromium.org/616763002/diff/1/chrome/browser/importer/in_p... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/browser/importer/in_p... chrome/browser/importer/in_process_importer_bridge.cc:111: const base::string16& raw_url) { On 2014/09/30 20:54:12, Ilya Sherman wrote: > Peter understands the TemplateURL code better than I do; but my guess is that it > would be better to pass in a single parameter to this method, rather than > passing both a |url| and a |raw_url|. That would, I think, make the code easier > to understand. I agree. The caller should simply not call this if we don't want to create a TURL. https://codereview.chromium.org/616763002/diff/1/chrome/common/importer/impor... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/1/chrome/common/importer/impor... chrome/common/importer/importer_data_types.h:55: base::string16 raw_url; // holds raw url when GURL contains invalid url. As earlier, I think it would be better to only have one of (url, raw_url). https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:173: if (!shortcut.empty()) { Doesn't this mean we'll also import non-replaceable bookmarked URLs with keywords as search engines? That doesn't seem right. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:263: // If |url| is valid then we are not supposed to be here. That implies callers shouldn't be calling us. I don't think that's actually what you mean. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:270: url::RawCanonOutputW<1024> canon_output; On 2014/09/30 20:54:12, Ilya Sherman wrote: > Hmm, how do you know that 1024 characters is enough? It's not necessarily enough. This is almost certainly an exploitable security hole. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:271: url::DecodeURLEscapeSequences(raw_url.c_str(), raw_url.size(), &canon_output); Why can't we use the higher-level unescaping functions? Using the raw canonicalizer functions is both cumbersome and dangerous. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:288: } On 2014/09/30 20:54:12, Ilya Sherman wrote: > Please use a utility function from base/strings/string_util.h rather than > writing your own version here. Furthermore, there's already code in Chrome that attempts to determine if some string is a valid, replaceable URL for TemplateURL purposes. This whole function should be using that code, not reimplementing it. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:51: std::vector<importer::URLKeywordInfo>* url_keywords, "URL keyword" seems like a rather confusing name. Why not call the type and variable here something about "template URLs" or "search engines"? https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:55: // engine. Scan |url| to check if there is any replacement term, %s. If yes then Nit: Scan -> scans, substitute -> substitutes. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:56: // substitute the replacement term(s) with any arbitrary value. Returns true if What does "with any arbitrary value" mean? Surely it uses some particular value. Did you mean "an" instead of "any"? https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:260: // Import invalid bookmark's keyword as search engine. How is the functionality from this file different than in bookmark_html_reader.cc:ImportBookmarksFile()? Why do we have this code twice? We should only have one implementation of this sort of thing.
PTAL https://codereview.chromium.org/616763002/diff/1/chrome/browser/importer/in_p... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/browser/importer/in_p... chrome/browser/importer/in_process_importer_bridge.cc:106: // imported as search engine. |title| and |raw_url| may be empty. On 2014/09/30 20:54:12, Ilya Sherman wrote: > nit: "as search engine" -> "as a search engine" Done. https://codereview.chromium.org/616763002/diff/1/chrome/browser/importer/in_p... chrome/browser/importer/in_process_importer_bridge.cc:111: const base::string16& raw_url) { On 2014/09/30 20:54:12, Ilya Sherman wrote: > Peter understands the TemplateURL code better than I do; but my guess is that it > would be better to pass in a single parameter to this method, rather than > passing both a |url| and a |raw_url|. That would, I think, make the code easier > to understand. In that case I would use |raw_url| and get rid of |url|. Reason: When the execution reaches this particular chunk of codes, both url.spec() and url.possibly_invalid_spec() returns empty (if url is invalid). Consequently, it becomes troublesome while passing parameter to SetURL function at line 126. In addition, we can create a GURL object by using |raw_url|, if needed. https://codereview.chromium.org/616763002/diff/1/chrome/browser/importer/in_p... chrome/browser/importer/in_process_importer_bridge.cc:111: const base::string16& raw_url) { On 2014/10/01 01:02:54, Peter Kasting wrote: > On 2014/09/30 20:54:12, Ilya Sherman wrote: > > Peter understands the TemplateURL code better than I do; but my guess is that > it > > would be better to pass in a single parameter to this method, rather than > > passing both a |url| and a |raw_url|. That would, I think, make the code > easier > > to understand. > > I agree. The caller should simply not call this if we don't want to create a > TURL. Acknowledged. https://codereview.chromium.org/616763002/diff/1/chrome/common/importer/impor... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/1/chrome/common/importer/impor... chrome/common/importer/importer_data_types.h:55: base::string16 raw_url; // holds raw url when GURL contains invalid url. On 2014/10/01 01:02:54, Peter Kasting wrote: > As earlier, I think it would be better to only have one of (url, raw_url). I agree. As mentioned earlier, I would use raw_url. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:164: || CanImportURLAsSearchEngine(url, &decoded_url))) { On 2014/09/30 20:54:12, Ilya Sherman wrote: > nit: The "||" should be on the previous line. ("git cl format" can help you > find and fix formatting errors) Done. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:173: if (!shortcut.empty()) { On 2014/10/01 01:02:55, Peter Kasting wrote: > Doesn't this mean we'll also import non-replaceable bookmarked URLs with > keywords as search engines? That doesn't seem right. Agree with you. Updated logic to address your comment. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:263: // If |url| is valid then we are not supposed to be here. On 2014/10/01 01:02:55, Peter Kasting wrote: > That implies callers shouldn't be calling us. I don't think that's actually > what you mean. Updated comments. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:270: url::RawCanonOutputW<1024> canon_output; On 2014/10/01 01:02:55, Peter Kasting wrote: > On 2014/09/30 20:54:12, Ilya Sherman wrote: > > Hmm, how do you know that 1024 characters is enough? > > It's not necessarily enough. This is almost certainly an exploitable security > hole. Acknowledged. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:270: url::RawCanonOutputW<1024> canon_output; On 2014/09/30 20:54:12, Ilya Sherman wrote: > Hmm, how do you know that 1024 characters is enough? Can you use an > automatically resizing class instead? Acknowledged. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:271: url::DecodeURLEscapeSequences(raw_url.c_str(), raw_url.size(), &canon_output); On 2014/10/01 01:02:55, Peter Kasting wrote: > Why can't we use the higher-level unescaping functions? Using the raw > canonicalizer functions is both cumbersome and dangerous. My bad. I could not find higher-level unescaping function earlier. Replacing it with UnescapeURLComponent from net/base/escape.h. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:288: } On 2014/09/30 20:54:12, Ilya Sherman wrote: > Please use a utility function from base/strings/string_util.h rather than > writing your own version here. Replaced my own version with ReplaceSubstringsAfterOffset function from base/strings/string_util.h. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:288: } On 2014/10/01 01:02:55, Peter Kasting wrote: > On 2014/09/30 20:54:12, Ilya Sherman wrote: > > Please use a utility function from base/strings/string_util.h rather than > > writing your own version here. > > Furthermore, there's already code in Chrome that attempts to determine if some > string is a valid, replaceable URL for TemplateURL purposes. This whole > function should be using that code, not reimplementing it. Before writing my custom code I tried to use ReplaceSearchTerms function from components/search_engines/template_url.h. But it was giving me following fatal error: 'components/metrics/proto/omnibox_event.pb.h' file not found. Later on I found the file under out/Debug/gen/protoc_out/ directory (which was generated after compiling). Hence, thought not to use this approach. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:41: // |url_keywords| is a pointer toa vector, which is filled with the imported On 2014/09/30 20:54:12, Ilya Sherman wrote: > nit: "toa" -> "to a" Done. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:51: std::vector<importer::URLKeywordInfo>* url_keywords, On 2014/10/01 01:02:55, Peter Kasting wrote: > "URL keyword" seems like a rather confusing name. Why not call the type and > variable here something about "template URLs" or "search engines"? Renaming the variable to search_engine_keywords which will passed to InProcessImporterBridge::SetKeywords(...) as parameter. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:55: // engine. Scan |url| to check if there is any replacement term, %s. If yes then On 2014/10/01 01:02:55, Peter Kasting wrote: > Nit: Scan -> scans, substitute -> substitutes. Done. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:56: // substitute the replacement term(s) with any arbitrary value. Returns true if On 2014/10/01 01:02:55, Peter Kasting wrote: > What does "with any arbitrary value" mean? Surely it uses some particular > value. Did you mean "an" instead of "any"? I meant "an" here. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.h:57: // the resulted url is a valid one. On 2014/09/30 20:54:12, Ilya Sherman wrote: > nit: "resulted" -> "resulting" Done. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader_unittest.cc:336: } // namespace bookmark_html_reader On 2014/09/30 20:54:12, Ilya Sherman wrote: > Please add test coverage for your changes. Done. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmarks_file_importer_unittest.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmarks_file_importer_unittest.cc:14: } // namespace namespace bookmark_html_reader On 2014/09/30 20:54:13, Ilya Sherman wrote: > nit: Why is this needed? Can you #include the appropriate header instead? Removed. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmarks_file_importer_unittest.cc:22: On 2014/09/30 20:54:12, Ilya Sherman wrote: > nit: Please revert this diff. Done. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmarks_file_importer_unittest.cc:48: internal::CanImportURL(GURL(test_cases[i].url))); On 2014/09/30 20:54:12, Ilya Sherman wrote: > nit: Please revert this diff. Done. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmarks_file_importer_unittest.cc:66: } On 2014/09/30 20:54:13, Ilya Sherman wrote: > This test code belongs in the bookmark_html_reader_unittest.cc file. Also, you > should test the public API, rather than testing just this helper method. Removed. Corresponding test code is added to bookmark_html_reader_unittest.cc. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:260: // Import invalid bookmark's keyword as search engine. On 2014/10/01 01:02:55, Peter Kasting wrote: > How is the functionality from this file different than in > bookmark_html_reader.cc:ImportBookmarksFile()? Why do we have this code twice? > We should only have one implementation of this sort of thing. I agree. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:272: } On 2014/09/30 20:54:13, Ilya Sherman wrote: > Please add test coverage for these changes Added a new bookmark entry into places.sql files placed in firefox profile directories under /chrome/test/data/ directory: 1) url: http://example.%s.com/ 2) keyword: keyword 3) Title: Bookmark Keyword. P.S.: If I recall correctly, earlier we had issue in passing browser tests due to some updated binary files. As a work around I had to email you those files. Please let me know if you want me to send place.sql files via email/attachment.
Thanks, Tapu! https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:288: } On 2014/10/07 02:02:44, Tapu wrote: > On 2014/10/01 01:02:55, Peter Kasting wrote: > > On 2014/09/30 20:54:12, Ilya Sherman wrote: > > > Please use a utility function from base/strings/string_util.h rather than > > > writing your own version here. > > > > Furthermore, there's already code in Chrome that attempts to determine if some > > string is a valid, replaceable URL for TemplateURL purposes. This whole > > function should be using that code, not reimplementing it. > > Before writing my custom code I tried to use ReplaceSearchTerms function from > components/search_engines/template_url.h. But it was giving me following fatal > error: > 'components/metrics/proto/omnibox_event.pb.h' file not found. > Later on I found the file under out/Debug/gen/protoc_out/ directory (which was > generated after compiling). Hence, thought not to use this approach. That sounds like a dependency issue, caused by the fact that you're writing code within //chrome/utility. I'm guessing that it would be fine to fix the dependencies, by adding //components/search_engines to the list of dependencies in [ https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_util... ]. It's definitely better to reuse the existing code rather than reimplementing it. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:272: } On 2014/10/07 02:02:45, Tapu wrote: > On 2014/09/30 20:54:13, Ilya Sherman wrote: > > Please add test coverage for these changes > > Added a new bookmark entry into places.sql files placed in firefox profile > directories under /chrome/test/data/ directory: > > 1) url: http://example.%25s.com/ > 2) keyword: keyword > 3) Title: Bookmark Keyword. > > P.S.: If I recall correctly, earlier we had issue in passing browser tests due > to some updated binary files. As a work around I had to email you those files. > Please let me know if you want me to send place.sql files via email/attachment. Let's get this CL fully sorted out, with all tests passing for you locally. At that point, if the CQ won't accept the binary files as part of your CL, I can land them in a separate CL. https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... chrome/browser/importer/in_process_importer_bridge.cc:108: const base::string16& raw_url) { nit: You might as well drop the "raw" prefix at this point. Or, if you want to emphasize that the URL might not be valid, I'd name this something like "possibly_invalid_url". https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:161: title, &search_engine_keyword); nit: This formatting looks off. Please run "git cl format" over this CL. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:262: // invalid. Hence, return false for valid |url|. Hmm, what if the replacement term is outside the host? Are there any test cases that would fail if this early return were removed entirely? https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:270: net::UnescapeRule::REPLACE_PLUS_WITH_SPACE); Hmm, why do you need to unescape spaces? https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:63: importer::URLKeywordInfo* search_engine_info); nit: Please leave a blank line after this one.
On 2014/10/07 22:08:22, Ilya Sherman wrote: > Thanks, Tapu! > > https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... > File chrome/utility/importer/bookmark_html_reader.cc (right): > > https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... > chrome/utility/importer/bookmark_html_reader.cc:288: } > On 2014/10/07 02:02:44, Tapu wrote: > > On 2014/10/01 01:02:55, Peter Kasting wrote: > > > On 2014/09/30 20:54:12, Ilya Sherman wrote: > > > > Please use a utility function from base/strings/string_util.h rather than > > > > writing your own version here. > > > > > > Furthermore, there's already code in Chrome that attempts to determine if > some > > > string is a valid, replaceable URL for TemplateURL purposes. This whole > > > function should be using that code, not reimplementing it. > > > > Before writing my custom code I tried to use ReplaceSearchTerms function from > > components/search_engines/template_url.h. But it was giving me following fatal > > error: > > 'components/metrics/proto/omnibox_event.pb.h' file not found. > > Later on I found the file under out/Debug/gen/protoc_out/ directory (which was > > generated after compiling). Hence, thought not to use this approach. > > That sounds like a dependency issue, caused by the fact that you're writing code > within //chrome/utility. I'm guessing that it would be fine to fix the > dependencies, by adding //components/search_engines to the list of dependencies > in [ > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_util... > ]. It's definitely better to reuse the existing code rather than reimplementing > it. > > https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... > File chrome/utility/importer/firefox_importer.cc (right): > > https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... > chrome/utility/importer/firefox_importer.cc:272: } > On 2014/10/07 02:02:45, Tapu wrote: > > On 2014/09/30 20:54:13, Ilya Sherman wrote: > > > Please add test coverage for these changes > > > > Added a new bookmark entry into places.sql files placed in firefox profile > > directories under /chrome/test/data/ directory: > > > > 1) url: http://example.%25s.com/ > > 2) keyword: keyword > > 3) Title: Bookmark Keyword. > > > > P.S.: If I recall correctly, earlier we had issue in passing browser tests due > > to some updated binary files. As a work around I had to email you those files. > > Please let me know if you want me to send place.sql files via > email/attachment. > > Let's get this CL fully sorted out, with all tests passing for you locally. At > that point, if the CQ won't accept the binary files as part of your CL, I can > land them in a separate CL. > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... > File chrome/browser/importer/in_process_importer_bridge.cc (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... > chrome/browser/importer/in_process_importer_bridge.cc:108: const base::string16& > raw_url) { > nit: You might as well drop the "raw" prefix at this point. Or, if you want to > emphasize that the URL might not be valid, I'd name this something like > "possibly_invalid_url". > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > File chrome/utility/importer/bookmark_html_reader.cc (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.cc:161: title, > &search_engine_keyword); > nit: This formatting looks off. Please run "git cl format" over this CL. > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.cc:262: // invalid. Hence, return > false for valid |url|. > Hmm, what if the replacement term is outside the host? Are there any test cases > that would fail if this early return were removed entirely? > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.cc:270: > net::UnescapeRule::REPLACE_PLUS_WITH_SPACE); > Hmm, why do you need to unescape spaces? > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > File chrome/utility/importer/bookmark_html_reader.h (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.h:63: importer::URLKeywordInfo* > search_engine_info); > nit: Please leave a blank line after this one. Please let me know if you want me to email you the updated places.sqlite files for firefox profiles. Since those are binary files, I have a feeling that they may cause issues in passing the tests.
On 2014/10/07 22:24:02, Tapu wrote: > On 2014/10/07 22:08:22, Ilya Sherman wrote: > > Thanks, Tapu! > > > > > https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... > > File chrome/utility/importer/bookmark_html_reader.cc (right): > > > > > https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... > > chrome/utility/importer/bookmark_html_reader.cc:288: } > > On 2014/10/07 02:02:44, Tapu wrote: > > > On 2014/10/01 01:02:55, Peter Kasting wrote: > > > > On 2014/09/30 20:54:12, Ilya Sherman wrote: > > > > > Please use a utility function from base/strings/string_util.h rather > than > > > > > writing your own version here. > > > > > > > > Furthermore, there's already code in Chrome that attempts to determine if > > some > > > > string is a valid, replaceable URL for TemplateURL purposes. This whole > > > > function should be using that code, not reimplementing it. > > > > > > Before writing my custom code I tried to use ReplaceSearchTerms function > from > > > components/search_engines/template_url.h. But it was giving me following > fatal > > > error: > > > 'components/metrics/proto/omnibox_event.pb.h' file not found. > > > Later on I found the file under out/Debug/gen/protoc_out/ directory (which > was > > > generated after compiling). Hence, thought not to use this approach. > > > > That sounds like a dependency issue, caused by the fact that you're writing > code > > within //chrome/utility. I'm guessing that it would be fine to fix the > > dependencies, by adding //components/search_engines to the list of > dependencies > > in [ > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_util... > > ]. It's definitely better to reuse the existing code rather than > reimplementing > > it. > > > > > https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... > > File chrome/utility/importer/firefox_importer.cc (right): > > > > > https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... > > chrome/utility/importer/firefox_importer.cc:272: } > > On 2014/10/07 02:02:45, Tapu wrote: > > > On 2014/09/30 20:54:13, Ilya Sherman wrote: > > > > Please add test coverage for these changes > > > > > > Added a new bookmark entry into places.sql files placed in firefox profile > > > directories under /chrome/test/data/ directory: > > > > > > 1) url: http://example.%25s.com/ > > > 2) keyword: keyword > > > 3) Title: Bookmark Keyword. > > > > > > P.S.: If I recall correctly, earlier we had issue in passing browser tests > due > > > to some updated binary files. As a work around I had to email you those > files. > > > Please let me know if you want me to send place.sql files via > > email/attachment. > > > > Let's get this CL fully sorted out, with all tests passing for you locally. > At > > that point, if the CQ won't accept the binary files as part of your CL, I can > > land them in a separate CL. > > > > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... > > File chrome/browser/importer/in_process_importer_bridge.cc (right): > > > > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... > > chrome/browser/importer/in_process_importer_bridge.cc:108: const > base::string16& > > raw_url) { > > nit: You might as well drop the "raw" prefix at this point. Or, if you want > to > > emphasize that the URL might not be valid, I'd name this something like > > "possibly_invalid_url". > > > > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > > File chrome/utility/importer/bookmark_html_reader.cc (right): > > > > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > > chrome/utility/importer/bookmark_html_reader.cc:161: title, > > &search_engine_keyword); > > nit: This formatting looks off. Please run "git cl format" over this CL. > > > > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > > chrome/utility/importer/bookmark_html_reader.cc:262: // invalid. Hence, return > > false for valid |url|. > > Hmm, what if the replacement term is outside the host? Are there any test > cases > > that would fail if this early return were removed entirely? > > > > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > > chrome/utility/importer/bookmark_html_reader.cc:270: > > net::UnescapeRule::REPLACE_PLUS_WITH_SPACE); > > Hmm, why do you need to unescape spaces? > > > > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > > File chrome/utility/importer/bookmark_html_reader.h (right): > > > > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > > chrome/utility/importer/bookmark_html_reader.h:63: importer::URLKeywordInfo* > > search_engine_info); > > nit: Please leave a blank line after this one. > > Please let me know if you want me to email you the updated places.sqlite files > for firefox profiles. Since those are binary files, I have a feeling that they > may cause issues in passing the tests. Not yet. Let's see if the CQ can handle them now; if not, I'll ask you to email them to me later.
https://codereview.chromium.org/616763002/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/bookmark_html_writer_unittest.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:246: std::vector<importer::URLKeywordInfo> parsed_url_keywords; Nit: parsed_search_engines? https://codereview.chromium.org/616763002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:252: &parsed_url_keywords, Should we at least be testing that this is empty? Should there be any other tests of this in this file? https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... chrome/browser/importer/in_process_importer_bridge.cc:104: // Creates a TemplateURL with the |keyword| and |raw_url|. |title| may be empty. This comment should probably also talk about what constraints, if any, apply to the keyword and URL. https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... chrome/browser/importer/in_process_importer_bridge.cc:109: // Skip if the |raw_url| is empty. This comment just restates the code. Instead, if a comment is needed, say _why_ the URL might be empty. https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... chrome/browser/importer/in_process_importer_bridge.cc:115: data.SetKeyword(TemplateURL::GenerateKeyword(GURL(raw_url))); This assumes that |raw_url| will construct a valid GURL. Is that assumption guaranteed? See also comments in importer_data_types.h. https://codereview.chromium.org/616763002/diff/20001/chrome/common/importer/i... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/20001/chrome/common/importer/i... chrome/common/importer/importer_data_types.h:50: // Contains information needed for importing bookmarks/search engine urls, etc. Is this comment still accurate? It seems like this doesn't have anything to do with bookmarks, just search engines. Maybe it should also be named SearchEngineInfo? https://codereview.chromium.org/616763002/diff/20001/chrome/common/importer/i... chrome/common/importer/importer_data_types.h:54: base::string16 raw_url; Nit: Why are you moving this to the bottom? Leave it at the top, name it |url|, and add comments about why it's not a GURL and other guarantees, e.g. when this is guaranteed to convert to a valid GURL. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:159: bool is_valid_replaceable_url = Nit: Why are the variable and the function named totally different things? https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:161: title, &search_engine_keyword); On 2014/10/07 22:08:22, Ilya Sherman wrote: > nit: This formatting looks off. Please run "git cl format" over this CL. No, this formatting is legal. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:270: net::UnescapeRule::REPLACE_PLUS_WITH_SPACE); On 2014/10/07 22:08:22, Ilya Sherman wrote: > Hmm, why do you need to unescape spaces? And pluses? Does this need to be some sort of "fully unescaped" string lest we double-escape it later? If so that needs to be clearer. Can we just replace basically this whole function with something like this?: TemplateURLData data; data.SetURL(url_as_string); SearchTermsData search_terms_data; return TemplateURL(data).SupportsReplacement(search_terms_data); This lets the TemplateURL machinery do the parsing instead of you rewriting it. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:42: // imported keywords that will be imported as search engines. Nit: "filled with the imported search engines". (I don't know what a "keyword that will be imported as a search engine" is supposed to mean.) https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:54: // Checks if a bookmark that contains invalid |url| can be imported as search Wait, so |url| is known to be invalid when this function is called? https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:57: // true if the resulting url is a valid one. This is describing more what the implementation does than what the caller cares about from an API perspective. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:59: // |keyword| and decoded url derived from |url|. This is mostly a description of the structure definition of URLKeywordInfo, which doesn't belong here. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/bookmarks_file_importer.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmarks_file_importer.cc:109: if (!url_keywords.empty() && !cancelled()) { If we're supposed to check cancelled() here, are we supposed to check it in the next conditional? Should cancelled() result in some sort of early-return or something? https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:251: if (item->type == TYPE_FOLDER) { Nit: Slightly shorter: if (item->type != TYPE_BOOKMARK) { // ... if ((item->type != TYPE_FOLDER) || !item->empty_folder) continue; } else if (!CanImportURL(item->url)) { ... continue; } https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:260: // Import invalid bookmark's keyword as search engine. This comment sounds unconditional, which isn't true of the code.
On 2014/10/07 23:17:39, Peter Kasting wrote: > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/bookmarks... > File chrome/browser/bookmarks/bookmark_html_writer_unittest.cc (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/bookmarks... > chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:246: > std::vector<importer::URLKeywordInfo> parsed_url_keywords; > Nit: parsed_search_engines? > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/bookmarks... > chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:252: > &parsed_url_keywords, > Should we at least be testing that this is empty? Should there be any other > tests of this in this file? > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... > File chrome/browser/importer/in_process_importer_bridge.cc (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... > chrome/browser/importer/in_process_importer_bridge.cc:104: // Creates a > TemplateURL with the |keyword| and |raw_url|. |title| may be empty. > This comment should probably also talk about what constraints, if any, apply to > the keyword and URL. > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... > chrome/browser/importer/in_process_importer_bridge.cc:109: // Skip if the > |raw_url| is empty. > This comment just restates the code. Instead, if a comment is needed, say _why_ > the URL might be empty. > > https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... > chrome/browser/importer/in_process_importer_bridge.cc:115: > data.SetKeyword(TemplateURL::GenerateKeyword(GURL(raw_url))); > This assumes that |raw_url| will construct a valid GURL. Is that assumption > guaranteed? See also comments in importer_data_types.h. > > https://codereview.chromium.org/616763002/diff/20001/chrome/common/importer/i... > File chrome/common/importer/importer_data_types.h (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/common/importer/i... > chrome/common/importer/importer_data_types.h:50: // Contains information needed > for importing bookmarks/search engine urls, etc. > Is this comment still accurate? It seems like this doesn't have anything to do > with bookmarks, just search engines. > > Maybe it should also be named SearchEngineInfo? > > https://codereview.chromium.org/616763002/diff/20001/chrome/common/importer/i... > chrome/common/importer/importer_data_types.h:54: base::string16 raw_url; > Nit: Why are you moving this to the bottom? Leave it at the top, name it |url|, > and add comments about why it's not a GURL and other guarantees, e.g. when this > is guaranteed to convert to a valid GURL. > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > File chrome/utility/importer/bookmark_html_reader.cc (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.cc:159: bool > is_valid_replaceable_url = > Nit: Why are the variable and the function named totally different things? > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.cc:161: title, > &search_engine_keyword); > On 2014/10/07 22:08:22, Ilya Sherman wrote: > > nit: This formatting looks off. Please run "git cl format" over this CL. > > No, this formatting is legal. > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.cc:270: > net::UnescapeRule::REPLACE_PLUS_WITH_SPACE); > On 2014/10/07 22:08:22, Ilya Sherman wrote: > > Hmm, why do you need to unescape spaces? > > And pluses? Does this need to be some sort of "fully unescaped" string lest we > double-escape it later? If so that needs to be clearer. > > Can we just replace basically this whole function with something like this?: > > TemplateURLData data; > data.SetURL(url_as_string); > SearchTermsData search_terms_data; > return TemplateURL(data).SupportsReplacement(search_terms_data); > > This lets the TemplateURL machinery do the parsing instead of you rewriting it. > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > File chrome/utility/importer/bookmark_html_reader.h (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.h:42: // imported keywords that > will be imported as search engines. > Nit: "filled with the imported search engines". (I don't know what a "keyword > that will be imported as a search engine" is supposed to mean.) > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.h:54: // Checks if a bookmark that > contains invalid |url| can be imported as search > Wait, so |url| is known to be invalid when this function is called? > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.h:57: // true if the resulting url > is a valid one. > This is describing more what the implementation does than what the caller cares > about from an API perspective. > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmark_html_reader.h:59: // |keyword| and decoded url > derived from |url|. > This is mostly a description of the structure definition of URLKeywordInfo, > which doesn't belong here. > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > File chrome/utility/importer/bookmarks_file_importer.cc (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/bookmarks_file_importer.cc:109: if > (!url_keywords.empty() && !cancelled()) { > If we're supposed to check cancelled() here, are we supposed to check it in the > next conditional? Should cancelled() result in some sort of early-return or > something? > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > File chrome/utility/importer/firefox_importer.cc (right): > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/firefox_importer.cc:251: if (item->type == TYPE_FOLDER) > { > Nit: Slightly shorter: > > if (item->type != TYPE_BOOKMARK) { > // ... > if ((item->type != TYPE_FOLDER) || !item->empty_folder) > continue; > } else if (!CanImportURL(item->url)) { > ... > continue; > } > > https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... > chrome/utility/importer/firefox_importer.cc:260: // Import invalid bookmark's > keyword as search engine. > This comment sounds unconditional, which isn't true of the code. PTAL.
PTAL https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:288: } On 2014/10/07 22:08:22, Ilya Sherman wrote: > On 2014/10/07 02:02:44, Tapu wrote: > > On 2014/10/01 01:02:55, Peter Kasting wrote: > > > On 2014/09/30 20:54:12, Ilya Sherman wrote: > > > > Please use a utility function from base/strings/string_util.h rather than > > > > writing your own version here. > > > > > > Furthermore, there's already code in Chrome that attempts to determine if > some > > > string is a valid, replaceable URL for TemplateURL purposes. This whole > > > function should be using that code, not reimplementing it. > > > > Before writing my custom code I tried to use ReplaceSearchTerms function from > > components/search_engines/template_url.h. But it was giving me following fatal > > error: > > 'components/metrics/proto/omnibox_event.pb.h' file not found. > > Later on I found the file under out/Debug/gen/protoc_out/ directory (which was > > generated after compiling). Hence, thought not to use this approach. > > That sounds like a dependency issue, caused by the fact that you're writing code > within //chrome/utility. I'm guessing that it would be fine to fix the > dependencies, by adding //components/search_engines to the list of dependencies > in [ > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_util... > ]. It's definitely better to reuse the existing code rather than reimplementing > it. Tried after adding '../components/search_engines.gypi:search_engines'. But no luck. https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:272: } On 2014/10/07 22:08:22, Ilya Sherman wrote: > On 2014/10/07 02:02:45, Tapu wrote: > > On 2014/09/30 20:54:13, Ilya Sherman wrote: > > > Please add test coverage for these changes > > > > Added a new bookmark entry into places.sql files placed in firefox profile > > directories under /chrome/test/data/ directory: > > > > 1) url: http://example.%25s.com/ > > 2) keyword: keyword > > 3) Title: Bookmark Keyword. > > > > P.S.: If I recall correctly, earlier we had issue in passing browser tests due > > to some updated binary files. As a work around I had to email you those files. > > Please let me know if you want me to send place.sql files via > email/attachment. > > Let's get this CL fully sorted out, with all tests passing for you locally. At > that point, if the CQ won't accept the binary files as part of your CL, I can > land them in a separate CL. Tests passing for me locally. https://codereview.chromium.org/616763002/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/bookmark_html_writer_unittest.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:246: std::vector<importer::URLKeywordInfo> parsed_url_keywords; On 2014/10/07 23:17:38, Peter Kasting wrote: > Nit: parsed_search_engines? Done. https://codereview.chromium.org/616763002/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:252: &parsed_url_keywords, On 2014/10/07 23:17:38, Peter Kasting wrote: > Should we at least be testing that this is empty? Should there be any other > tests of this in this file? Since there is no entry in the BookmarkModel that can be imported as search engine, code added to verify that parsed_url_keywords is empty. An entry could have been made to BookmarkModel such that it can be imported as search engine. And appropriate test could have been added. But similar test is attempted in /chrome/utility/importer/bookmark_html_reader_unittest.cc. https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... chrome/browser/importer/in_process_importer_bridge.cc:104: // Creates a TemplateURL with the |keyword| and |raw_url|. |title| may be empty. On 2014/10/07 23:17:38, Peter Kasting wrote: > This comment should probably also talk about what constraints, if any, apply to > the keyword and URL. Done. https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... chrome/browser/importer/in_process_importer_bridge.cc:108: const base::string16& raw_url) { On 2014/10/07 22:08:22, Ilya Sherman wrote: > nit: You might as well drop the "raw" prefix at this point. Or, if you want to > emphasize that the URL might not be valid, I'd name this something like > "possibly_invalid_url". Dropped "raw" prefix. Here, the url can be either valid or invalid. Hence, not naming it as "possibly_invalid_url". https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... chrome/browser/importer/in_process_importer_bridge.cc:109: // Skip if the |raw_url| is empty. On 2014/10/07 23:17:38, Peter Kasting wrote: > This comment just restates the code. Instead, if a comment is needed, say _why_ > the URL might be empty. Agree with you. https://codereview.chromium.org/616763002/diff/20001/chrome/browser/importer/... chrome/browser/importer/in_process_importer_bridge.cc:115: data.SetKeyword(TemplateURL::GenerateKeyword(GURL(raw_url))); On 2014/10/07 23:17:38, Peter Kasting wrote: > This assumes that |raw_url| will construct a valid GURL. Is that assumption > guaranteed? See also comments in importer_data_types.h. The assumption is not guaranteed. Updated logic so address it. https://codereview.chromium.org/616763002/diff/20001/chrome/common/importer/i... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/20001/chrome/common/importer/i... chrome/common/importer/importer_data_types.h:50: // Contains information needed for importing bookmarks/search engine urls, etc. On 2014/10/07 23:17:38, Peter Kasting wrote: > Is this comment still accurate? It seems like this doesn't have anything to do > with bookmarks, just search engines. > > Maybe it should also be named SearchEngineInfo? Done. https://codereview.chromium.org/616763002/diff/20001/chrome/common/importer/i... chrome/common/importer/importer_data_types.h:54: base::string16 raw_url; On 2014/10/07 23:17:38, Peter Kasting wrote: > Nit: Why are you moving this to the bottom? Leave it at the top, name it |url|, > and add comments about why it's not a GURL and other guarantees, e.g. when this > is guaranteed to convert to a valid GURL. Done. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:159: bool is_valid_replaceable_url = On 2014/10/07 23:17:38, Peter Kasting wrote: > Nit: Why are the variable and the function named totally different things? Renamed to search_engine with type as SearchEngineInfo. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:161: title, &search_engine_keyword); On 2014/10/07 22:08:22, Ilya Sherman wrote: > nit: This formatting looks off. Please run "git cl format" over this CL. Acknowledged. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:161: title, &search_engine_keyword); On 2014/10/07 23:17:38, Peter Kasting wrote: > On 2014/10/07 22:08:22, Ilya Sherman wrote: > > nit: This formatting looks off. Please run "git cl format" over this CL. > > No, this formatting is legal. Acknowledged. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:262: // invalid. Hence, return false for valid |url|. On 2014/10/07 22:08:22, Ilya Sherman wrote: > Hmm, what if the replacement term is outside the host? Are there any test cases > that would fail if this early return were removed entirely? Removed the condition. After looking at the flow in chrome/utility/importer/firefox_importer.cc, I thought the function should only be called for invalid urls. However, by looking at flow of ImportBookmarksFile, I think we should call it even for valid urls. Test case added. (chrome/test/data/bookmark_html_reader/firefox_bookmark_keyword.html) https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:270: net::UnescapeRule::REPLACE_PLUS_WITH_SPACE); On 2014/10/07 22:08:22, Ilya Sherman wrote: > Hmm, why do you need to unescape spaces? Removed REPLACE_PLUS_WITH_SPACE. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.cc:270: net::UnescapeRule::REPLACE_PLUS_WITH_SPACE); On 2014/10/07 23:17:38, Peter Kasting wrote: > On 2014/10/07 22:08:22, Ilya Sherman wrote: > > Hmm, why do you need to unescape spaces? > > And pluses? Does this need to be some sort of "fully unescaped" string lest we > double-escape it later? If so that needs to be clearer. > > Can we just replace basically this whole function with something like this?: > > TemplateURLData data; > data.SetURL(url_as_string); > SearchTermsData search_terms_data; > return TemplateURL(data).SupportsReplacement(search_terms_data); > > This lets the TemplateURL machinery do the parsing instead of you rewriting it. As I mentioned in Patch Set 1...... when I try to include "components/search_engines/template_url.h" in the header I get following fatal error: 'components/metrics/proto/omnibox_event.pb.h' file not found. Since I am writing code in //chrome/utility, Ilya thought it as a dependency issue and suggested to add //components/search_engines to the list of dependencies in //chrome/chrome_utility.gypi. I added "../components/search_engines.gypi:search_engines" as suggested. But no luck. I was wondering if anyone can give little more knowledge on this issue. I am definitely wishing to re-use the existing code (instead of writing new code with same functionality....which makes no sense). https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:42: // imported keywords that will be imported as search engines. On 2014/10/07 23:17:38, Peter Kasting wrote: > Nit: "filled with the imported search engines". (I don't know what a "keyword > that will be imported as a search engine" is supposed to mean.) Rephrased as you suggested. However, I meant to say "filled with the imported bookmark keywords that will be imported as search engine". Here, we are trying to import bookmarks as search engines if the corresponding bookmark contains any keyword. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:54: // Checks if a bookmark that contains invalid |url| can be imported as search On 2014/10/07 23:17:38, Peter Kasting wrote: > Wait, so |url| is known to be invalid when this function is called? After looking at the flow in chrome/utility/importer/firefox_importer.cc, I thought that this particular function will only be called if a bookmark contains invalid |url|. According to the flow, if |url| contains replacement term in host (e.g., http://example.%s.com) then GURL treats it as invalid. However, it can be imported as search engine. And from firefox_importer.cc this function is only called when url.is_valid() returns false. But when I was addressing one of Ilya's comments for chrome/utility/importer/bookmark_html_reader.cc, I realized that this function should also be called for valid url (e.g., http://example.com?id=%s). In short, |url| can be either valid or invalid when this function is called. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:57: // true if the resulting url is a valid one. On 2014/10/07 23:17:38, Peter Kasting wrote: > This is describing more what the implementation does than what the caller cares > about from an API perspective. Acknowledged. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:59: // |keyword| and decoded url derived from |url|. On 2014/10/07 23:17:38, Peter Kasting wrote: > This is mostly a description of the structure definition of URLKeywordInfo, > which doesn't belong here. Acknowledged. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmark_html_reader.h:63: importer::URLKeywordInfo* search_engine_info); On 2014/10/07 22:08:22, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/bookmarks_file_importer.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/bookmarks_file_importer.cc:109: if (!url_keywords.empty() && !cancelled()) { On 2014/10/07 23:17:39, Peter Kasting wrote: > If we're supposed to check cancelled() here, are we supposed to check it in the > next conditional? Should cancelled() result in some sort of early-return or > something? Removed check for cancelled(). https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:251: if (item->type == TYPE_FOLDER) { On 2014/10/07 23:17:39, Peter Kasting wrote: > Nit: Slightly shorter: > > if (item->type != TYPE_BOOKMARK) { > // ... > if ((item->type != TYPE_FOLDER) || !item->empty_folder) > continue; > } else if (!CanImportURL(item->url)) { > ... > continue; > } Done. https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:260: // Import invalid bookmark's keyword as search engine. On 2014/10/07 23:17:39, Peter Kasting wrote: > This comment sounds unconditional, which isn't true of the code. Acknowledged.
https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:288: } On 2014/10/12 00:58:19, Tapu wrote: > On 2014/10/07 22:08:22, Ilya Sherman wrote: > > On 2014/10/07 02:02:44, Tapu wrote: > > > On 2014/10/01 01:02:55, Peter Kasting wrote: > > > > On 2014/09/30 20:54:12, Ilya Sherman wrote: > > > > > Please use a utility function from base/strings/string_util.h rather > than > > > > > writing your own version here. > > > > > > > > Furthermore, there's already code in Chrome that attempts to determine if > > some > > > > string is a valid, replaceable URL for TemplateURL purposes. This whole > > > > function should be using that code, not reimplementing it. > > > > > > Before writing my custom code I tried to use ReplaceSearchTerms function > from > > > components/search_engines/template_url.h. But it was giving me following > fatal > > > error: > > > 'components/metrics/proto/omnibox_event.pb.h' file not found. > > > Later on I found the file under out/Debug/gen/protoc_out/ directory (which > was > > > generated after compiling). Hence, thought not to use this approach. > > > > That sounds like a dependency issue, caused by the fact that you're writing > code > > within //chrome/utility. I'm guessing that it would be fine to fix the > > dependencies, by adding //components/search_engines to the list of > dependencies > > in [ > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_util... > > ]. It's definitely better to reuse the existing code rather than > reimplementing > > it. > > Tried after adding '../components/search_engines.gypi:search_engines'. But no > luck. Hmm, based on local testing, it looks like you need to add these two lines: '../components/components.gyp:component_metrics_proto', '../components/components.gyp:search_engines', The first one shouldn't be necessary; instead, //components/metrics.gypi or //components/search_engines.gypi should somehow specify this dependency. Unfortunately, I'm not sufficiently well versed in the build system to know how exactly to correctly express this. I'd recommend asking around on the #chromium IRC channel. Note that you should actually update both the .gyp* files and the BUILD.gn files, and verify that compilation works using both systems.
ghose.tapu@gmail.com changed reviewers: - pkasting@chromium.org
https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/1/chrome/utility/importer/book... chrome/utility/importer/bookmark_html_reader.cc:288: } On 2014/10/14 00:41:49, Ilya Sherman wrote: > On 2014/10/12 00:58:19, Tapu wrote: > > On 2014/10/07 22:08:22, Ilya Sherman wrote: > > > On 2014/10/07 02:02:44, Tapu wrote: > > > > On 2014/10/01 01:02:55, Peter Kasting wrote: > > > > > On 2014/09/30 20:54:12, Ilya Sherman wrote: > > > > > > Please use a utility function from base/strings/string_util.h rather > > than > > > > > > writing your own version here. > > > > > > > > > > Furthermore, there's already code in Chrome that attempts to determine > if > > > some > > > > > string is a valid, replaceable URL for TemplateURL purposes. This whole > > > > > function should be using that code, not reimplementing it. > > > > > > > > Before writing my custom code I tried to use ReplaceSearchTerms function > > from > > > > components/search_engines/template_url.h. But it was giving me following > > fatal > > > > error: > > > > 'components/metrics/proto/omnibox_event.pb.h' file not found. > > > > Later on I found the file under out/Debug/gen/protoc_out/ directory (which > > was > > > > generated after compiling). Hence, thought not to use this approach. > > > > > > That sounds like a dependency issue, caused by the fact that you're writing > > code > > > within //chrome/utility. I'm guessing that it would be fine to fix the > > > dependencies, by adding //components/search_engines to the list of > > dependencies > > > in [ > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_util... > > > ]. It's definitely better to reuse the existing code rather than > > reimplementing > > > it. > > > > Tried after adding '../components/search_engines.gypi:search_engines'. But no > > luck. > > Hmm, based on local testing, it looks like you need to add these two lines: > > '../components/components.gyp:component_metrics_proto', > '../components/components.gyp:search_engines', > > The first one shouldn't be necessary; instead, //components/metrics.gypi or > //components/search_engines.gypi should somehow specify this dependency. > Unfortunately, I'm not sufficiently well versed in the build system to know how > exactly to correctly express this. I'd recommend asking around on the #chromium > IRC channel. Note that you should actually update both the .gyp* files and the > BUILD.gn files, and verify that compilation works using both systems. After modifying .gyp* and BUILD.gn do I need to run any specific command other than "ninja -C out/Debug chrome"?
ghose.tapu@gmail.com changed reviewers: + pkasting@google.com
PTAL.
FYI, while submitting the patch I noticed following pre-submit message/warning:
** Presubmit Messages **
Missing LGTM from OWNERS of dependencies added to DEPS:
'+components/search_engines',
** Presubmit Warnings **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:22
https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/bookmark_html_reader.cc (right):
https://codereview.chromium.org/616763002/diff/20001/chrome/utility/importer/...
chrome/utility/importer/bookmark_html_reader.cc:270:
net::UnescapeRule::REPLACE_PLUS_WITH_SPACE);
On 2014/10/12 00:58:20, Tapu wrote:
> On 2014/10/07 23:17:38, Peter Kasting wrote:
> > On 2014/10/07 22:08:22, Ilya Sherman wrote:
> > > Hmm, why do you need to unescape spaces?
> >
> > And pluses? Does this need to be some sort of "fully unescaped" string lest
> we
> > double-escape it later? If so that needs to be clearer.
> >
> > Can we just replace basically this whole function with something like this?:
> >
> > TemplateURLData data;
> > data.SetURL(url_as_string);
> > SearchTermsData search_terms_data;
> > return TemplateURL(data).SupportsReplacement(search_terms_data);
> >
> > This lets the TemplateURL machinery do the parsing instead of you rewriting
> it.
>
> As I mentioned in Patch Set 1......
>
> when I try to include "components/search_engines/template_url.h" in the header
I
> get following fatal error: 'components/metrics/proto/omnibox_event.pb.h' file
> not found.
>
> Since I am writing code in //chrome/utility, Ilya thought it as a dependency
> issue and suggested to add //components/search_engines to the list of
> dependencies in //chrome/chrome_utility.gypi. I added
>
> "../components/search_engines.gypi:search_engines" as suggested. But no luck.
>
> I was wondering if anyone can give little more knowledge on this issue. I am
> definitely wishing to re-use the existing code (instead of writing new code
with
> same functionality....which makes no sense).
Finally, the dependency issue has been resolved. I raised this dependency issue
in #chromium IRC channel. The dev who worked on search_engines componentization
submitted a path to fix the dependency issue. Thanks to him and all other
members who threw their advice in the thread.
I then replaced my own function as Peter suggested. But then
TemplateURL(data).SupportsReplacement(search_terms_data) was always returning
false. After investigation I found that it is failing in ParseURL function of
../components/search_engines/template_url.cc (line 672)
For my test case the value of parsed_url was "http://example.%s.com/". To
overcome this I have replaced the replacement term (%s) by "{searchTerms}"
before calling SupportsReplacement function.
Thanks, Tapu. I've kicked off some try bot runs, so we'll see whether binary files are supported yet. https://codereview.chromium.org/616763002/diff/180001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer_unittest.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:21: #include "chrome/common/importer/importer_data_types.h" nit: The presubmit warning was warning you that this line is not alphabetized correctly. https://codereview.chromium.org/616763002/diff/180001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:266: // imported as search engine, verify that we got back no search engine. nit: "no search engine" -> "no search engines" https://codereview.chromium.org/616763002/diff/180001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:96: // If |url| contains replacement term(s) in host then it cannot be converted nit: "in host" -> "in the host" https://codereview.chromium.org/616763002/diff/180001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:97: // into a valid GURL. In such scenario, |keyword| can not be empty. |title| may nit: "In such scenario" -> "In such a scenario" https://codereview.chromium.org/616763002/diff/180001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:120: TemplateURLRef::DisplayURLToURLRef(url)); nit: Looks like you can un-wrap this line. https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:53: // terms(s) in host (e.g., http://example.%s.com). Since |url| may contain nit: Is "%s" still the up-to-date string? https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:54: // replacement term(s) in host, |url| is chosen as string instead of GURL. nit: Please indent these comment lines two spaces each. https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:54: // replacement term(s) in host, |url| is chosen as string instead of GURL. nit: "in host" -> "in the host" https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:54: // replacement term(s) in host, |url| is chosen as string instead of GURL. nit: "is chose as string instead of GURL" -> "is stored as a string rather than as a GURL" https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:163: title, &search_engine); nit: Looks like this doesn't need to wrap. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:168: is_valid_replaceable_url)) { Should you also check "!shortcut.empty()" here? It might be simpler to move the inner if-stmt that you've added -- the one with the condition "is_valid_replaceable_url && !shortcut.empty()" -- to be above this if-stmt, rather than within it. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:263: url.is_valid() ? url.spec() : url.possibly_invalid_spec(); nit: Why not just always grab the possibly_invalid_spec? https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:271: net::UnescapeRule::URL_SPECIAL_CHARS); Do you really need both of these unescape rules? If so, could you please clarify why? Naively, it seems to me like you could skip unescaping the url entirely. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:286: return TemplateURL(data).SupportsReplacement(search_terms_data); nit: I believe that you can combine the above two lines as "return TemplateURL(data).SupportsReplacement(SearchTermsData());" https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:54: // Checks if a bookmark |url| can be imported as search engine. A bookmark |url| nit: "as search engine" -> "as a search engine" https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:55: // may not be imported as bookmark if |url| contains replacement term(s) in nit: "as bookmark" -> "as a bookmark" https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:56: // host. However, such |url| may be eligible to be imported as search engine. nit: "as search engine" -> "as a search engine" https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:345: namespace { nit: Please restore the newline after this line. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:367: } // namespace bookmark_html_reader Please add test coverage specifically for CanImportURLAsSearchEngine(), since that's exported as part of the public API for this file. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/bookmarks_file_importer.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmarks_file_importer.cc:111: } nit: No need for curly braces. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:262: bookmark_html_reader::CanImportURLAsSearchEngine(item->url, nit: Please wrap this arg to the next line.
PTAL. https://codereview.chromium.org/616763002/diff/180001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_html_writer_unittest.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:21: #include "chrome/common/importer/importer_data_types.h" On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: The presubmit warning was warning you that this line is not alphabetized > correctly. Done. https://codereview.chromium.org/616763002/diff/180001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:266: // imported as search engine, verify that we got back no search engine. On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: "no search engine" -> "no search engines" Done. https://codereview.chromium.org/616763002/diff/180001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:96: // If |url| contains replacement term(s) in host then it cannot be converted On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: "in host" -> "in the host" Done. https://codereview.chromium.org/616763002/diff/180001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:97: // into a valid GURL. In such scenario, |keyword| can not be empty. |title| may On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: "In such scenario" -> "In such a scenario" Done. https://codereview.chromium.org/616763002/diff/180001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:120: TemplateURLRef::DisplayURLToURLRef(url)); On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: Looks like you can un-wrap this line. Done. https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:53: // terms(s) in host (e.g., http://example.%s.com). Since |url| may contain On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: Is "%s" still the up-to-date string? In this case yes. However, "%s" is replaced with "{searchTerms}" before passing it to SupportsReplacement function of TemplateURL. https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:54: // replacement term(s) in host, |url| is chosen as string instead of GURL. On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: "is chose as string instead of GURL" -> "is stored as a string rather than > as a GURL" Done. https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:54: // replacement term(s) in host, |url| is chosen as string instead of GURL. On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: Please indent these comment lines two spaces each. Done. https://codereview.chromium.org/616763002/diff/180001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:54: // replacement term(s) in host, |url| is chosen as string instead of GURL. On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: "in host" -> "in the host" Done. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:163: title, &search_engine); On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: Looks like this doesn't need to wrap. Done. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:168: is_valid_replaceable_url)) { On 2014/11/07 00:22:34, Ilya Sherman wrote: > Should you also check "!shortcut.empty()" here? It might be simpler to move the > inner if-stmt that you've added -- the one with the condition > "is_valid_replaceable_url && !shortcut.empty()" -- to be above this if-stmt, > rather than within it. Moved above the parent if-stmt. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:263: url.is_valid() ? url.spec() : url.possibly_invalid_spec(); On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: Why not just always grab the possibly_invalid_spec? Agreed. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:271: net::UnescapeRule::URL_SPECIAL_CHARS); On 2014/11/07 00:22:34, Ilya Sherman wrote: > Do you really need both of these unescape rules? If so, could you please > clarify why? Naively, it seems to me like you could skip unescaping the url > entirely. I think unescaping the url is necessary. For instance, url "http://example.%s.com" without unescape rule appears as "http://example.%25s.com". However, UnescapeRule::SPACES rule is not required. I have removed this particular rule. Moreover, unescaping the url is needed to show proper url in the UI ("Manager Search Engines" from chrome://settings). https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:286: return TemplateURL(data).SupportsReplacement(search_terms_data); On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: I believe that you can combine the above two lines as > > "return TemplateURL(data).SupportsReplacement(SearchTermsData());" Done. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:54: // Checks if a bookmark |url| can be imported as search engine. A bookmark |url| On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: "as search engine" -> "as a search engine" Done. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:55: // may not be imported as bookmark if |url| contains replacement term(s) in On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: "as bookmark" -> "as a bookmark" Done. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:56: // host. However, such |url| may be eligible to be imported as search engine. On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: "as search engine" -> "as a search engine" Done. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:345: namespace { On 2014/11/07 00:22:34, Ilya Sherman wrote: > nit: Please restore the newline after this line. Done. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:367: } // namespace bookmark_html_reader On 2014/11/07 00:22:34, Ilya Sherman wrote: > Please add test coverage specifically for CanImportURLAsSearchEngine(), since > that's exported as part of the public API for this file. Added. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/bookmarks_file_importer.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/bookmarks_file_importer.cc:111: } On 2014/11/07 00:22:35, Ilya Sherman wrote: > nit: No need for curly braces. Done. https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/180001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:262: bookmark_html_reader::CanImportURLAsSearchEngine(item->url, On 2014/11/07 00:22:35, Ilya Sherman wrote: > nit: Please wrap this arg to the next line. Done.
LGTM % nits. Thanks, Tapu! Note that you'll need Peter to approve the CL as well. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:159: CanImportURLAsSearchEngine(url, shortcut, title, &search_engine); Optional nit: I'd probably inline this into the if-stmt condition, rather than defining a separate variable for it. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:56: // in host. However, such |url| may be eligible to be imported as a search nit: "in host" -> "in its host" https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:150: base::UTF8ToUTF16("title1"), nit: Looks like you could omit the keyword and title from the test cases, and just pass fixed strings on lines 166 and 167. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:150: base::UTF8ToUTF16("title1"), nit: Please use ASCIIToUTF16; below as well. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:158: }, Please also test: (1) and empty URL, (2) a URL that needs to be escaped, (3) multiple instances of "%s" in the URL. Perhaps there are more interesting test cases that you can think of as well? https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:163: EXPECT_EQ(test_cases[i].can_be_imported_as_search_engine, nit: Please either wrap the arg, or align the subsequent line with the parenthesis.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:98: // may be empty. Nit: How about this slight tweak: Attempts to create a TemplateURL from the provided data. |title| is optional. If |keyword| is not specified, the function will attempt to generate the keyword from |url|, which in that case must be a valid GURL. This can fail if e.g. the replacement term is in the host. If TemplateURL creation fails, returns NULL. https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:234: for (size_t i = 0; i < search_engines.size(); ++i) { Nit: C++11: for (const auto& search_engine : search_engines) { ... use |search_engine| here ... https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:236: CreateTemplateURL(search_engines[i].display_name, Shouldn't we check for NULL before blindly pushing onto the vector? Otherwise, ProfileWriter::AddKeywords() has to check for embedded NULLs, and from looking at that, I don't think it does. (Seems like there should be a unittest for this?) https://codereview.chromium.org/616763002/diff/200001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/200001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:55: // a GURL. Nit: How about just: |url| is a string instead of a GURL since it may not actually be a valid GURL directly (e.g. for "http://%s.com"). https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:260: importer::SearchEngineInfo* search_engine) { Rather than taking a SearchEngineInfo*, it seems like this function should just take a (string) URL outparam. The caller can construct the SearchEngineInfo itself if it makes sense. This also lets you remove the meaningless keyword and title strings from the unittest code that exercises this function. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:279: ReplaceSubstringsAfterOffset(&raw_url, 0, kReplacementTerm, kSearchTerms); Please use TemplateURLRef::DisplayURLToURLRef() instead. Why are we not doing this transform on the URL before putting it into |search_engine|? Seems like that would prevent us having to do the same transform again later, as you've done in in_process_importer_bridge.cc:CreateTemplateURL(). https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:54: // Checks if a bookmark |url| can be imported as a search engine. A bookmark Nit: How about: Returns true if |url| should be imported as a search engine, i.e. because it has replacement terms. Chrome treats such bookmarks as search engines rather than true bookmarks. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:257: } else if (!CanImportURL(item->url)) { This will only convert invalid bookmark URLs to search engines. I'd think most "search engines" would have valid bookmark URLs, e.g. "http://foo.com/?q=%s". Shouldn't we check CanImportURLAsSearchEngine() first? https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:261: if (!item->keyword.empty() && Shouldn't we be willing to import bookmarks with empty keywords too (and autogenerate the keywords)?
PTAL. https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:98: // may be empty. On 2014/11/11 22:42:11, Peter Kasting wrote: > Nit: How about this slight tweak: > > Attempts to create a TemplateURL from the provided data. |title| is optional. > If |keyword| is not specified, the function will attempt to generate the keyword > from |url|, which in that case must be a valid GURL. This can fail if e.g. the > replacement term is in the host. If TemplateURL creation fails, returns NULL. Rephrased comment as you suggested. https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:234: for (size_t i = 0; i < search_engines.size(); ++i) { On 2014/11/11 22:42:11, Peter Kasting wrote: > Nit: C++11: > > for (const auto& search_engine : search_engines) { > ... use |search_engine| here ... I like this range-based for loop. Since the existing code was using traditional for-loop syntax, I thought not to play with it. https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:236: CreateTemplateURL(search_engines[i].display_name, On 2014/11/11 22:42:11, Peter Kasting wrote: > Shouldn't we check for NULL before blindly pushing onto the vector? Otherwise, > ProfileWriter::AddKeywords() has to check for embedded NULLs, and from looking > at that, I don't think it does. (Seems like there should be a unittest for > this?) Updated CreateTemplateURL to make use that either keyword or generated keyword is used for short name, if title is empty. Please let me know if my approach is wrong. https://codereview.chromium.org/616763002/diff/200001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/200001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:55: // a GURL. On 2014/11/11 22:42:11, Peter Kasting wrote: > Nit: How about just: > > |url| is a string instead of a GURL since it may not actually be a valid GURL > directly (e.g. for "http://%s.com"). Done. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:159: CanImportURLAsSearchEngine(url, shortcut, title, &search_engine); On 2014/11/10 23:00:22, Ilya Sherman wrote: > Optional nit: I'd probably inline this into the if-stmt condition, rather than > defining a separate variable for it. Done. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:260: importer::SearchEngineInfo* search_engine) { On 2014/11/11 22:42:11, Peter Kasting wrote: > Rather than taking a SearchEngineInfo*, it seems like this function should just > take a (string) URL outparam. The caller can construct the SearchEngineInfo > itself if it makes sense. This also lets you remove the meaningless keyword and > title strings from the unittest code that exercises this function. My understanding was that the caller indeed needs a SearchEngineInfo anyway. But I agree with you. The caller can construct it, if needed. In this way we can decouple the checking (which is the real purpose of this function) from SearchEngineInfo creation. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:279: ReplaceSubstringsAfterOffset(&raw_url, 0, kReplacementTerm, kSearchTerms); On 2014/11/11 22:42:12, Peter Kasting wrote: > Please use TemplateURLRef::DisplayURLToURLRef() instead. Many many thanks for this suggestion. > > Why are we not doing this transform on the URL before putting it into > |search_engine|? Seems like that would prevent us having to do the same > transform again later, as you've done in > in_process_importer_bridge.cc:CreateTemplateURL(). Agree with you. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.h (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:54: // Checks if a bookmark |url| can be imported as a search engine. A bookmark On 2014/11/11 22:42:12, Peter Kasting wrote: > Nit: How about: > > Returns true if |url| should be imported as a search engine, i.e. because it has > replacement terms. Chrome treats such bookmarks as search engines rather than > true bookmarks. Agreed. Looks simpler. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.h:56: // in host. However, such |url| may be eligible to be imported as a search On 2014/11/10 23:00:22, Ilya Sherman wrote: > nit: "in host" -> "in its host" Acknowledged. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:150: base::UTF8ToUTF16("title1"), On 2014/11/10 23:00:22, Ilya Sherman wrote: > nit: Looks like you could omit the keyword and title from the test cases, and > just pass fixed strings on lines 166 and 167. Removed keyword and title from test cases. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:150: base::UTF8ToUTF16("title1"), On 2014/11/10 23:00:22, Ilya Sherman wrote: > nit: Please use ASCIIToUTF16; below as well. Acknowledged. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:158: }, On 2014/11/10 23:00:22, Ilya Sherman wrote: > Please also test: (1) and empty URL, (2) a URL that needs to be escaped, (3) > multiple instances of "%s" in the URL. Perhaps there are more interesting test > cases that you can think of as well? Done. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:163: EXPECT_EQ(test_cases[i].can_be_imported_as_search_engine, On 2014/11/10 23:00:22, Ilya Sherman wrote: > nit: Please either wrap the arg, or align the subsequent line with the > parenthesis. Done. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:257: } else if (!CanImportURL(item->url)) { On 2014/11/11 22:42:12, Peter Kasting wrote: > This will only convert invalid bookmark URLs to search engines. I'd think most > "search engines" would have valid bookmark URLs, e.g. "http://foo.com/?q=%s". > Shouldn't we check CanImportURLAsSearchEngine() first? I think valid bookmark URLs are handled at line 327 (though it only looking for URL with keywords). https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:261: if (!item->keyword.empty() && On 2014/11/11 22:42:12, Peter Kasting wrote: > Shouldn't we be willing to import bookmarks with empty keywords too (and > autogenerate the keywords)? You are right.
https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:236: CreateTemplateURL(search_engines[i].display_name, On 2014/11/15 05:22:19, Tapu wrote: > On 2014/11/11 22:42:11, Peter Kasting wrote: > > Shouldn't we check for NULL before blindly pushing onto the vector? > Otherwise, > > ProfileWriter::AddKeywords() has to check for embedded NULLs, and from looking > > at that, I don't think it does. (Seems like there should be a unittest for > > this?) > > Updated CreateTemplateURL to make use that either keyword or generated keyword > is used for short name, if title is empty. Please let me know if my approach is > wrong. That's fine, but that's not what I meant by this comment. What I meant was, CreateTemplateURL() can fail and return NULL, but we don't check the return value here. That means we can push NULL onto |owned_template_urls|. That will cause ProfileWriter::AddKeywords() to crash later. This should be fixed, and a test added that would crash with your patch without the fix. https://codereview.chromium.org/616763002/diff/200001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/200001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:55: // a GURL. On 2014/11/15 05:22:19, Tapu wrote: > On 2014/11/11 22:42:11, Peter Kasting wrote: > > Nit: How about just: > > > > |url| is a string instead of a GURL since it may not actually be a valid > GURL > > directly (e.g. for "http://%s.com"). > > Done. No, I meant use my proposal for the entire comment. Right now it says the same thing three times, in a confusing (and non-grammatical) way. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:257: } else if (!CanImportURL(item->url)) { On 2014/11/15 05:22:20, Tapu wrote: > On 2014/11/11 22:42:12, Peter Kasting wrote: > > This will only convert invalid bookmark URLs to search engines. I'd think > most > > "search engines" would have valid bookmark URLs, e.g. "http://foo.com/?q=%s". > > Shouldn't we check CanImportURLAsSearchEngine() first? > > I think valid bookmark URLs are handled at line 327 (though it only looking for > URL with keywords). It's really confusing that we add things as search engines in two different places, and they have two different ways of constructing the SearchEngineInfo object and two different URL validity checks. Can we combine these into one place that handles everything, maybe by removing the CanImportURL() check here and executing this stuff unconditionally? https://codereview.chromium.org/616763002/diff/220001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/220001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:106: base::string16 generated_keyword; Nit: Declare this where it's actually defined https://codereview.chromium.org/616763002/diff/220001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:109: GURL gurl = GURL(url); Nit: Shorter: GURL gurl(url); https://codereview.chromium.org/616763002/diff/220001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:114: data.short_name = title.empty() ? generated_keyword : title; Nit: Factor this out of both branches and use keyword() https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:157: std::string search_engine_url; Nit: Declare this below the comment instead of above it https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:268: net::UnescapeRule::URL_SPECIAL_CHARS); Do you have a testcase that will fail without this unescaping? If not, please add one. https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:149: { "http://www.test&test.%s.com", true }, These cases all test %s in the hostname. Shouldn't this also test the more common case of %s in the query or ref, as well as probably in the path, and probably %S as well? Probably we should also test "%x" (or some other non-s character).
PTAL. My apologies for being late in addressing the latest feedback. I was occupied by some major product releases at my work place. Please let me know your feedback, if any. Thanks. ~ Tapu. https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:236: CreateTemplateURL(search_engines[i].display_name, On 2014/11/17 21:30:35, Peter Kasting wrote: > On 2014/11/15 05:22:19, Tapu wrote: > > On 2014/11/11 22:42:11, Peter Kasting wrote: > > > Shouldn't we check for NULL before blindly pushing onto the vector? > > Otherwise, > > > ProfileWriter::AddKeywords() has to check for embedded NULLs, and from > looking > > > at that, I don't think it does. (Seems like there should be a unittest for > > > this?) > > > > Updated CreateTemplateURL to make use that either keyword or generated keyword > > is used for short name, if title is empty. Please let me know if my approach > is > > wrong. > > That's fine, but that's not what I meant by this comment. > > What I meant was, CreateTemplateURL() can fail and return NULL, but we don't > check the return value here. That means we can push NULL onto > |owned_template_urls|. That will cause ProfileWriter::AddKeywords() to crash > later. This should be fixed, and a test added that would crash with your patch > without the fix. Got it. Checked NULL before pushing onto |owned_template_urls|. Also, I am going to revert the changes that I have made in CreateTemplateURL for setting short name either from keyword or generated keyword. Added a test case (http://%x.example.%s.com without keyword) in //chrome/browser/importer/firefox_importer_browsertest.cc that would result in segmentation fault, if NULL was not checked before pushing onto |owned_template_urls|. Please let me know if the test case was supposed to be handled in some other place. https://codereview.chromium.org/616763002/diff/200001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/200001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:55: // a GURL. On 2014/11/17 21:30:35, Peter Kasting wrote: > On 2014/11/15 05:22:19, Tapu wrote: > > On 2014/11/11 22:42:11, Peter Kasting wrote: > > > Nit: How about just: > > > > > > |url| is a string instead of a GURL since it may not actually be a valid > > GURL > > > directly (e.g. for "http://%s.com"). > > > > Done. > > No, I meant use my proposal for the entire comment. Right now it says the same > thing three times, in a confusing (and non-grammatical) way. I see. https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/200001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:257: } else if (!CanImportURL(item->url)) { On 2014/11/17 21:30:35, Peter Kasting wrote: > On 2014/11/15 05:22:20, Tapu wrote: > > On 2014/11/11 22:42:12, Peter Kasting wrote: > > > This will only convert invalid bookmark URLs to search engines. I'd think > > most > > > "search engines" would have valid bookmark URLs, e.g. > "http://foo.com/?q=%s". > > > Shouldn't we check CanImportURLAsSearchEngine() first? > > > > I think valid bookmark URLs are handled at line 327 (though it only looking > for > > URL with keywords). > > It's really confusing that we add things as search engines in two different > places, and they have two different ways of constructing the SearchEngineInfo > object and two different URL validity checks. > > Can we combine these into one place that handles everything, maybe by removing > the CanImportURL() check here and executing this stuff unconditionally? Agreed. https://codereview.chromium.org/616763002/diff/220001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/220001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:106: base::string16 generated_keyword; On 2014/11/17 21:30:35, Peter Kasting wrote: > Nit: Declare this where it's actually defined Reverted all changes made in this function for generating keyword. https://codereview.chromium.org/616763002/diff/220001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:109: GURL gurl = GURL(url); On 2014/11/17 21:30:35, Peter Kasting wrote: > Nit: Shorter: GURL gurl(url); Done. https://codereview.chromium.org/616763002/diff/220001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:114: data.short_name = title.empty() ? generated_keyword : title; On 2014/11/17 21:30:35, Peter Kasting wrote: > Nit: Factor this out of both branches and use keyword() Acknowledged. https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:157: std::string search_engine_url; On 2014/11/17 21:30:36, Peter Kasting wrote: > Nit: Declare this below the comment instead of above it Done. https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:268: net::UnescapeRule::URL_SPECIAL_CHARS); On 2014/11/17 21:30:36, Peter Kasting wrote: > Do you have a testcase that will fail without this unescaping? If not, please > add one. As I mentioned in Patch Set 4, unescaping the url is necessary. For instance, url "http://example.%s.com" without unescape rule appears as "http://example.%25s.com". If I skip unescaping the url then |SupportsReplacement| at line 277 always returns false, because it looks for "%s" (not %25s). In addition, unescaping is needed to show proper url in the UI ("Manager Search Engines" from chrome://settings). Hence, all test cases that are supposed to pass would fail, if unescaping the url is omitted. https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/220001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:149: { "http://www.test&test.%s.com", true }, On 2014/11/17 21:30:36, Peter Kasting wrote: > These cases all test %s in the hostname. Shouldn't this also test the more > common case of %s in the query or ref, as well as probably in the path, and > probably %S as well? Probably we should also test "%x" (or some other non-s > character). Added few more test cases.
https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:102: // CreateTemplateURL will return NULL for the following bookmark. Nit: Since "CreateTemplateURL" doesn't otherwise appear in this file, a reader may not know what it refers to. I would write "in_process_importer_bridge.cc:CreateTemplateURL()". https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:103: // Consequently, it won't be imported as search engine. Nit: as -> as a https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:116: data.short_name = title.empty() ? keyword : title; I think this should be "keyword()" instead of "keyword" here, so that if we generate the keyword, we still have a title. You should probably ensure one of the tests checks for this (i.e. that entries with autogenerated keywords have a non-empty title). However, see later comments about my skepticism that we actually want to allow keyword autogeneration for imports at all. https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:232: TemplateURL* owned_template_url; Nit: Declare this where it's defined, in the loop below, since we don't need its lifetime to extend beyond the loop end. https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:153: { "http://www.example.com/%s/?q=%s&foo=bar", true }, I'm actually a bit surprised we allow importing this... does it work correctly once imported? I think pieces of our search engine machinery assume there's only one substitution point in the URL. If that's correct, and this doesn't actually work, we should probably not be willing to import it either. https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:317: // engine url. I don't think the logic here is quite correct. The confusing cases all happen when there's no keyword. If there's no keyword, then even if the URL is something we can turn into a search engine URL, how was the user invoking this search engine in their old browser? The only way to activate such a bookmark would have been to click it, which wouldn't provide an opportunity to type in search terms. So I wonder if we should ever support autogeneration of keywords for imported items at all. It seems like if the user had "http://foo.com/?q=%s" bookmarked with no keyword, something is just wrong. The only way this is valid is if for some reason they had a real, standard URL with a "%s" sequence in it. In that case we should just go ahead and import as a standard bookmark, assuming the URL is valid. (If the URL is invalid, we drop this on the floor.) If we don't support autogenerating keywords, that probably makes the importer bridge simpler, and it certainly makes this code simpler: we should just import everything as a search engine for which: !item->keyword.empty() && (item->url.is_valid() || CanImportURLAsSearchEngine(item->url, ...)) https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:321: search_engine_info.url.assign(base::UTF8ToUTF16(item->url.spec())); Nit: Just use = instead of .assign() (3 places)
PTAL. https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:102: // CreateTemplateURL will return NULL for the following bookmark. On 2014/12/09 23:10:18, Peter Kasting wrote: > Nit: Since "CreateTemplateURL" doesn't otherwise appear in this file, a reader > may not know what it refers to. I would write > "in_process_importer_bridge.cc:CreateTemplateURL()". Done. https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:103: // Consequently, it won't be imported as search engine. On 2014/12/09 23:10:18, Peter Kasting wrote: > Nit: as -> as a Done. Realized that we may not have any cases that would return NULL from "CreateTemplateURL". CreateTemplateURL returns NULL in two cases: 1. passing emtpy url 2. invalid url is encountered while autogenerating keyword Case 1 will never be reached as the condition is already handled by "url.is_valid()" or "CanImportURLAsSearchEngine". Case 2 won't be reached as we do not import bookmark if there is no associated keyword. Hence, no need to autogenerate keyword. Having said that....I think we can completely eliminate this particular test case. Removing this test case....Please let me know if you want me to re-add the test case. https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:116: data.short_name = title.empty() ? keyword : title; On 2014/12/09 23:10:19, Peter Kasting wrote: > I think this should be "keyword()" instead of "keyword" here, so that if we > generate the keyword, we still have a title. > Made use of "data.keyword()" instead of "keyword". > You should probably ensure one of the tests checks for this (i.e. that entries > with autogenerated keywords have a non-empty title). > Updated logic to reflect your skepticism regarding keyword autogeneration for bookmark import. Hence, this test check is not necessary. > However, see later comments about my skepticism that we actually want to allow > keyword autogeneration for imports at all. Based on your skepticism I have updated the logic such that we do not import bookmarks that have empty keyword. In other words, we do not need keyword autogeneration for bookmark import. However, I have not removed the existing logic for autogenerating keyword from this function. I was wondering if I should remove this in order to make the code cleaner. I hope cleaning this won't break something else. https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:232: TemplateURL* owned_template_url; On 2014/12/09 23:10:19, Peter Kasting wrote: > Nit: Declare this where it's defined, in the loop below, since we don't need its > lifetime to extend beyond the loop end. Agreed. https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader_unittest.cc:153: { "http://www.example.com/%s/?q=%s&foo=bar", true }, On 2014/12/09 23:10:19, Peter Kasting wrote: > I'm actually a bit surprised we allow importing this... does it work correctly > once imported? I think pieces of our search engine machinery assume there's > only one substitution point in the URL. If that's correct, and this doesn't > actually work, we should probably not be willing to import it either. I think it works fine. I conducted the following test steps in my local machine: 1. Added a custom search engine with url as "https://code.google.com/p/chromium/codesearch#chromium/src/%s/search_engines/template_url_data.h&sq=package:chromium&l=20&q=%s" and made it my browser's default search engine 2. In omni box I typed "components" as search terms. 3. It replaced both instances of "%s" with "components" and came up with correct result. https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:317: // engine url. On 2014/12/09 23:10:19, Peter Kasting wrote: > I don't think the logic here is quite correct. > > The confusing cases all happen when there's no keyword. If there's no keyword, > then even if the URL is something we can turn into a search engine URL, how was > the user invoking this search engine in their old browser? The only way to > activate such a bookmark would have been to click it, which wouldn't provide an > opportunity to type in search terms. > > So I wonder if we should ever support autogeneration of keywords for imported > items at all. It seems like if the user had "http://foo.com/?q=%s" bookmarked > with no keyword, something is just wrong. The only way this is valid is if for > some reason they had a real, standard URL with a "%s" sequence in it. In that > case we should just go ahead and import as a standard bookmark, assuming the URL > is valid. (If the URL is invalid, we drop this on the floor.) > > If we don't support autogenerating keywords, that probably makes the importer > bridge simpler, and it certainly makes this code simpler: we should just import > everything as a search engine for which: > > !item->keyword.empty() && > (item->url.is_valid() || CanImportURLAsSearchEngine(item->url, ...)) Skipping the autogenerating keywords makes life simpler. https://codereview.chromium.org/616763002/diff/240001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:321: search_engine_info.url.assign(base::UTF8ToUTF16(item->url.spec())); On 2014/12/09 23:10:19, Peter Kasting wrote: > Nit: Just use = instead of .assign() (3 places) Done.
https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:103: // Consequently, it won't be imported as search engine. On 2014/12/10 04:41:55, Tapu wrote: > Having said that....I think we can completely eliminate this particular test > case. Removing this test case....Please let me know if you want me to re-add the > test case. We should probably still have the test, as we don't have any other tests in here of invalid input URLs. https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:116: data.short_name = title.empty() ? keyword : title; On 2014/12/10 04:41:55, Tapu wrote: > I have not removed the existing > logic for autogenerating keyword from this function. I was wondering if I should > remove this in order to make the code cleaner. I hope cleaning this won't break > something else. Well, where else is this called from? You should remove it if we don't need it, and if we do need it, you should document why.
PTAL. https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/firefox_importer_browsertest.cc:103: // Consequently, it won't be imported as search engine. On 2014/12/10 17:41:16, Peter Kasting wrote: > On 2014/12/10 04:41:55, Tapu wrote: > > Having said that....I think we can completely eliminate this particular test > > case. Removing this test case....Please let me know if you want me to re-add > the > > test case. > > We should probably still have the test, as we don't have any other tests in here > of invalid input URLs. Acknowledged. https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/240001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:116: data.short_name = title.empty() ? keyword : title; On 2014/12/10 17:41:16, Peter Kasting wrote: > On 2014/12/10 04:41:55, Tapu wrote: > > I have not removed the existing > > logic for autogenerating keyword from this function. I was wondering if I > should > > remove this in order to make the code cleaner. I hope cleaning this won't > break > > something else. > > Well, where else is this called from? You should remove it if we don't need it, > and if we do need it, you should document why. Looks like this is called while importing bookmark from HTML file or browser. In both cases, before calling this function, we make a check if keyword is non-empty. This means it is safe enough if the logic for keyword autogeneration is removed.
LGTM https://codereview.chromium.org/616763002/diff/280001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/280001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:106: data.short_name = title.empty() ? data.keyword() : title; Nit: After I harassed you to use data.keyword() here... now that the conditional above is gone, using |keyword| directly is just as good (and slightly terser). https://codereview.chromium.org/616763002/diff/280001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:224: CreateTemplateURL(search_engine.display_name, Tiny nit: I'd probably pass the args in the order they're declared in SearchEngineInfo. https://codereview.chromium.org/616763002/diff/280001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/280001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:316: // engine, provided that its url is a valid search engine url. Nit: How about: Import this bookmark as a search engine if it has a keyword and its URL is usable as a search engine URL. (Even if the URL doesn't allow substitution, importing as a "search engine" allows users to trigger the bookmark by entering its keyword in the omnibox.)
PTAL. https://codereview.chromium.org/616763002/diff/280001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/280001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:106: data.short_name = title.empty() ? data.keyword() : title; On 2014/12/12 01:04:42, Peter Kasting wrote: > Nit: After I harassed you to use data.keyword() here... now that the conditional > above is gone, using |keyword| directly is just as good (and slightly terser). I was about to do the same. But then thought you might want keyword() instead of keyword. Thanks for preferring |keyword|. https://codereview.chromium.org/616763002/diff/280001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:224: CreateTemplateURL(search_engine.display_name, On 2014/12/12 01:04:42, Peter Kasting wrote: > Tiny nit: I'd probably pass the args in the order they're declared in > SearchEngineInfo. Acknowledged.
On 2014/12/12 05:20:54, Tapu Ghose wrote: > PTAL. Sorry, if I LGTMed things, you don't need me to re-review. If you want to submit, you should just be able to check the commit box.
The CQ bit was checked by ghose.tapu@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616763002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
isherman@chromium.org changed reviewers: + palmer@chromium.org, sky@chromium.org
+sky for chrome/browser/bookmarks/bookmark_html_writer_unittest.cc +palmer for chrome/common/importer/profile_import_process_messages.h
https://codereview.chromium.org/616763002/diff/300001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/300001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:97: TemplateURL* CreateTemplateURL(const base::string16& url, Weakening the type of this from GURL to string seems like a bad idea. Why do this? https://codereview.chromium.org/616763002/diff/300001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/300001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:52: // |url| is a string instead of a GURL since it may not actually be a valid Hmm, aren't search engine URLs usually more like https://search.com/?q=%s ? I'd like to see some data validation, even if only validating that the origin us a valid URL. Is that possible? Also, minimum and maximum string lengths? Any lexical/syntactic requirements for |keyword| and |display_name|?
/bookmark_html_writer_unittest.cc LGTM Any reason this review isn't cc'ing chromium-reviews?
https://codereview.chromium.org/616763002/diff/300001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (right): https://codereview.chromium.org/616763002/diff/300001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:97: TemplateURL* CreateTemplateURL(const base::string16& url, On 2015/01/06 00:39:14, Chromium Palmer wrote: > Weakening the type of this from GURL to string seems like a bad idea. Why do > this? Because we need to be able to pass in strings that aren't valid URLs as-is. https://codereview.chromium.org/616763002/diff/300001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/616763002/diff/300001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:52: // |url| is a string instead of a GURL since it may not actually be a valid On 2015/01/06 00:39:14, Chromium Palmer wrote: > Hmm, aren't search engine URLs usually more like https://search.com/?q=%25s ? > > I'd like to see some data validation, even if only validating that the origin us > a valid URL. Is that possible? No, it's not. > Also, minimum and maximum string lengths? What do you mean? > Any lexical/syntactic requirements for |keyword| and |display_name|? No.
> > I'd like to see some data validation, even if only validating that the origin > us > > a valid URL. Is that possible? > > No, it's not. Why not? > > Also, minimum and maximum string lengths? > > What do you mean? > > > Any lexical/syntactic requirements for |keyword| and |display_name|? > > No. So an IPC caller can send 2 GiB of U+1F4A9 for the |display_name| and that is working as intended?
On 2015/01/06 00:40:03, sky wrote: > Any reason this review isn't cc'ing chromium-reviews? I think it was probably dropped by accident; I've restored it now. Tapu, please make sure that it wasn't originally missing due to a misconfiguration of your local checkout or tools.
On 2015/01/06 01:16:54, Chromium Palmer wrote:
> > > I'd like to see some data validation, even if only validating that the
> origin
> > us
> > > a valid URL. Is that possible?
> >
> > No, it's not.
>
> Why not?
Because, as I said, not all valid inputs are valid URLs, e.g. "{searchTerms}/".
> > > Also, minimum and maximum string lengths?
> >
> > What do you mean?
> >
> > > Any lexical/syntactic requirements for |keyword| and |display_name|?
> >
> > No.
>
> So an IPC caller can send 2 GiB of U+1F4A9 for the |display_name| and that is
> working as intended?
(1) Yes, I'm fine with that, it shouldn't cause problems AFAIK (other than
likely triggering OOM)
(2) More importantly, we're not changing anything about the keyword and display
name w.r.t. IPC in this CL, so why is trying to validate them a blocker for
landing this? If these need some kind of auditing or changing, I think that
should happen independently of this CL. We're renaming a struct (no effect) and
changing a member from GURL (which is a string + some metadata) to a string.
On 2015/01/06 01:22:09, Ilya Sherman wrote: > On 2015/01/06 00:40:03, sky wrote: > > Any reason this review isn't cc'ing chromium-reviews? > > I think it was probably dropped by accident; I've restored it now. Tapu, please > make sure that it wasn't originally missing due to a misconfiguration of your > local checkout or tools. Sure.
> (2) More importantly, we're not changing anything about the keyword and display > name w.r.t. IPC in this CL, so why is trying to validate them a blocker for > landing this? If these need some kind of auditing or changing, I think that > should happen independently of this CL. We're renaming a struct (no effect) and > changing a member from GURL (which is a string + some metadata) to a string. Weak typing/weak validation is 1 of the reasons IPC security review exists. Fast and loose IPC has embarrassed us before, so now we check. I fear that if some simple checks are not put in place now, they won't ever be.
On 2015/01/07 01:29:44, Chromium Palmer wrote: > > (2) More importantly, we're not changing anything about the keyword and > display > > name w.r.t. IPC in this CL, so why is trying to validate them a blocker for > > landing this? If these need some kind of auditing or changing, I think that > > should happen independently of this CL. We're renaming a struct (no effect) > and > > changing a member from GURL (which is a string + some metadata) to a string. > > Weak typing/weak validation is 1 of the reasons IPC security review exists. Fast > and loose IPC has embarrassed us before, so now we check. > > I fear that if some simple checks are not put in place now, they won't ever be. Do they even need to be? I'm not sure that the out-of-process importing codepath is even executed anymore. If it is, it doesn't seem like either: (a) This patch makes any potential problem worse, or (b) We're aware of any existing concrete problems here Given the above, and that this is an in-progress-for-three-months patch by an external contributor, I really don't think we should hold it up further. Can we address what concerns you have on a separate bug/patch?
OK. LGTM.
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616763002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/616763002/diff/300001/chrome/chrome_utility.gypi File chrome/chrome_utility.gypi (right): https://codereview.chromium.org/616763002/diff/300001/chrome/chrome_utility.g... chrome/chrome_utility.gypi:106: '../components/components.gyp:search_engines', You probably need to update the //chrome/utility/BUILD.gn file to match this change. Chrome currently has two build systems: GYP, which is the older system, and GN, which is the newer system that we're in the process of switching to. Unfortunately, that means that right now, we need to maintain both. https://codereview.chromium.org/616763002/diff/300001/chrome/common/importer/... File chrome/common/importer/importer_bridge.h (right): https://codereview.chromium.org/616763002/diff/300001/chrome/common/importer/... chrome/common/importer/importer_bridge.h:32: struct SearchEngineInfo; It looks like your Windows compilation failures are related to renaming this struct -- you'll need to update the IE importer to use this name as well.
PTAL. https://codereview.chromium.org/616763002/diff/280001/chrome/utility/importer... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/616763002/diff/280001/chrome/utility/importer... chrome/utility/importer/firefox_importer.cc:316: // engine, provided that its url is a valid search engine url. On 2014/12/12 01:04:42, Peter Kasting wrote: > Nit: How about: > > Import this bookmark as a search engine if it has a keyword and its URL is > usable as a search engine URL. (Even if the URL doesn't allow substitution, > importing as a "search engine" allows users to trigger the bookmark by entering > its keyword in the omnibox.) Acknowledged. https://codereview.chromium.org/616763002/diff/300001/chrome/chrome_utility.gypi File chrome/chrome_utility.gypi (right): https://codereview.chromium.org/616763002/diff/300001/chrome/chrome_utility.g... chrome/chrome_utility.gypi:106: '../components/components.gyp:search_engines', On 2015/01/07 22:51:26, Ilya Sherman wrote: > You probably need to update the //chrome/utility/BUILD.gn file to match this > change. Chrome currently has two build systems: GYP, which is the older system, > and GN, which is the newer system that we're in the process of switching to. > Unfortunately, that means that right now, we need to maintain both. Acknowledged. https://codereview.chromium.org/616763002/diff/300001/chrome/common/importer/... File chrome/common/importer/importer_bridge.h (right): https://codereview.chromium.org/616763002/diff/300001/chrome/common/importer/... chrome/common/importer/importer_bridge.h:32: struct SearchEngineInfo; On 2015/01/07 22:51:26, Ilya Sherman wrote: > It looks like your Windows compilation failures are related to renaming this > struct -- you'll need to update the IE importer to use this name as well. Acknowledged.
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616763002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This page might be useful for understanding and fixing the GN compilation failure: https://code.google.com/p/chromium/wiki/gn
Thanks Ilya. I am on it. On Fri, Jan 9, 2015 at 5:54 PM, <isherman@chromium.org> wrote: > This page might be useful for understanding and fixing the GN compilation > failure: https://code.google.com/p/chromium/wiki/gn > > https://codereview.chromium.org/616763002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I see the following compilation error (stacktrace) from log: ---------------- In file included from ../../chrome/utility/importer/bookmark_html_reader.cc:19: In file included from ../../components/search_engines/template_url.h:15: gen/components/metrics/proto/omnibox_event.pb.h:9:10: fatal error: 'google/protobuf/stubs/common.h' file not found --------------- However, I receive no compilation error in my local machine which is running 64 bit linux. I have re-ordered the dependency list in //chrome/utility/BUILF.gn such that it follows the same order that are presented in //chrome/chrome_utility.gypi. Since I cannot regenerate the error in my machine, its hard to debug. Committing the change. Lets see how it goes. In the mean time I will pass sometime on GN documentation suggested by Ilya. Thanks. ~ Tapu.
The CQ bit was checked by ghose.tapu@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616763002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
What command are you running to compile on your Linux machine? To test the GN build, you need to run: gn gen out/gn (once; you can pick a different directory other than out/gn) ninja -C out/gn chrome (to build the chrome target) In any case, I suspect that the issue is that //components/search_engines/BUILD.gn needs to list "//components/metrics/proto" as one of its public_deps, to match the corresponding .gypi file, which exports this dependency. This is because a public header file in the search_engines component depends on the protobuf definitions, which means that any file that #includes that header also inherits the dependency. On Mon, Jan 12, 2015 at 4:28 PM, <ghose.tapu@gmail.com> wrote: > I see the following compilation error (stacktrace) from log: > > ---------------- > In file included from ../../chrome/utility/importer/ > bookmark_html_reader.cc:19: > In file included from ../../components/search_engines/template_url.h:15: > gen/components/metrics/proto/omnibox_event.pb.h:9:10: fatal error: > 'google/protobuf/stubs/common.h' file not found > --------------- > > However, I receive no compilation error in my local machine which is > running 64 > bit linux. > > I have re-ordered the dependency list in //chrome/utility/BUILF.gn such > that it > follows the same order that are presented in //chrome/chrome_utility.gypi. > Since > I cannot regenerate the error in my machine, its hard to debug. Committing > the > change. Lets see how it goes. In the mean time I will pass sometime on GN > documentation suggested by Ilya. > > Thanks. > > ~ Tapu. > > > https://codereview.chromium.org/616763002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Usually, I run ninja -C out/Debug chrome for compiling. Yesterday, I did executed the following two commands: gn gen out/Default ninja -C out/Default gn Just now I executed "ninja -C out/Default chrome" and I guess it will take a while to finish the compilation. Thanks a lot Ilya. On Tue, Jan 13, 2015 at 9:12 PM, Ilya Sherman <isherman@chromium.org> wrote: > What command are you running to compile on your Linux machine? To test > the GN build, you need to run: > > gn gen out/gn (once; you can pick a different directory other than out/gn) > ninja -C out/gn chrome (to build the chrome target) > > In any case, I suspect that the issue is that > //components/search_engines/BUILD.gn needs to list > "//components/metrics/proto" as one of its public_deps, to match the > corresponding .gypi file, which exports this dependency. This is because a > public header file in the search_engines component depends on the protobuf > definitions, which means that any file that #includes that header also > inherits the dependency. > > On Mon, Jan 12, 2015 at 4:28 PM, <ghose.tapu@gmail.com> wrote: > >> I see the following compilation error (stacktrace) from log: >> >> ---------------- >> In file included from ../../chrome/utility/importer/ >> bookmark_html_reader.cc:19: >> In file included from ../../components/search_engines/template_url.h:15: >> gen/components/metrics/proto/omnibox_event.pb.h:9:10: fatal error: >> 'google/protobuf/stubs/common.h' file not found >> --------------- >> >> However, I receive no compilation error in my local machine which is >> running 64 >> bit linux. >> >> I have re-ordered the dependency list in //chrome/utility/BUILF.gn such >> that it >> follows the same order that are presented in >> //chrome/chrome_utility.gypi. Since >> I cannot regenerate the error in my machine, its hard to debug. >> Committing the >> change. Lets see how it goes. In the mean time I will pass sometime on GN >> documentation suggested by Ilya. >> >> Thanks. >> >> ~ Tapu. >> >> >> https://codereview.chromium.org/616763002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Illya, Seems like "//components/metrics/proto" is already included as one of public_deps in "//components/search_engines/BUILD.gn". After executing "ninja -C out/Default chrome" I got to see the same error message as we are seeing in try-bot. However, I am not seeing the error any more once I included "//components/metrics/proto" as one of the public_deps in "//chrome/utility/BUILD.gn". But my machine is hanging up while linking ./chrome. I was wondering should I commit my change and give a try in try-bots. ~ Tapu. On Tue, Jan 13, 2015 at 9:20 PM, Tapu Ghose <ghose.tapu@gmail.com> wrote: > Usually, I run > > ninja -C out/Debug chrome > > for compiling. Yesterday, I did executed the following two commands: > > gn gen out/Default > ninja -C out/Default gn > > Just now I executed "ninja -C out/Default chrome" and I guess it will take > a while to finish the compilation. > > Thanks a lot Ilya. > > On Tue, Jan 13, 2015 at 9:12 PM, Ilya Sherman <isherman@chromium.org> > wrote: > >> What command are you running to compile on your Linux machine? To test >> the GN build, you need to run: >> >> gn gen out/gn (once; you can pick a different directory other than >> out/gn) >> ninja -C out/gn chrome (to build the chrome target) >> >> In any case, I suspect that the issue is that >> //components/search_engines/BUILD.gn needs to list >> "//components/metrics/proto" as one of its public_deps, to match the >> corresponding .gypi file, which exports this dependency. This is because a >> public header file in the search_engines component depends on the protobuf >> definitions, which means that any file that #includes that header also >> inherits the dependency. >> >> On Mon, Jan 12, 2015 at 4:28 PM, <ghose.tapu@gmail.com> wrote: >> >>> I see the following compilation error (stacktrace) from log: >>> >>> ---------------- >>> In file included from ../../chrome/utility/importer/ >>> bookmark_html_reader.cc:19: >>> In file included from ../../components/search_engines/template_url.h:15: >>> gen/components/metrics/proto/omnibox_event.pb.h:9:10: fatal error: >>> 'google/protobuf/stubs/common.h' file not found >>> --------------- >>> >>> However, I receive no compilation error in my local machine which is >>> running 64 >>> bit linux. >>> >>> I have re-ordered the dependency list in //chrome/utility/BUILF.gn such >>> that it >>> follows the same order that are presented in >>> //chrome/chrome_utility.gypi. Since >>> I cannot regenerate the error in my machine, its hard to debug. >>> Committing the >>> change. Lets see how it goes. In the mean time I will pass sometime on GN >>> documentation suggested by Ilya. >>> >>> Thanks. >>> >>> ~ Tapu. >>> >>> >>> https://codereview.chromium.org/616763002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
(Seems I forgot to send this earlier:) https://codereview.chromium.org/616763002/diff/340001/chrome/utility/BUILD.gn File chrome/utility/BUILD.gn (right): https://codereview.chromium.org/616763002/diff/340001/chrome/utility/BUILD.gn... chrome/utility/BUILD.gn:21: "//components/search_engines", nit: Please keep this list alphabetized.
On Thu, Jan 15, 2015 at 4:44 PM, Tapu Ghose <ghose.tapu@gmail.com> wrote: > Hi Illya, > > Seems like "//components/metrics/proto" is already included as one of > public_deps in "//components/search_engines/BUILD.gn". > Could you link me to the location where "//components/metrics/proto" is listed as one of the public_deps in //components/search_engines/BUILD.gn? I see it listed as one of the deps, but not as one of the public_deps. > After executing "ninja -C out/Default chrome" I got to see the same error > message as we are seeing in try-bot. However, I am not seeing the error any > more once I included "//components/metrics/proto" as one of the public_deps > in "//chrome/utility/BUILD.gn". But my machine is hanging up while linking > ./chrome. I was wondering should I commit my change and give a try in > try-bots. > > ~ Tapu. > > > On Tue, Jan 13, 2015 at 9:20 PM, Tapu Ghose <ghose.tapu@gmail.com> wrote: > >> Usually, I run >> >> ninja -C out/Debug chrome >> >> for compiling. Yesterday, I did executed the following two commands: >> >> gn gen out/Default >> ninja -C out/Default gn >> >> Just now I executed "ninja -C out/Default chrome" and I guess it will >> take a while to finish the compilation. >> >> Thanks a lot Ilya. >> >> On Tue, Jan 13, 2015 at 9:12 PM, Ilya Sherman <isherman@chromium.org> >> wrote: >> >>> What command are you running to compile on your Linux machine? To test >>> the GN build, you need to run: >>> >>> gn gen out/gn (once; you can pick a different directory other than >>> out/gn) >>> ninja -C out/gn chrome (to build the chrome target) >>> >>> In any case, I suspect that the issue is that >>> //components/search_engines/BUILD.gn needs to list >>> "//components/metrics/proto" as one of its public_deps, to match the >>> corresponding .gypi file, which exports this dependency. This is because a >>> public header file in the search_engines component depends on the protobuf >>> definitions, which means that any file that #includes that header also >>> inherits the dependency. >>> >>> On Mon, Jan 12, 2015 at 4:28 PM, <ghose.tapu@gmail.com> wrote: >>> >>>> I see the following compilation error (stacktrace) from log: >>>> >>>> ---------------- >>>> In file included from ../../chrome/utility/importer/ >>>> bookmark_html_reader.cc:19: >>>> In file included from ../../components/search_ >>>> engines/template_url.h:15: >>>> gen/components/metrics/proto/omnibox_event.pb.h:9:10: fatal error: >>>> 'google/protobuf/stubs/common.h' file not found >>>> --------------- >>>> >>>> However, I receive no compilation error in my local machine which is >>>> running 64 >>>> bit linux. >>>> >>>> I have re-ordered the dependency list in //chrome/utility/BUILF.gn such >>>> that it >>>> follows the same order that are presented in >>>> //chrome/chrome_utility.gypi. Since >>>> I cannot regenerate the error in my machine, its hard to debug. >>>> Committing the >>>> change. Lets see how it goes. In the mean time I will pass sometime on >>>> GN >>>> documentation suggested by Ilya. >>>> >>>> Thanks. >>>> >>>> ~ Tapu. >>>> >>>> >>>> https://codereview.chromium.org/616763002/ >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL. https://codereview.chromium.org/616763002/diff/340001/chrome/utility/BUILD.gn File chrome/utility/BUILD.gn (right): https://codereview.chromium.org/616763002/diff/340001/chrome/utility/BUILD.gn... chrome/utility/BUILD.gn:21: "//components/search_engines", On 2015/01/16 02:14:20, Ilya Sherman wrote: > nit: Please keep this list alphabetized. Acknowledged.
The CQ bit was checked by ghose.tapu@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616763002/360001
BUILD file changes LGTM -- thanks :)
Message was sent while issue was closed.
Committed patchset #13 (id:360001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/4183aa2da39c20f9e49f7e9bd51f08544c36af83 Cr-Commit-Position: refs/heads/master@{#311834}
Message was sent while issue was closed.
Happy to see that the build passed in all try-bots. I would like to thank all of you for making out some time in reviewing the patch sets. Ilya and Peter, I appreciate your patience in answering my queries and guiding me in resolving the issue. Looking forward to work together again. ~ Tapu. On Thu, Jan 15, 2015 at 11:23 PM, <commit-bot@chromium.org> wrote: > Patchset 13 (id:??) landed as > https://crrev.com/4183aa2da39c20f9e49f7e9bd51f08544c36af83 > Cr-Commit-Position: refs/heads/master@{#311834} > > https://codereview.chromium.org/616763002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Glad you finally were able to get the CL into a happy state. Thanks for your persistent efforts :) Cheers, ~ilya On Thu, Jan 15, 2015 at 8:33 PM, Tapu Ghose <ghose.tapu@gmail.com> wrote: > Happy to see that the build passed in all try-bots. I would like to thank > all of you for making out some time in reviewing the patch sets. > > Ilya and Peter, > > I appreciate your patience in answering my queries and guiding me in > resolving the issue. Looking forward to work together again. > > ~ Tapu. > > On Thu, Jan 15, 2015 at 11:23 PM, <commit-bot@chromium.org> wrote: > >> Patchset 13 (id:??) landed as >> https://crrev.com/4183aa2da39c20f9e49f7e9bd51f08544c36af83 >> Cr-Commit-Position: refs/heads/master@{#311834} >> >> https://codereview.chromium.org/616763002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
