Avoid overlapping label text in autocomplete popup with icon
Created attachment 72134 [details] Patch
paintRow() probably also needs to updated to take the icon width into account for the |restrictWidthOfListBox| case. It's not entirely obvious to me how that should work -- should the label take precedence to the icon, or vice versa? If there are multiple rows and only some of the rows have overflow, what alignment do we want across the rows? This could probably also use a layout test, but I couldn't find any existing layout tests for these functions. Could someone please point me in the right direction?
I should have mentioned -- this corresponds to Chromium bug 60096: http://code.google.com/p/chromium/issues/detail?id=60096
View in context: https://bugs.webkit.org/attachment.cgi?id=72134&action=review > WebCore/platform/chromium/PopupMenuChromium.cpp:1272 > + if (width > 0) 4 space indent > WebCore/platform/chromium/PopupMenuChromium.cpp:1277 > + We should probably take into account the icon when computing the row height.
Created attachment 72274 [details] Patch, addressing Jay's comments
Comment on attachment 72274 [details] Patch, addressing Jay's comments New patch up, addressing Jay's review comments
Attachment 72274 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4830078
I am not a reviewer but R+ for me.
Comment on attachment 72274 [details] Patch, addressing Jay's comments View in context: https://bugs.webkit.org/attachment.cgi?id=72274&action=review > WebCore/ChangeLog:741 > +2010-10-27 Ilya Sherman <isherman@chromium.org> New ChangeLog entry should be at the beginning of the file. > WebCore/platform/chromium/PopupMenuChromium.cpp:1278 > + width += kLabelToIconPadding; We don't use "k" prefix in WebKit style. I understand this symbol is not introduced by this patch. So you don't need to address this issue for now.
Created attachment 72531 [details] Patch I think I uploaded an old version of the patch somehow last time; hoping things work better this time
Created attachment 72532 [details] Patch Third time's the charm?
Comment on attachment 72532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72532&action=review > WebCore/ChangeLog:9 > + No new tests. (OOPS!) Remove this line please.
(In reply to comment #12) > (From update of attachment 72532 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72532&action=review > > > WebCore/ChangeLog:9 > > + No new tests. (OOPS!) > > Remove this line please. Does that mean this patch doesn't need any tests? I'd figured that we want tests, but wasn't sure where to add them and what to use as a starting point...
Comment on attachment 72532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72532&action=review >>> WebCore/ChangeLog:9 >>> + No new tests. (OOPS!) >> >> Remove this line please. > > Does that mean this patch doesn't need any tests? I'd figured that we want tests, but wasn't sure where to add them and what to use as a starting point... Of course we should have tests as possible. I meant we should remove (OOPS!) except "Reviewed by" line. Commit-queue rejects a patch with (OOPS!). We already have WebKit/chromium/tests/PopupMenuTest.cpp, but I think it's difficult to add a test for this change in WebKit. So I think it's ok to have no tests.
Created attachment 72675 [details] Patch
(In reply to comment #14) > (From update of attachment 72532 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72532&action=review > > >>> WebCore/ChangeLog:9 > >>> + No new tests. (OOPS!) > >> > >> Remove this line please. > > > > Does that mean this patch doesn't need any tests? I'd figured that we want tests, but wasn't sure where to add them and what to use as a starting point... > > Of course we should have tests as possible. I meant we should remove (OOPS!) except "Reviewed by" line. Commit-queue rejects a patch with (OOPS!). > > We already have WebKit/chromium/tests/PopupMenuTest.cpp, but I think it's difficult to add a test for this change in WebKit. So I think it's ok to have no tests. Ok, removed.
Comment on attachment 72675 [details] Patch Clearing flags on attachment: 72675 Committed r71191: <http://trac.webkit.org/changeset/71191>
All reviewed patches have been landed. Closing bug.