Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
264053007:
[rAC] Display Autofill popups vertically compact. (Closed)
|
Created:
6 years, 7 months ago by groby-ooo-7-16 Modified:
6 years, 5 months ago CC:
chromium-reviews, benquan, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, please use gerrit instead Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[rAC] Display Autofill popups vertically compact.
If an autofill popup is supposed to display a multi-line address,
the address needs to be converted into a single-line format with
separators.
BUG=362373
R=estade@chromium.org
TBR=isherman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282710
Patch Set 1 #
Total comments: 1
Patch Set 2 : Moved code to PersonalDataManager #
Total comments: 3
Patch Set 3 : Remove DEPS/gypi #
Total comments: 2
Patch Set 4 : Review fixes. #
Total comments: 4
Patch Set 5 : Sigh. Missed parentheses. #
Total comments: 4
Patch Set 6 : Use function from new spot in addressinput_util #Patch Set 7 : And now using the right function from the right place... #
Total comments: 2
Patch Set 8 : Fix review nit. #
Total comments: 2
Patch Set 9 : Fix review issue. #
Messages
Total messages: 28 (0 generated)
https://codereview.chromium.org/264053007/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/264053007/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2155: } This needs to be done in GetProfileSuggestions so it applies to normal web autofill as well. This will be much easier after https://codereview.chromium.org/261013010/
How is that CL making it easier? CreateAddressData will save the Split() call, but the joining still needs to be done somewhere? Either way, do I wait for the CL, or do you just want to take the bug?
The change should be in GetProfileSuggestions, which is in components/, components/ doesn't currently depend on libaddressinput, after that CL components/ will depend on (parts of) libaddressinput. I suggest you wait for the other CL and then simply move your new block of code.
sgtm On Wed, May 7, 2014 at 3:44 PM, <estade@chromium.org> wrote: > The change should be in GetProfileSuggestions, which is in components/, > components/ doesn't currently depend on libaddressinput, after that CL > components/ will depend on (parts of) libaddressinput. I suggest you wait > for > the other CL and then simply move your new block of code. > > https://codereview.chromium.org/264053007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, moved over to PersonalDataManager. Waiting for your CL to land so I can kill DEPS and gypi entries.
https://codereview.chromium.org/264053007/diff/20001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager.cc:646: (*values)[i] = (JoinString(lines, compact_separator)); you'll have to move this to ~L581 and do the operation on multi_values so you can use the language code from the profile.
FYI, the blocking CL landed
PTAL
https://codereview.chromium.org/264053007/diff/20001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager.cc:646: (*values)[i] = (JoinString(lines, compact_separator)); On 2014/05/14 02:36:36, Evan Stade wrote: > you'll have to move this to ~L581 and do the operation on multi_values so you > can use the language code from the profile. ^ https://codereview.chromium.org/264053007/diff/40001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/40001/components/autofill/core... components/autofill/core/browser/personal_data_manager.cc:646: (*values)[i] = (JoinString(lines, compact_separator)); nit: why the extra parens aroudn JoinString?
estade:PTAL TBR'd isherman@ for gypi change https://codereview.chromium.org/264053007/diff/20001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager.cc:646: (*values)[i] = (JoinString(lines, compact_separator)); On 2014/05/14 02:36:36, Evan Stade wrote: > you'll have to move this to ~L581 and do the operation on multi_values so you > can use the language code from the profile. Done. https://codereview.chromium.org/264053007/diff/40001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/40001/components/autofill/core... components/autofill/core/browser/personal_data_manager.cc:646: (*values)[i] = (JoinString(lines, compact_separator)); On 2014/05/22 18:14:23, Evan Stade wrote: > nit: why the extra parens aroudn JoinString? No idea. Removed. https://codereview.chromium.org/264053007/diff/50001/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/264053007/diff/50001/components/autofill.gypi... components/autofill.gypi:100: '../third_party/libaddressinput/libaddressinput.gyp:libaddressinput', Note: GetCompactAddressLinesSeparator is part of libaddress_input instead of util. We might want to change that in libaddressinput, but I suggest a different CL.
https://codereview.chromium.org/264053007/diff/50001/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/264053007/diff/50001/components/autofill.gypi... components/autofill.gypi:100: '../third_party/libaddressinput/libaddressinput.gyp:libaddressinput', On 2014/05/22 22:54:46, groby wrote: > Note: GetCompactAddressLinesSeparator is part of libaddress_input instead of > util. We might want to change that in libaddressinput, but I suggest a different > CL. The point of libaddressinput_util is to be able to limit what we include here. So I think that should be fixed before this CL rather than after. https://codereview.chromium.org/264053007/diff/70001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/70001/components/autofill/core... components/autofill/core/browser/personal_data_manager.cc:587: ::i18n::addressinput::GetCompactAddressLinesSeparator(app_locale_)); not |app_locale_|. profile->language_code().
https://codereview.chromium.org/264053007/diff/50001/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/264053007/diff/50001/components/autofill.gypi... components/autofill.gypi:100: '../third_party/libaddressinput/libaddressinput.gyp:libaddressinput', On 2014/05/22 23:41:28, Evan Stade wrote: > On 2014/05/22 22:54:46, groby wrote: > > Note: GetCompactAddressLinesSeparator is part of libaddress_input instead of > > util. We might want to change that in libaddressinput, but I suggest a > different > > CL. > > The point of libaddressinput_util is to be able to limit what we include here. > So I think that should be fixed before this CL rather than after. see very similar change here: https://codereview.chromium.org/261013010/diff/250001/third_party/libaddressi...
LGTM https://codereview.chromium.org/264053007/diff/70001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/70001/components/autofill/core... components/autofill/core/browser/personal_data_manager.cc:584: // multi-line names into a single line, using a separator. nit: "names" -> "addresses"?
ping?
On 2014/06/26 16:05:14, Evan Stade wrote: > ping? ping Rachel. Will you have time for this this week?
I was checking issue 367937 (https://code.google.com/p/chromium/issues/detail?id=367937) and from what I checked, it looks like the change in GetProfileSuggestions() to replace separator so as to use single line address will fix the issue (I could be way off since I haven't tried the fix yet). I'll stop checking that issue since this patch should take care of it. If, just in case, this is not being actively worked on then please let me know. Thanks!
Yes, finally getting to this. Wrapping tonight.
PTAL https://codereview.chromium.org/264053007/diff/50001/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/264053007/diff/50001/components/autofill.gypi... components/autofill.gypi:100: '../third_party/libaddressinput/libaddressinput.gyp:libaddressinput', On 2014/05/22 23:44:19, Evan Stade wrote: > On 2014/05/22 23:41:28, Evan Stade wrote: > > On 2014/05/22 22:54:46, groby wrote: > > > Note: GetCompactAddressLinesSeparator is part of libaddress_input instead of > > > util. We might want to change that in libaddressinput, but I suggest a > > different > > > CL. > > > > The point of libaddressinput_util is to be able to limit what we include here. > > So I think that should be fixed before this CL rather than after. > > see very similar change here: > https://codereview.chromium.org/261013010/diff/250001/third_party/libaddressi... Done. https://codereview.chromium.org/264053007/diff/70001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/70001/components/autofill/core... components/autofill/core/browser/personal_data_manager.cc:584: // multi-line names into a single line, using a separator. On 2014/05/23 10:05:05, Ilya Sherman wrote: > nit: "names" -> "addresses"? Done. https://codereview.chromium.org/264053007/diff/70001/components/autofill/core... components/autofill/core/browser/personal_data_manager.cc:587: ::i18n::addressinput::GetCompactAddressLinesSeparator(app_locale_)); On 2014/05/22 23:41:28, Evan Stade wrote: > not |app_locale_|. profile->language_code(). Done.
Mostly lg, but I suspect this will conflict with https://codereview.chromium.org/298863012/ (+cc rouslan) https://codereview.chromium.org/264053007/diff/110001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/110001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:582: base::SplitString(multi_values[i], '\n', &lines); use ReplaceChars?
Yes, working on this. On Mon, Jul 7, 2014 at 8:28 AM, <n.bansal@samsung.com> wrote: > I was checking issue 367937 > (https://code.google.com/p/chromium/issues/detail?id=367937) and from > what I > checked, it looks like the change in GetProfileSuggestions() to replace > separator so as to use single line address will fix the issue (I could be > way > off since I haven't tried the fix yet). > > I'll stop checking that issue since this patch should take care of it. If, > just > in case, this is not being actively worked on then please let me know. > > Thanks! > > https://codereview.chromium.org/264053007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Well, I suppose I'll wait for the big change from rouslan@ to land, and then rebase once more. I'd still appreciate LGs if possible - if the mega-patch fails, I'd love to submit :) https://codereview.chromium.org/264053007/diff/110001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/110001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:582: base::SplitString(multi_values[i], '\n', &lines); On 2014/07/10 18:01:35, Evan Stade wrote: > use ReplaceChars? Done.
The mega-patch landed. Let's hope the build does not break and there will be no reverts. On Thu, Jul 10, 2014 at 11:56 AM, <groby@chromium.org> wrote: > Well, I suppose I'll wait for the big change from rouslan@ to land, and > then > rebase once more. > > I'd still appreciate LGs if possible - if the mega-patch fails, I'd love to > submit :) > > > > https://codereview.chromium.org/264053007/diff/110001/ > components/autofill/core/browser/personal_data_manager.cc > File components/autofill/core/browser/personal_data_manager.cc (right): > > https://codereview.chromium.org/264053007/diff/110001/ > components/autofill/core/browser/personal_data_manager.cc#newcode582 > components/autofill/core/browser/personal_data_manager.cc:582: > base::SplitString(multi_values[i], '\n', &lines); > On 2014/07/10 18:01:35, Evan Stade wrote: > >> use ReplaceChars? >> > > Done. > > https://codereview.chromium.org/264053007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/264053007/diff/130001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/130001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:581: base::UTF8ToUTF16("\n"), nit: ASCIIToUTF16
Since it still compiles & runs with Rouslan's changes, it doesn't _seem_ there's an issue - off to the CQ we go. https://codereview.chromium.org/264053007/diff/130001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/264053007/diff/130001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:581: base::UTF8ToUTF16("\n"), On 2014/07/10 20:25:03, Evan Stade wrote: > nit: ASCIIToUTF16 Done.
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/264053007/150001
Message was sent while issue was closed.
Change committed as 282710 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
