Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1873403003:
Read bookmarks from 'NETSCAPE-Bookmark-file's that don't have a beginning <DT> tag. (Closed)
|
Created:
4 years, 8 months ago by Tom (Use chromium acct) Modified:
4 years, 8 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, tfarina, gab Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRead bookmarks from 'NETSCAPE-Bookmark-file's that don't have a beginning <DT> tag.
BUG=601677
Committed: https://crrev.com/fc21e5d1a95a9607ca3375b7e09521a95cc4a306
Cr-Commit-Position: refs/heads/master@{#387618}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed failing tests #
Total comments: 4
Patch Set 3 : Reverted to old interface #
Messages
Total messages: 29 (14 generated)
thomasanderson@google.com changed reviewers: + gab@chromium.org
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1873403003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1873403003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
gab@chromium.org changed reviewers: + isherman@chromium.org
I'm mostly familiar with the import architecture in Chrome but not the importers' logic themselves, Ilya is the right person for that. Redirected, thanks!
Description was changed from ========== Read bookmarks from 'NETSCAPE-Bookmark-file's that don't have a beginning <DT> tag. BUG=601677 ========== to ========== Read bookmarks from 'NETSCAPE-Bookmark-file's that don't have a beginning <DT> tag. BUG=601677 ==========
gab@chromium.org changed reviewers: - gab@chromium.org
LGTM % a nit. Thanks! https://codereview.chromium.org/1873403003/diff/1/chrome/utility/importer/boo... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/1873403003/diff/1/chrome/utility/importer/boo... chrome/utility/importer/bookmark_html_reader.cc:137: base::CompareCase::INSENSITIVE_ASCII)) { nit: This indentation looks a bit off to me. Could you please run clang-format to double-check?
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1873403003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1873403003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Made some interface changes to get tests passing. Pinging Ilya to verify the deltas https://codereview.chromium.org/1873403003/diff/1/chrome/utility/importer/boo... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/1873403003/diff/1/chrome/utility/importer/boo... chrome/utility/importer/bookmark_html_reader.cc:137: base::CompareCase::INSENSITIVE_ASCII)) { On 2016/04/13 00:11:11, Ilya Sherman wrote: > nit: This indentation looks a bit off to me. Could you please run clang-format > to double-check? Done.
https://codereview.chromium.org/1873403003/diff/20001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/1873403003/diff/20001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:313: bool ParseFolderNameFromLine(std::string line, Even if you want to modify the variable in the method implementation, it's preferred to pass the string by const-ref for the method signature, and then make a copy inline, as needed. This way, the interface does not change even if the implementation changes to no longer need the copy; and moreover people reading the interface don't spend any time wondering why one of the strings is passed by const-ref, and the other by value. https://codereview.chromium.org/1873403003/diff/20001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:323: stripDt(line); Hmm, why do you need to also strip DT tags here, rather than just in the spot where HRs are stripped?
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/1873403003/diff/20001/chrome/utility/importer... File chrome/utility/importer/bookmark_html_reader.cc (right): https://codereview.chromium.org/1873403003/diff/20001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:313: bool ParseFolderNameFromLine(std::string line, On 2016/04/14 02:22:53, Ilya Sherman wrote: > Even if you want to modify the variable in the method implementation, it's > preferred to pass the string by const-ref for the method signature, and then > make a copy inline, as needed. This way, the interface does not change even if > the implementation changes to no longer need the copy; and moreover people > reading the interface don't spend any time wondering why one of the strings is > passed by const-ref, and the other by value. Done. https://codereview.chromium.org/1873403003/diff/20001/chrome/utility/importer... chrome/utility/importer/bookmark_html_reader.cc:323: stripDt(line); On 2016/04/14 02:22:53, Ilya Sherman wrote: > Hmm, why do you need to also strip DT tags here, rather than just in the spot > where HRs are stripped? I removed the code that strips the DT near where were strip the HR because some unit tests were failing that expected the Parse*FromLine functions to work with a leading <DT>.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1873403003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1873403003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1873403003/#ps40001 (title: "Reverted to old interface")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1873403003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1873403003/40001
Message was sent while issue was closed.
Description was changed from ========== Read bookmarks from 'NETSCAPE-Bookmark-file's that don't have a beginning <DT> tag. BUG=601677 ========== to ========== Read bookmarks from 'NETSCAPE-Bookmark-file's that don't have a beginning <DT> tag. BUG=601677 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Read bookmarks from 'NETSCAPE-Bookmark-file's that don't have a beginning <DT> tag. BUG=601677 ========== to ========== Read bookmarks from 'NETSCAPE-Bookmark-file's that don't have a beginning <DT> tag. BUG=601677 Committed: https://crrev.com/fc21e5d1a95a9607ca3375b7e09521a95cc4a306 Cr-Commit-Position: refs/heads/master@{#387618} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fc21e5d1a95a9607ca3375b7e09521a95cc4a306 Cr-Commit-Position: refs/heads/master@{#387618} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
