Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1589483002:
[Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords (Closed)
|
Created:
4 years, 11 months ago by kolos1 Modified:
4 years, 8 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords
BUG=593027
Committed: https://crrev.com/62ad5a8fc17197b1383df48afa0dda9654202d3f
Cr-Commit-Position: refs/heads/master@{#384524}
Patch Set 1 : #Patch Set 2 : #
Total comments: 41
Patch Set 3 : Changes addressed to reviewer comments #Patch Set 4 : Use non-hashed passwords, StringPiece in SplitByDotAndReverse #
Total comments: 4
Patch Set 5 : Removed mentions of hashing in comments, removed include "base/sha1.h" #
Total comments: 2
Patch Set 6 : Mistype in a comment #
Depends on Patchset: Messages
Total messages: 36 (20 generated)
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Description was changed from ========== [Password Manager] Implements entries sorting and duplicates removing on chrome://settings/passwords ========== to ========== [Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords ==========
kolos@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, Please review this CL that introduces entries sorting and duplicates omitting on chrome://settings/passwords Regards, Maxim
Hi Maxim, Please add a bug number to the CL description. Otherwise LGTM with some comments. Cheers, Vaclav https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:50: const int kAndroidAppSchemeAndDelimeterLength = 10; // Length of 'android://'. typo: delimeter -> delimiter https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:55: std::string SplitByDotAndReverse(const std::string& host) { To improve efficiency, consider passing a StringPiece instead of a const std::string& -- this allows you to pass in substrings (like on line 89) cheaply, without string copying. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:75: // If |username_and_password_in_key == true|, the key is appended with username nit: To avoid misunderstanding of what is appended to what, I suggest replacing "the key ... password" with "username and hashed password is appended to the key". https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:78: std::string CreateSortKey(const scoped_ptr<autofill::PasswordForm>& form, Please replace the const scoped_ptr with just a raw pointer. See also: http://dev.chromium.org/developers/coding-style#TOC-Object-ownership-and-call... https://www.chromium.org/developers/smart-pointer-guidelines https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:99: if (username_and_password_in_key) nit: Add { and }, this is not a one-liner. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:113: void RemoveDuplicates(const scoped_ptr<autofill::PasswordForm>& form, Also here: raw pointer instead of a scoped_ptr. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:123: for (autofill::DuplicatesMap::iterator it = dups.first; it != dups.second; nit: Use range-based loop, it's shorter and easier to read: for (const auto& duplicate : dups) {...} https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:23: using DuplicatesMap = Please pull DuplicatesMap out of the autofill namespace, there is no reason to put it in there. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:24: std::multimap<std::string, scoped_ptr<autofill::PasswordForm>>; #include <ma> for multimap https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:81: // there is several forms with the same key, the duplicates are moved from nit: To clarify that "duplicates" exclude the first occurrence, consider rephrasing: "...the same key, all such forms but the first one are stored in |duplicates| instead of |list|. I would omit "(the multimap from sort key to forms)" and instead note next to the definition of DuplicatesMap that the std::string is a sort key. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:83: // |username_and_password_in_key| defines whether username and hashed The last sentence is not completely clear -- from the above comment it looks like the username and the hashed comment are always included. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:88: autofill::DuplicatesMap* duplicates_, nit: Remove the trailing underscore in "duplicates_", this is not a member variable. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:130: int number_of_entries, int -> size_t https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:134: for (int i = 0; i < number_of_entries; i++) { int -> size_t https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:135: const SortEntry entry = test_entries[i]; Make this a reference to avoid copying. Also on line 159. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:163: << "position in sorted list: " << entry.expected_position; To avoid code duplication, use SCOPED_TRACE(testing::Message("position in sorted list: ") << entry.expected_position); That will append this information automatically to any failed EXPECT_* message.
Actually, one more question below. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:102: base::SHA1HashString(base::UTF16ToUTF8(form->password_value)); Why do we actually hash the passwords? The keys not sent to the renderer, are they?
Great thanks for your comments, Vaclav. Please have a look at changes. Regards, Maxim https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:50: const int kAndroidAppSchemeAndDelimeterLength = 10; // Length of 'android://'. On 2016/03/08 13:09:28, vabr (Chromium) wrote: > typo: delimeter -> delimiter Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:55: std::string SplitByDotAndReverse(const std::string& host) { Yes, it would be nice to replace std::string with StringPiece, but there is no JoinString for vector<StringPiece> (which is weird). So, we will have to convert every StringPiece to std::string and then join them, i.e. there is no sense to pass StringPiece, as I understand. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:75: // If |username_and_password_in_key == true|, the key is appended with username On 2016/03/08 13:09:28, vabr (Chromium) wrote: > nit: To avoid misunderstanding of what is appended to what, I suggest replacing > "the key ... password" with "username and hashed password is appended to the > key". Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:78: std::string CreateSortKey(const scoped_ptr<autofill::PasswordForm>& form, Since we don't modify |form|, I redeclared it as const ref. Did the same in RemoveDuplicates. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:99: if (username_and_password_in_key) On 2016/03/08 13:09:28, vabr (Chromium) wrote: > nit: Add { and }, this is not a one-liner. Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:102: base::SHA1HashString(base::UTF16ToUTF8(form->password_value)); Lets somebody want to see one of passwords listed on password page, but he doesn't know admin password. Lets he is able to add dummy password entries with specified passwords. Then, based on sorting results of dummy entries and the real entry, it is possible to know the real password. In few words, sorting shows passwords implicitly. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:113: void RemoveDuplicates(const scoped_ptr<autofill::PasswordForm>& form, Since we don't modify form, redeclared as const ref. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:123: for (autofill::DuplicatesMap::iterator it = dups.first; it != dups.second; dups is a pair of iterators. I didn't find a way to iterate over multimap entries with the specific key in for_each loop. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:23: using DuplicatesMap = On 2016/03/08 13:09:28, vabr (Chromium) wrote: > Please pull DuplicatesMap out of the autofill namespace, there is no reason to > put it in there. Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:24: std::multimap<std::string, scoped_ptr<autofill::PasswordForm>>; On 2016/03/08 13:09:28, vabr (Chromium) wrote: > #include <ma> for multimap Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:81: // there is several forms with the same key, the duplicates are moved from On 2016/03/08 13:09:28, vabr (Chromium) wrote: > nit: To clarify that "duplicates" exclude the first occurrence, consider > rephrasing: > "...the same key, all such forms but the first one are stored in |duplicates| > instead of |list|. > > I would omit "(the multimap from sort key to forms)" and instead note next to > the definition of DuplicatesMap that the std::string is a sort key. Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:83: // |username_and_password_in_key| defines whether username and hashed On 2016/03/08 13:09:28, vabr (Chromium) wrote: > The last sentence is not completely clear -- from the above comment it looks > like the username and the hashed comment are always included. Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:88: autofill::DuplicatesMap* duplicates_, On 2016/03/08 13:09:28, vabr (Chromium) wrote: > nit: Remove the trailing underscore in "duplicates_", this is not a member > variable. Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:130: int number_of_entries, On 2016/03/08 13:09:28, vabr (Chromium) wrote: > int -> size_t Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:134: for (int i = 0; i < number_of_entries; i++) { On 2016/03/08 13:09:28, vabr (Chromium) wrote: > int -> size_t Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:135: const SortEntry entry = test_entries[i]; On 2016/03/08 13:09:28, vabr (Chromium) wrote: > Make this a reference to avoid copying. > > Also on line 159. Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:163: << "position in sorted list: " << entry.expected_position; On 2016/03/08 13:09:28, vabr (Chromium) wrote: > To avoid code duplication, use > SCOPED_TRACE(testing::Message("position in sorted list: ") << > entry.expected_position); > > That will append this information automatically to any failed EXPECT_* message. Done.
Description was changed from ========== [Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords ========== to ========== [Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords BUG=593027 ==========
Thanks, Maxim. I still have issues with the hashing :), everything else looks good. Cheers, Vaclav https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:55: std::string SplitByDotAndReverse(const std::string& host) { On 2016/03/08 15:14:24, kolos1 wrote: > Yes, it would be nice to replace std::string with StringPiece, but there is no > JoinString for vector<StringPiece> (which is weird). So, we will have to convert > every StringPiece to std::string and then join them, i.e. there is no sense to > pass StringPiece, as I understand. This is not about changing the SplitString/JoinString call, that stays as it is (note that SplitString takes StringPiece arguments anyway). What happens now is: at the callsite, a new string is created (by calling string::substr), then SplitString reads it and creates the vector. If you use StringPiece, instead of creating the substring as a new string, SplitByDotAndReverse will get a "viewport" to the full string "covering" the unnecessary parts. SplitString will then happily use that viewport to create a vector if strings (not StringPieces) again. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:78: std::string CreateSortKey(const scoped_ptr<autofill::PasswordForm>& form, On 2016/03/08 15:14:23, kolos1 wrote: > Since we don't modify |form|, I redeclared it as const ref. > > Did the same in RemoveDuplicates. Acknowledged. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:102: base::SHA1HashString(base::UTF16ToUTF8(form->password_value)); On 2016/03/08 15:14:23, kolos1 wrote: > Lets somebody want to see one of passwords listed on password page, but he > doesn't know admin password. Lets he is able to add dummy password entries with > specified passwords. Then, based on sorting results of dummy entries and the > real entry, it is possible to know the real password. > > In few words, sorting shows passwords implicitly. Oh, that's a interesting good point, especially the use of sorting to guide the search for passwords. However, could we get the security team's opinion on this (jww@, mkwst@, or anybody else)? My concern is that there are far easier ways to discover the password (e.g., via DevTools) for somebody with so much time that they can add lots of dummy passwords to the password store, so we might be investing this effort in a vain. If the security team recommends hashing, I'd still like to see a quick comment explaining why is it there (basically what you wrote above). https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:113: void RemoveDuplicates(const scoped_ptr<autofill::PasswordForm>& form, On 2016/03/08 15:14:23, kolos1 wrote: > Since we don't modify form, redeclared as const ref. Acknowledged. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:123: for (autofill::DuplicatesMap::iterator it = dups.first; it != dups.second; On 2016/03/08 15:14:23, kolos1 wrote: > dups is a pair of iterators. > > I didn't find a way to iterate over multimap entries with the specific key in > for_each loop. Ah right, I got confused by the STL naming. :) The current code is fine, sorry for the noise.
https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:55: std::string SplitByDotAndReverse(const std::string& host) { On 2016/03/08 16:04:44, vabr (Chromium) wrote: > On 2016/03/08 15:14:24, kolos1 wrote: > > Yes, it would be nice to replace std::string with StringPiece, but there is no > > JoinString for vector<StringPiece> (which is weird). So, we will have to > convert > > every StringPiece to std::string and then join them, i.e. there is no sense to > > pass StringPiece, as I understand. > > This is not about changing the SplitString/JoinString call, that stays as it is > (note that SplitString takes StringPiece arguments anyway). What happens now is: > at the callsite, a new string is created (by calling string::substr), then > SplitString reads it and creates the vector. > If you use StringPiece, instead of creating the substring as a new string, > SplitByDotAndReverse will get a "viewport" to the full string "covering" the > unnecessary parts. SplitString will then happily use that viewport to create a > vector if strings (not StringPieces) again. Done. https://codereview.chromium.org/1589483002/diff/260001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:102: base::SHA1HashString(base::UTF16ToUTF8(form->password_value)); I discussed this question with security team. They said we could use unhashed passwords here. It doesn't make any difference. Fixed to unhashed password.
Thanks for the careful discussion about hashes! LGTM with some comments below. Cheers, Vaclav https://codereview.chromium.org/1589483002/diff/300001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1589483002/diff/300001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:12: #include "base/sha1.h" This is no longer needed. https://codereview.chromium.org/1589483002/diff/300001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1589483002/diff/300001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:83: // username and hashed password are also included in sort key. If there is Please update the comment to not mention hashing any more.
Ah, yes, I forgot to update comments and includes. Thanks. https://codereview.chromium.org/1589483002/diff/300001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1589483002/diff/300001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:12: #include "base/sha1.h" On 2016/03/09 10:00:11, vabr (Chromium) wrote: > This is no longer needed. Done. https://codereview.chromium.org/1589483002/diff/300001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1589483002/diff/300001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:83: // username and hashed password are also included in sort key. If there is On 2016/03/09 10:00:11, vabr (Chromium) wrote: > Please update the comment to not mention hashing any more. Done.
Thank you, LGTM with the last nit. :) https://codereview.chromium.org/1589483002/diff/320001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1589483002/diff/320001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:83: // username and password are also included in sort key. If there is several grammar: is several -> are several
Thanks :) Done. Regards, Maxim https://codereview.chromium.org/1589483002/diff/320001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1589483002/diff/320001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.h:83: // username and password are also included in sort key. If there is several On 2016/03/09 13:45:42, vabr (Chromium) wrote: > grammar: is several -> are several Done.
lgtm
The CQ bit was checked by kolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1589483002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1589483002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1589483002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1589483002/340001
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords BUG=593027 ========== to ========== [Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords BUG=593027 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords BUG=593027 ========== to ========== [Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords BUG=593027 Committed: https://crrev.com/62ad5a8fc17197b1383df48afa0dda9654202d3f Cr-Commit-Position: refs/heads/master@{#384524} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/62ad5a8fc17197b1383df48afa0dda9654202d3f Cr-Commit-Position: refs/heads/master@{#384524} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
