Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22408007:
Remove "<HR>" tags to import Firefox bookmarks correctly. (Closed)
|
Created:
7 years, 4 months ago by zhchbin Modified:
7 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRemove "<HR>" tags to import Firefox bookmarks correctly.
BUG=257474
TEST=1. Go to chrome://bookmarks/#1;
2. Organise->Import Bookmarks from HTML file;
3. Select the HTML file which exports from bookmarks of Firefox.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217205
Patch Set 1 #
Total comments: 4
Patch Set 2 : Nits fix and add a test case. #
Total comments: 13
Patch Set 3 : Revise test cases. #
Total comments: 3
Patch Set 4 : Update comment. #
Total comments: 10
Patch Set 5 : Revise as comments. #
Total comments: 2
Patch Set 6 : Update comment. #
Messages
Total messages: 20 (0 generated)
Please review.
isherman would be a better reviewer than me for this. Please also add a test to cover this. Cheers, Gab
Please, when you start a patch related to a bug on crbug.com/, say so on crbug.com. I had started this work already, so our work are overlapping. I have even discussed this with Ilya on the bug. Thanks!
On 2013/08/08 16:00:03, tfarina wrote: > Please, when you start a patch related to a bug on crbug.com/, say so on > http://crbug.com. I had started this work already, so our work are overlapping. I have > even discussed this with Ilya on the bug. > > Thanks! Oops, sorry for the trouble. Should I close this CL?
On 2013/08/08 16:09:06, zhchbin wrote: > On 2013/08/08 16:00:03, tfarina wrote: > > Please, when you start a patch related to a bug on crbug.com/, say so on > > http://crbug.com. I had started this work already, so our work are > overlapping. I have > > even discussed this with Ilya on the bug. > > > > Thanks! > > Oops, sorry for the trouble. Should I close this CL? No, go ahead with this since you already sent for review. Just remember for the next time. Thanks,
Please add a test for this. Otherwise, this change looks good -- thanks! Please note that I'll be out of town for two weeks starting in a few hours, so you'll need to find another reviewer to grant you the final approval if you don't want to wait that long for a review :) https://codereview.chromium.org/22408007/diff/1/chrome/utility/importer/bookm... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/1/chrome/utility/importer/bookm... chrome/utility/importer/bookmark_html_reader.cc:113: TrimString(lines[i], " ", &line); nit: Please leave a blank line after this one. https://codereview.chromium.org/22408007/diff/1/chrome/utility/importer/bookm... chrome/utility/importer/bookmark_html_reader.cc:116: TrimString(line.substr(4), " ", &line); Optional nit: I'd write this as follows, so that the '4' is not such a magical constant: const std::string kHrTag = "<HR>"; if (StartsWithASCII(line, kHrTag, false)) TrimString(line.substr(kHrTag.size()), " ", &line);
@gab. Could you please take a look? A test case is added. @Ilya, thanks for the review and have a nice vacation. @tfarina, Sorry again for the trouble, I will remember next time. https://codereview.chromium.org/22408007/diff/1/chrome/utility/importer/bookm... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/1/chrome/utility/importer/bookm... chrome/utility/importer/bookmark_html_reader.cc:113: TrimString(lines[i], " ", &line); On 2013/08/09 01:26:18, Ilya Sherman (Away Aug. 9-25) wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/22408007/diff/1/chrome/utility/importer/bookm... chrome/utility/importer/bookmark_html_reader.cc:116: TrimString(line.substr(4), " ", &line); On 2013/08/09 01:26:18, Ilya Sherman (Away Aug. 9-25) wrote: > Optional nit: I'd write this as follows, so that the '4' is not such a magical > constant: > > const std::string kHrTag = "<HR>"; > if (StartsWithASCII(line, kHrTag, false)) > TrimString(line.substr(kHrTag.size()), " ", &line); Done.
Comments below. Cheers! Gab https://codereview.chromium.org/22408007/diff/9001/chrome/test/data/bookmark_... File chrome/test/data/bookmark_html_reader/firefox3.html (right): https://codereview.chromium.org/22408007/diff/9001/chrome/test/data/bookmark_... chrome/test/data/bookmark_html_reader/firefox3.html:13: <HR> <DT><H3 ADD_DATE="1360718511" LAST_MODIFIED="1360718511">Internet</H3> There is no matching </HR>? What is this format?! Does this file come straight from FF? If so, is it really from FF3?! If not, we should probably name the file accordingly. https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader.cc:95: const std::string kHrTag = "<HR>"; All string constants should be of the form: static const char kFoo[] = "..."; so that they live in ro-data in the binary. Also the constant should be as close as possible to the code that uses it (i.e., between 116/117 below). https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader.cc:118: TrimString(line.substr(kHrTag.size()), " ", &line); use "arraysize(kHrTag)" here instead of kHrTag.size() since it will now be a char array. https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader.cc:118: TrimString(line.substr(kHrTag.size()), " ", &line); This line is slightly tricky and it creates a new string which I think can be avoided by splitting the logic as: line.erase(0, arraysize(kHrTag)) TrimString(line, " ", &line); which on top of being more explicit avoids creating a new string (which the substr command otherwise does). https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader_unittest.cc:182: TEST(BookmarkHTMLReaderTest, Firefox2BookmarkFileImport) { Make all the BookmarkHTMLReaderTest here and below test fixtures, e.g. something like: class BookmarkHTMLReaderTestWithData : public ::testing::Test { public: virtual void SetUp() OVERRIDE { ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_path_)); test_data_path_ = test_data_path_.AppendASCII("bookmark_html_reader"); } protected: base::FilePath test_data_path_; }; And then change all tests here and below from TEST(BookmarkHTMLReaderTest, ...) to TEST_F(BookmarkHTMLReaderTestWithData, ...) Instances of |path| should be replaced with |test_data_path_| in every test. This avoids repeating the initialization code that all of these tests have in common. Furthermore, all of the methods in the unnamed namespace above can then move as protected methods of the test fixture. (note that BookmarkHTMLReaderTest.ParseTests can remain a TEST with no fixture since it requires no initialization) https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader_unittest.cc:210: ASSERT_EQ(7U, bookmarks.size()); Can you also verify the bookmarks' contents like the other tests do.
On Fri, Aug 9, 2013 at 10:27 AM, <gab@chromium.org> wrote: > (note that BookmarkHTMLReaderTest.ParseTests can remain a TEST with no > fixture since it requires no initialization) > I doubt gtest will let will mix TEST and TEST_F with the same name BookmarkHTMLReaderTest. But let's him try and see if it works ;) > https://codereview.chromium.org/22408007/ -- Thiago
@gab please take a look at my comments below. I will update this cl tomorrow because I have to leave lab in my school. (now is 22:00) @tfarina, I will take have a try. https://codereview.chromium.org/22408007/diff/9001/chrome/test/data/bookmark_... File chrome/test/data/bookmark_html_reader/firefox3.html (right): https://codereview.chromium.org/22408007/diff/9001/chrome/test/data/bookmark_... chrome/test/data/bookmark_html_reader/firefox3.html:13: <HR> <DT><H3 ADD_DATE="1360718511" LAST_MODIFIED="1360718511">Internet</H3> On 2013/08/09 13:27:30, gab wrote: > There is no matching </HR>? What is this format?! In HTML, the <hr> tag has no end tag. > > Does this file come straight from FF? > > If so, is it really from FF3?! If not, we should probably name the file > accordingly. Oops, I thought the "2" in "firefox2.html" is a magic number, so I just follow the way.... This file is exported from Firefox. https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader.cc:118: TrimString(line.substr(kHrTag.size()), " ", &line); On 2013/08/09 13:27:30, gab wrote: > This line is slightly tricky and it creates a new string which I think can be > avoided by splitting the logic as: > > line.erase(0, arraysize(kHrTag)) > TrimString(line, " ", &line); > > which on top of being more explicit avoids creating a new string (which the > substr command otherwise does). Can you accept line.erase(0, arraysize(kHrTag) - 1); arraysize(kHrTag) is 5. https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader_unittest.cc:182: TEST(BookmarkHTMLReaderTest, Firefox2BookmarkFileImport) { On 2013/08/09 13:27:30, gab wrote: > Make all the BookmarkHTMLReaderTest here and below test fixtures, e.g. something > like: > > class BookmarkHTMLReaderTestWithData : public ::testing::Test { > public: > virtual void SetUp() OVERRIDE { > ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_path_)); > test_data_path_ = test_data_path_.AppendASCII("bookmark_html_reader"); > } > > protected: > base::FilePath test_data_path_; > }; > > And then change all tests here and below from > TEST(BookmarkHTMLReaderTest, ...) > to > TEST_F(BookmarkHTMLReaderTestWithData, ...) > > Instances of |path| should be replaced with |test_data_path_| in every test. > > This avoids repeating the initialization code that all of these tests have in > common. > > Furthermore, all of the methods in the unnamed namespace above can then move as > protected methods of the test fixture. > > > (note that BookmarkHTMLReaderTest.ParseTests can remain a TEST with no fixture > since it requires no initialization) I will finish it as soon as I can. Thanks for the instruction. https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader_unittest.cc:210: ASSERT_EQ(7U, bookmarks.size()); On 2013/08/09 13:27:30, gab wrote: > Can you also verify the bookmarks' contents like the other tests do. I mean to keep this test case simple and only testing the size seems already enough for this case. However, if we need it I will add.
Sounds good, thanks for doing this! https://codereview.chromium.org/22408007/diff/9001/chrome/test/data/bookmark_... File chrome/test/data/bookmark_html_reader/firefox3.html (right): https://codereview.chromium.org/22408007/diff/9001/chrome/test/data/bookmark_... chrome/test/data/bookmark_html_reader/firefox3.html:13: <HR> <DT><H3 ADD_DATE="1360718511" LAST_MODIFIED="1360718511">Internet</H3> On 2013/08/09 14:20:00, zhchbin wrote: > On 2013/08/09 13:27:30, gab wrote: > > There is no matching </HR>? What is this format?! > > In HTML, the <hr> tag has no end tag. Ah ok, thanks, sorry HTML noob here :). > > > > > Does this file come straight from FF? > > > > If so, is it really from FF3?! If not, we should probably name the file > > accordingly. > > Oops, I thought the "2" in "firefox2.html" is a magic number, so I just follow > the way.... > > This file is exported from Firefox. Ok sg, please rename file accordingly. https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader.cc:118: TrimString(line.substr(kHrTag.size()), " ", &line); On 2013/08/09 14:20:00, zhchbin wrote: > On 2013/08/09 13:27:30, gab wrote: > > This line is slightly tricky and it creates a new string which I think can be > > avoided by splitting the logic as: > > > > line.erase(0, arraysize(kHrTag)) > > > TrimString(line, " ", &line); > > > > which on top of being more explicit avoids creating a new string (which the > > substr command otherwise does). > > Can you accept > > line.erase(0, arraysize(kHrTag) - 1); > > arraysize(kHrTag) is 5. Ah, right, yes, because the actual arraysize includes the terminating-NULL character. Good catch. https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/22408007/diff/9001/chrome/utility/importer/bo... chrome/utility/importer/bookmark_html_reader_unittest.cc:210: ASSERT_EQ(7U, bookmarks.size()); On 2013/08/09 14:20:00, zhchbin wrote: > On 2013/08/09 13:27:30, gab wrote: > > Can you also verify the bookmarks' contents like the other tests do. > > I mean to keep this test case simple and only testing the size seems already > enough for this case. However, if we need it I will add. Well, from knowing the implementation, we know that this only makes sure it works with the <HR> tags, but this test doesn't need to know that and should check that the full bookmarks were imported as expected. Feel free to export a smaller test file than 7 bookmarks to make for the smallest test possible that has full coverage.
I have updated this cl as your comments. PTAL. @tfarina, gtest didn't allow test cases to mix TEST and TEST_F with the same name. It can compile but fail when running with the following message. All tests in the same test case must use the same test fixture class, so mixing TEST_F and TEST in the same test case is illegal. In test case BookmarkHTMLReaderTest, test Test is defined using TEST_F but test ParseTests is defined using TEST. You probably want to change the TEST to TEST_F or move it to another test case. https://codereview.chromium.org/22408007/diff/20001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/20001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader.cc:117: while (StartsWithASCII(line, kHrTag, false)) { Note this, I have change it from "if" to "while". Actually speaking, "<HR>" is the "separator" in the firefox. I have upload a video[0] to show it. My test case file "firefox23.html" is also exported with these bookmarks. [0]: https://docs.google.com/file/d/0B_7w63Sy3AuycWlfTmhvanZLUEE/edit?usp=sharing
https://codereview.chromium.org/22408007/diff/20001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/20001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader.cc:117: while (StartsWithASCII(line, kHrTag, false)) { On 2013/08/10 06:06:02, zhchbin wrote: > Note this, I have change it from "if" to "while". Actually speaking, "<HR>" is > the "separator" in the firefox. I have upload a video[0] to show it. My test > case file "firefox23.html" is also exported with these bookmarks. > > [0]: > https://docs.google.com/file/d/0B_7w63Sy3AuycWlfTmhvanZLUEE/edit?usp=sharing Could also note in the comments that we are removing/skipping the <HR> tag because it's the separator in Firefox that Chrome does not support?
https://codereview.chromium.org/22408007/diff/20001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/20001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader.cc:117: while (StartsWithASCII(line, kHrTag, false)) { On 2013/08/10 14:30:51, tfarina wrote: > On 2013/08/10 06:06:02, zhchbin wrote: > > Note this, I have change it from "if" to "while". Actually speaking, "<HR>" is > > the "separator" in the firefox. I have upload a video[0] to show it. My test > > case file "firefox23.html" is also exported with these bookmarks. > > > > [0]: > > https://docs.google.com/file/d/0B_7w63Sy3AuycWlfTmhvanZLUEE/edit?usp=sharing > Could also note in the comments that we are removing/skipping the <HR> tag > because it's the separator in Firefox that Chrome does not support? Done.
Looks good, last comments below. Cheers! Gab https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader.cc:116: // bookmark entrys in Firefox that Chrome does not support. I would rephrase the last sentence as: "<HR>" is the bookmark separator in Firefox that Chrome does not support (otherwise it makes it feel like its the "bookmark entries" we don't support...) WDYT? https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader_unittest.cc (left): https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader_unittest.cc:217: class CancelAfterFifteenCalls { I like to have this beside the test that uses it... I'd leave this here. https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader_unittest.cc:251: bool IsURLValid(const GURL& url) { I like to have this beside the test that uses it... I'd leave this here. https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader_unittest.cc:137: class BookmarkHTMLReaderTestWithData : public testing::Test { Put the fixture in the unnamed namespace. https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader_unittest.cc:139: BookmarkHTMLReaderTestWithData() {} Don't need this empty constructor.
https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader.cc:116: // bookmark entrys in Firefox that Chrome does not support. On 2013/08/12 14:17:41, gab wrote: > I would rephrase the last sentence as: > > "<HR>" is the bookmark separator in Firefox that Chrome does not support > > (otherwise it makes it feel like its the "bookmark entries" we don't support...) > > WDYT? Done. Sorry for my poor English. https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader_unittest.cc (left): https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader_unittest.cc:217: class CancelAfterFifteenCalls { On 2013/08/12 14:17:41, gab wrote: > I like to have this beside the test that uses it... I'd leave this here. Done. https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader_unittest.cc:251: bool IsURLValid(const GURL& url) { On 2013/08/12 14:17:41, gab wrote: > I like to have this beside the test that uses it... I'd leave this here. Done. https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader_unittest.cc (right): https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader_unittest.cc:137: class BookmarkHTMLReaderTestWithData : public testing::Test { On 2013/08/12 14:17:41, gab wrote: > Put the fixture in the unnamed namespace. Done. https://codereview.chromium.org/22408007/diff/26001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader_unittest.cc:139: BookmarkHTMLReaderTestWithData() {} On 2013/08/12 14:17:41, gab wrote: > Don't need this empty constructor. Done.
lgtm w/ comment change below. Cheers! Gab https://codereview.chromium.org/22408007/diff/32001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/32001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader.cc:119: while (StartsWithASCII(line, kHrTag, false)) { Just noticed this change to a while loop (and saw your previous comment about it), please update this comment to reflect this), something like: // Remove "<HR>" if |line| starts with it. "<HR>" is the bookmark entries // separator in Firefox that Chrome does not support. Note that there can be // multiple "<HR>" tags at the beginning of a single line. // See http://crbug.com/257474.
https://codereview.chromium.org/22408007/diff/32001/chrome/utility/importer/b... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/22408007/diff/32001/chrome/utility/importer/b... chrome/utility/importer/bookmark_html_reader.cc:119: while (StartsWithASCII(line, kHrTag, false)) { On 2013/08/12 16:28:22, gab wrote: > Just noticed this change to a while loop (and saw your previous comment about > it), please update this comment to reflect this), something like: > > > // Remove "<HR>" if |line| starts with it. "<HR>" is the bookmark entries > // separator in Firefox that Chrome does not support. Note that there can be > // multiple "<HR>" tags at the beginning of a single line. > // See http://crbug.com/257474. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/22408007/37001
Message was sent while issue was closed.
Change committed as 217205 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
