[Chromium] Font size in suggestions popup menu should be correlated with the font size of its text field.
In Chromium bug tracker @ http://crbug.com/7376
I don't see a patch. Did something go wrong with webkit-patch upload?
Created attachment 54061 [details] patch
I just uploaded the patch.
Comment on attachment 54061 [details] patch > > 2010-04-16 Jarkko Sakkinen <jarkko.j.sakkinen@gmail.com> > - > + It looks like your editor may have mangled the changelog (?) Can you describe the testing situation for this? (E.g. something about how it can't be tested? Maybe we should check in a manual test file?)
Sorry, I meant: testing discussion belongs in the ChangeLog.
Created attachment 54064 [details] patch Added a comment about the testing situation.
Comment on attachment 54064 [details] patch > Index: ChangeLog > =================================================================== > --- ChangeLog (revision 58098) > +++ ChangeLog (working copy) > @@ -1,3 +1,18 @@ > +2010-04-22 Roman Gershman <romange@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + [Chromium] Font size in suggestions popup menu should be correlated with the font size of its text field. > + Currently it's not possible to test this change via LayoutTests, > + since test_shell does not implement suggestions popup. The diff for the ChangeLog is touches way too much. Can you please reduce that? > Unreviewed, rolling out r58069. > Index: src/SuggestionsPopupMenuClient.cpp > =================================================================== > --- src/SuggestionsPopupMenuClient.cpp (revision 58097) > +++ src/SuggestionsPopupMenuClient.cpp (working copy) > @@ -156,11 +156,9 @@ void SuggestionsPopupMenuClient::initial > FontDescription fontDescription; > RenderTheme::defaultTheme()->systemFont(CSSValueWebkitControl, > fontDescription); > + RenderStyle* style = m_textField->computedStyle(); > + fontDescription.setComputedSize(style->fontDescription().computedSize()); ... but we can write a unit test for this, right?
I uploaded the correct patch. It should be ok now.
Comment on attachment 54064 [details] patch ok. I was looking at a wrong diff. Also, let's let Jay look at it.
Jay, can you take a look?
This looks good to me. (I am not a reviewer.)
BTW, http://ponderer.org/tests/inputsize.html is a useful test. I wonder if it can be checked in somewhere.
(In reply to comment #13) > BTW, http://ponderer.org/tests/inputsize.html is a useful test. I wonder if it > can be checked in somewhere. WebCore/manual-tests +1 to adding a manual test for now. One day hopefully we can convert it to an automated one.
Comment on attachment 54064 [details] patch Bad indent in ChangeLog. Maybe you have a tab? Please add the manual test requested above.
Created attachment 54235 [details] patch
Created attachment 54241 [details] patch
(In reply to comment #15) > (From update of attachment 54064 [details]) > Bad indent in ChangeLog. Maybe you have a tab? > > Please add the manual test requested above. Done.
Comment on attachment 54241 [details] patch Ob-Disclaimer: I'm not a WebKit reviewer, a few general remarks: WebCore/manual-tests/chromium/suggestions-popup-font-change.html:5 + <p>Do the following, the test passes if step 2 gets the expected behavior</p> nit: Perhaps something like: Verify that font size in suggestion popup matches the corresponding input field, test passes if step 2 matches expected behavior. WebCore/manual-tests/chromium/suggestions-popup-font-change.html:9 + should be similar to the one in the text form</li> nit - perhaps: Verify that font size of text in suggestion popup is the same as in the corresponding input form. WebCore/manual-tests/chromium/suggestions-popup-font-change.html:10 + <li>3. Now delete the text from the input box and press Ctrl+. Repeat the step 2. nit - perhaps: Delete contents of input box, press Cntrl/+ [Command/+ on Mac] to magnify the page contents. Repeat step 2. , text size should still match. WebKit/chromium/ChangeLog:5 + [Chromium] Font size in suggestions popup menu should be correlated with the font size of its text field. Weird indent WebKit/chromium/ChangeLog:3 + Reviewed by Adam Barth nit: I always keep this as "Nobody" till I land the patch, the commit-queue will automagically place the reviewer name here for you if you land it that way. WebKit/chromium/ChangeLog:6 + Test: WebCore/manual-tests/chromium/suggestions-popup-font-change.html I wonder if it make sense to place this in WebCore/manual-tests since this isn't really chromium specific?
The other WebKit ports just use a native select widget so they don't have this issue. I worry this is getting reviewed to death -- poor Roman hasn't quite got the procedure right but it's literally two-liner change, can anyone R+ it so I can clean up the ChangeLog whitespace for him? :)
Comment on attachment 54241 [details] patch okiedokes
(In reply to comment #20) > The other WebKit ports just use a native select widget so they don't have this > issue. I worry this is getting reviewed to death -- poor Roman hasn't quite > got the procedure right but it's literally two-liner change, can anyone R+ it > so I can clean up the ChangeLog whitespace for him? :) Thanks Evan :) My English is far from being perfect, so I prefer to address Jeremy's comments before submitting this patch.
Created attachment 54280 [details] Patch Updated patch - [hopefully] ready for landing. Adam, could you please r+ if this LGTY?
Comment on attachment 54280 [details] Patch ok.
Committed r58259: <http://trac.webkit.org/changeset/58259>
Committed r58260: <http://trac.webkit.org/changeset/58260>