Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23477015:
[sync] Significantly speed up password model association on mac (Closed)
|
Created:
7 years, 3 months ago by Raghu Simha Modified:
7 years, 2 months ago CC:
chromium-reviews, rlarocque, tim (not reviewing) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[sync] Significantly speed up password model association on mac
During password model association on mac, the helper function
internal_keychain_helpers::GetPasswordsForForms individually searches
through the OS keychain for passwords matching each web form for which a
password was saved. With tracing turned on, it turned out that these
O(N) keychain searches, one for each password stored in the keychain,
accounted for more than 80% of the password model association time.
In this patch, we make GetPasswordsForForms first pre-load the attributes
of all passwords stored in the keychain without loading the actual password
data (this doesn't require authorization and won't pop up a UI prompt), and
then individually load password data only for the keychain entries that match
the web forms we're interested in.
This is still considerably faster than doing O(N) keychain searches.
For a test account with 50 synced passwords, this change yielded almost
an 80% reduction in the time spent in GetPasswordsForForms().
BUG=263685
TEST=Enable tracing for password model association and measure its
performance with and without this change.
R=stuartmorgan@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227954
Patch Set 1 #
Total comments: 2
Patch Set 2 : First pre-load only attributes of all items, then match, then selectively load passwords. #Patch Set 3 : Further reduce keychain reads #
Total comments: 12
Patch Set 4 : Rebase (no code changes) #Patch Set 5 : Address feedback #
Total comments: 14
Patch Set 6 : Address feedback; Add more unit tests. #Patch Set 7 : Rebase (no code changes) #
Total comments: 10
Patch Set 8 : Fix nits. #Patch Set 9 : Rebase #
Messages
Total messages: 24 (0 generated)
Stuart, please review. Thanks.
https://codereview.chromium.org/23477015/diff/1/chrome/browser/password_manag... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/23477015/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_store_mac.cc:449: // We load all passwords added to the keychain by Chrome at the start, instead This comment is incorrect. You aren't loading all the passwords added to the keychain "by Chrome", you are loading *all* form-type passwords ever stored in the keychain by any application. This will potentially trigger N copies of a dialog asking the user to authorize access to keychain entries that Chrome has never used, which is unacceptable. This would have to be done in conjunction with something like https://codereview.chromium.org/19775014/ for it to be viable.
+Ilya. Thanks for the review, Stuart. As I mention in my response to your comment, restricting the pre-fetch of passwords to only those owned by the particular flavor of Chrome that's running (Google Chrome or Chromium) doesn't work. Perhaps we can re-visit this fix after Ilya checks in https://codereview.chromium.org/19775014, and decide at that point if it is viable to call GetAllPasswordFormPasswords? As the code is written today, doing O(N) searches via FindMatchingItems() seems to be slowing down password model association a great deal. Since this happens every time chrome is restarted, it seems worthwhile to pursue a fix assuming it can be done correctly without popping up a whole lot of prompts. I wonder if there's a way to do a single keychain search for a set of passwords mergeable with a set of forms... i.e., the plural version PasswordsMergeableWithForms. From looking at the code in AppleKeychain and KeychainSearch, It didn't seem like this was possible. https://codereview.chromium.org/23477015/diff/1/chrome/browser/password_manag... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/23477015/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_store_mac.cc:449: // We load all passwords added to the keychain by Chrome at the start, instead On 2013/08/29 20:24:23, stuartmorgan wrote: > This comment is incorrect. You aren't loading all the passwords added to the > keychain "by Chrome", you are loading *all* form-type passwords ever stored in > the keychain by any application. This will potentially trigger N copies of a > dialog asking the user to authorize access to keychain entries that Chrome has > never used, which is unacceptable. You are correct. I only stepped through one level of the code and noticed that GetAllPasswordFormPasswords() was passing in CreatorCodeForSearch() while initializing keychain_search, and this led me to believe we were searching only for passwords owned by Chrome. I took a closer look and tried modifying my code to first call keychain_adapter.SetFindsOnlyOwnedItems(true) before calling GetAllPasswordFormPasswords(), but this only returned a subset of the passwords added by chrome. This is presumably because I've used Google Chrome to add passwords in some cases, and Chromium in other cases. Either way, setting finds_only_owned_ to true doesn't appear to work. > > This would have to be done in conjunction with something like > https://codereview.chromium.org/19775014/ for it to be viable.
I thought about this more last night, and if you want to do significant restructuring, you should be be able to get the effect you want. You'd need to rewrite the helpers to be able to defer password loading. Reading metadata about keychain entries doesn't trigger prompts, just reading the passwords. So you could bulk-load, convert to passwordless forms (holding keychain references with each one), do all the merging, and then fill the passwords for each entry that was actually used in a match.
Stuart, PTAL. On 2013/08/30 09:50:11, stuartmorgan wrote: > I thought about this more last night, and if you want to do significant > restructuring, you should be be able to get the effect you want. You'd need to > rewrite the helpers to be able to defer password loading. > > Reading metadata about keychain entries doesn't trigger prompts, just reading > the passwords. So you could bulk-load, convert to passwordless forms (holding > keychain references with each one), do all the merging, and then fill the > passwords for each entry that was actually used in a match. I've taken a stab at implementing what you suggest above. Let me know what you think of the new approach. To test this, I reset all the access permissions for items in the keychain, and was able to make sure that prompts were not raised for any passwords not being synced by specific chrome profile for which password model association was being done.
Post-holiday-weekend ping.
I grabbed the times taken by GetPasswordsForForms using my personal account with > 50 passwords after adding trace events and looking at chrome://tracing: - With no optimization (trunk): 1750 ms - With the optimization in patch set 2: 860 ms - With the optimization in patch set 3: 406 ms
And here are the times taken by GetPasswordsForForms on startup for a test account with 500 synced passwords: - No optimization: 7582 ms - Optimization in patch set 3: 1101 ms
Sorry for the delay on reviewing this; I need to have a large block of time to swap all this code back in, since it's been years since I was immersed in it, but there's a bunch of complexity so I need to be able to do a thorough review. Realistically, I'm expecting to be able to do it at the beginning of next week.
Pinging once again, since we're hoping to be able to get this into the dev channel soon. Thanks.
https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:466: // selective reads of only the passwords we're interested in. The we/us feels like noise here; would read better with s/We first/First/, s/allows us to avoid/avoids/, s/since we are//, s/passwords we're interested in/relevant passwords/ https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:486: } Please extract this into a helper method; this adds a lot of low-level detail to what's meant to be a high-level method. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:500: GURL((*i)->signon_realm) == GURL(j->second->signon_realm)) { Isn't this just FormsMatchForMerge? Except that it's not clear to me at all that you are handling blacklist times correctly in this loop, because you aren't checking for that after you do extract the password later on. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:509: keychain_matches.push_back(form_with_password); What happened to all the logic in MatchingKeychainItems? Where are you handling auth type? Port? etc. I don't see how this loop is equivalent to the code it replaces if you have any non-trivial cases. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:511: } Again, please extract a helper method. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:551: MacKeychainPasswordFormAdapter::PasswordsMergeableWithForm( Looks like this is dead code now.
Stuart, thanks for the review. PTAL. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:466: // selective reads of only the passwords we're interested in. On 2013/09/18 19:23:32, stuartmorgan wrote: > The we/us feels like noise here; would read better with s/We first/First/, > s/allows us to avoid/avoids/, s/since we are//, s/passwords we're interested > in/relevant passwords/ Done. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:486: } On 2013/09/18 19:23:32, stuartmorgan wrote: > Please extract this into a helper method; this adds a lot of low-level detail to > what's meant to be a high-level method. Done. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:500: GURL((*i)->signon_realm) == GURL(j->second->signon_realm)) { On 2013/09/18 19:23:32, stuartmorgan wrote: > Isn't this just FormsMatchForMerge? Except that it's not clear to me at all that > you are handling blacklist times correctly in this loop, because you aren't > checking for that after you do extract the password later on. I was getting tripped up by the fact that when FillPasswordFormFromKeychainItem was being used to extract only password attributes without the password value, it was (incorrectly) returning a PasswordForm with blacklisted_by_user == true. I've fixed the logic in FillPasswordFormFromKeychainItem and replaced this block with a call to FormsMatchForMerge. This should properly handle blacklisted items by leaving them out. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:509: keychain_matches.push_back(form_with_password); On 2013/09/18 19:23:32, stuartmorgan wrote: > What happened to all the logic in MatchingKeychainItems? Where are you handling > auth type? Port? etc. My earlier reasoning was as follows: - Since server, port, and protocol were components of signon_realm, I was matching them up by parsing the signon_realms into GURLs and comparing them. - Since auth_type and auth_domain are inferred from scheme, I was matching them up by comparing the scheme values. However, you're right that I was neglecting to validate the signon_realm components via ExtractSignonRealmComponents. I've addressed this in FormIsValidAndMatchesOtherForm in the latest patch set. Let me know what you think. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:511: } On 2013/09/18 19:23:32, stuartmorgan wrote: > Again, please extract a helper method. Done. https://codereview.chromium.org/23477015/diff/32001/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:551: MacKeychainPasswordFormAdapter::PasswordsMergeableWithForm( On 2013/09/18 19:23:32, stuartmorgan wrote: > Looks like this is dead code now. Removed and replaced with ExtractPasswordsMergeableWithForm. Done.
Ping.
Post-perf-deadline ping.
https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:365: if (form_a.blacklisted_by_user || form_b.blacklisted_by_user) { As far as I can tell, you are pulling a bunch of password forms that always say they aren't blacklist entries (even if they are), then feeding them into this method. So it seems like this will never fire, and you'll merge keychain blacklist entries from other browsers into the list of non-blacklist forms, breaking the contract of the higher-level methods. Am I missing a filtering step somewhere? On a related note, I'm concerned that several versions of this CL have had important behavioral regressions that apparently weren't covered by tests. Obviously that means I didn't write tests for enough of the cases, but if you are going to be changing core functioning of the password system I'd like to see increased test coverage of the things I've pointed out that earlier iterations would have broken so that we can be confident that each case we've talked about will survive the refactoring. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:569: MacKeychainPasswordFormAdapter::ExtractPasswordsMergeableWithForm( AFAICT this no longer operates on the keychain in any way, so it's weird to have it be part of MacKeychainPasswordFormAdapter. Can it just be a stand-alone helper? https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:577: // returned forms. Feel free to use ScopedVector anywhere it makes life easier here BTW; this code predates it, which is the only reason it does everything manually. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... File chrome/browser/password_manager/password_store_mac_internal.h (right): https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac_internal.h:150: // password attributes will be copied. See comment below; there's a critical caveat you've added to the behavior here that's not covered in the comment. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac_internal.h:195: // not require OS authorization. You should mention that the resulting PasswordForms essentially lie about whether or not they are blacklist entries, always claiming not to be. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac_internal.h:197: GetAllKeychainItemAttributesAsPasswordForms( I don't understand the use of the word "attributes" in this method name. Keychain items correspond to password forms; keychain item attributes correspond to password form fields. What is "attributes" intended to convey here? https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac_unittest.cc:283: *keychain_, keychain_item, &form, true); You added a new param, but set it to true in every unit test call, which means there's no clear unit test coverage of the new path you added.
Stuart, thanks again for the review. PTAL. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:365: if (form_a.blacklisted_by_user || form_b.blacklisted_by_user) { On 2013/09/30 22:39:12, stuartmorgan wrote: > As far as I can tell, you are pulling a bunch of password forms that always say > they aren't blacklist entries (even if they are), then feeding them into this > method. So it seems like this will never fire, and you'll merge keychain > blacklist entries from other browsers into the list of non-blacklist forms, > breaking the contract of the higher-level methods. Am I missing a filtering step > somewhere? There is a filtering step for blacklisted passwords in MergePasswordForms, which is called after ExtractPasswordsMergeableWithForm. In addition, the above check does fire when any one of form_a or form_b are blacklisted. I stepped through a debugger and was able to confirm that we are hitting this case due to the fact that the corresponding PasswordForms in database_forms do have the blacklisted_by_user flag set. I have gone ahead and added a step that checks for the blacklisted bit *after* the passwords are extracted from the keychain, and only then adds the form to |matches|. However, you're right that the documentation for GetAllKeychainItemAttributesAsPasswordForms should be updated to mention blacklist entries. > On a related note, I'm concerned that several versions of this CL have had > important behavioral regressions that apparently weren't covered by tests. > Obviously that means I didn't write tests for enough of the cases, but if you > are going to be changing core functioning of the password system I'd like to see > increased test coverage of the things I've pointed out that earlier iterations > would have broken so that we can be confident that each case we've talked about > will survive the refactoring. I added a new unit test for blacklisted passwords where the PasswordFormData entries contain non-empty passwords, but the keychain entries for those forms are blacklisted, and made sure that the collection of merged forms returned is empty. I've also added a new unit test to test FillPasswordFormFromKeychainItem for both versions of |extract_password_data|. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:569: MacKeychainPasswordFormAdapter::ExtractPasswordsMergeableWithForm( On 2013/09/30 22:39:12, stuartmorgan wrote: > AFAICT this no longer operates on the keychain in any way, so it's weird to have > it be part of MacKeychainPasswordFormAdapter. Can it just be a stand-alone > helper? We reference the keychain in line 580, while calling FillPasswordFormFromKeychainItem, but this can just as easily be done by passing a const reference to the keychain. The reason I originally made ExtractPasswordsMergeableWithForm a member of MacKeychainPasswordFormAdapter was because it calls FormIsValidAndMatchesOtherForm, which in turn calls ExtractSignonRealmComponents, both of which are members of MacKeychainPasswordFormAdapter. Since these methods do not actually operate on the keychain, I've made them helper functions. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac.cc:577: // returned forms. On 2013/09/30 22:39:12, stuartmorgan wrote: > Feel free to use ScopedVector anywhere it makes life easier here BTW; this code > predates it, which is the only reason it does everything manually. I did consider using a ScopedVector here, but ended up not doing so, since the vector of PasswordForm*s created here is processed in another function, and therefore, cannot be deleted here. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... File chrome/browser/password_manager/password_store_mac_internal.h (right): https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac_internal.h:150: // password attributes will be copied. On 2013/09/30 22:39:12, stuartmorgan wrote: > See comment below; there's a critical caveat you've added to the behavior here > that's not covered in the comment. Comment updated. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac_internal.h:195: // not require OS authorization. On 2013/09/30 22:39:12, stuartmorgan wrote: > You should mention that the resulting PasswordForms essentially lie about > whether or not they are blacklist entries, always claiming not to be. I've rewritten this comment and mentioned blacklist entries. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac_internal.h:197: GetAllKeychainItemAttributesAsPasswordForms( On 2013/09/30 22:39:12, stuartmorgan wrote: > I don't understand the use of the word "attributes" in this method name. > Keychain items correspond to password forms; keychain item attributes correspond > to password form fields. What is "attributes" intended to convey here? Good point :) I've renamed this ExtractAllKeychainItemAttributesIntoPasswordForms. It's a little wordy, but I couldn't think of a name that more accurately describes what's happening here. Let me know what you think. https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... File chrome/browser/password_manager/password_store_mac_unittest.cc (right): https://codereview.chromium.org/23477015/diff/62002/chrome/browser/password_m... chrome/browser/password_manager/password_store_mac_unittest.cc:283: *keychain_, keychain_item, &form, true); On 2013/09/30 22:39:12, stuartmorgan wrote: > You added a new param, but set it to true in every unit test call, which means > there's no clear unit test coverage of the new path you added. I've added a new test for this called TestFillPasswordFormFromKeychainItem.
Ping.
LGTM with nits. https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:462: MacKeychainPasswordFormAdapter keychain_adapter(&keychain); It feels weird that this uses the form adapter instead of being part of it, but since it's called from a function that's already not part of the class I don't see a great solution. I can't remember why GetPasswordsForForms is stand-alone, honestly. But restructuring that seems out of scope here, so this is fine. https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:579: delete form_with_password; Since adding is conditional, it seems like it would be more error-proof for future changes to make form_with_password a scoped_ptr, and then just reverse the if, use .release() in the push_back, and drop the else. We should avoid manual delete calls when possible. https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... File chrome/browser/password_manager/password_store_mac_internal.h (right): https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac_internal.h:39: // |query_form|. This is different form actually extracting the passwords s/different form/different from/ https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac_internal.h:40: // and checking the return count, since it would require reading the passwords s/it/doing that/ https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac_internal.h:130: // passwords ("" or " "). s/remain false for [...]/always be false/
Nits fixed. Will commit this via CQ after rebasing, running unit tests, and manually testing with a couple of my accounts and measuring actual performance gains. https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:462: MacKeychainPasswordFormAdapter keychain_adapter(&keychain); On 2013/10/09 23:49:25, stuartmorgan wrote: > It feels weird that this uses the form adapter instead of being part of it, but > since it's called from a function that's already not part of the class I don't > see a great solution. I can't remember why GetPasswordsForForms is stand-alone, > honestly. But restructuring that seems out of scope here, so this is fine. SGTM. https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:579: delete form_with_password; On 2013/10/09 23:49:25, stuartmorgan wrote: > Since adding is conditional, it seems like it would be more error-proof for > future changes to make form_with_password a scoped_ptr, and then just reverse > the if, use .release() in the push_back, and drop the else. We should avoid > manual delete calls when possible. Done. https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... File chrome/browser/password_manager/password_store_mac_internal.h (right): https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac_internal.h:39: // |query_form|. This is different form actually extracting the passwords On 2013/10/09 23:49:25, stuartmorgan wrote: > s/different form/different from/ Good catch. Done. https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac_internal.h:40: // and checking the return count, since it would require reading the passwords On 2013/10/09 23:49:25, stuartmorgan wrote: > s/it/doing that/ Done. https://codereview.chromium.org/23477015/diff/116001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac_internal.h:130: // passwords ("" or " "). On 2013/10/09 23:49:25, stuartmorgan wrote: > s/remain false for [...]/always be false/ Done.
Thanks for the detailed review, Stuart. I've manually tested the latest CL to make sure that blacklisted passwords are correctly omitted during model association, and that the time spent in loading 500 passwords from the keychain during password model association went down from ~7 seconds to ~1 second. Committing via CQ.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2013/10/10 02:14:15, Raghu Simha wrote: > the time spent in loading 500 passwords from the keychain > during password model association went down from ~7 seconds to ~1 second. Awesome! I have no idea why commit-bot doesn't think I'm a reviewer...
On 2013/10/10 03:00:55, stuartmorgan wrote: > I have no idea why commit-bot doesn't think I'm a reviewer... It's currently broken for other CLs too. Hooray!
Message was sent while issue was closed.
Committed patchset #9 manually as r227954 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
