Bug 37977

Summary: [Chromium] Font size in suggestions popup menu should be correlated with the font size of its text field.
Product: WebKit Reporter: Roman <romange>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, evan, jcivelli, ojan, playmobil, romange, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
patch
evan: review-
patch
abarth: review-
patch
none
patch
abarth: review+
Patch dglazkov: review+, dglazkov: commit-queue+

Description Roman 2010-04-22 00:13:11 PDT
[Chromium] Font size in suggestions popup menu should be correlated with the font size of its text field.
Comment 1 Jeremy Moskovich 2010-04-22 04:14:59 PDT
In Chromium bug tracker @ http://crbug.com/7376
Comment 2 Evan Martin 2010-04-22 08:13:23 PDT
I don't see a patch.  Did something go wrong with webkit-patch upload?
Comment 3 Roman 2010-04-22 08:44:23 PDT
Created attachment 54061 [details]
patch
Comment 4 Roman 2010-04-22 08:46:30 PDT
I just uploaded the patch.
Comment 5 Evan Martin 2010-04-22 08:46:45 PDT
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?)
Comment 6 Evan Martin 2010-04-22 08:46:58 PDT
Sorry, I meant: testing discussion belongs in the ChangeLog.
Comment 7 Roman 2010-04-22 09:01:54 PDT
Created attachment 54064 [details]
patch

Added a comment about the testing situation.
Comment 8 Dimitri Glazkov (Google) 2010-04-22 09:10:53 PDT
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?
Comment 9 Roman 2010-04-22 09:30:51 PDT
I uploaded the correct patch. It should be ok now.
Comment 10 Dimitri Glazkov (Google) 2010-04-22 09:37:22 PDT
Comment on attachment 54064 [details]
patch

ok. I was looking at a wrong diff. Also, let's let Jay look at it.
Comment 11 Dimitri Glazkov (Google) 2010-04-22 09:38:26 PDT
Jay, can you take a look?
Comment 12 Evan Martin 2010-04-22 10:09:44 PDT
This looks good to me.  (I am not a reviewer.)
Comment 13 Evan Martin 2010-04-22 10:33:08 PDT
BTW, http://ponderer.org/tests/inputsize.html is a useful test.  I wonder if it can be checked in somewhere.
Comment 14 Ojan Vafai 2010-04-22 10:50:50 PDT
(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 15 Adam Barth 2010-04-22 12:22:12 PDT
Comment on attachment 54064 [details]
patch

Bad indent in ChangeLog.  Maybe you have a tab?

Please add the manual test requested above.
Comment 16 Roman 2010-04-25 05:16:28 PDT
Created attachment 54235 [details]
patch
Comment 17 Roman 2010-04-25 07:21:04 PDT
Created attachment 54241 [details]
patch
Comment 18 Roman 2010-04-25 07:22:35 PDT
(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 19 Jeremy Moskovich 2010-04-25 08:53:23 PDT
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?
Comment 20 Evan Martin 2010-04-25 09:02:23 PDT
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 21 Adam Barth 2010-04-25 09:08:34 PDT
Comment on attachment 54241 [details]
patch

okiedokes
Comment 22 Roman 2010-04-25 09:26:06 PDT
(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.
Comment 23 Jeremy Moskovich 2010-04-26 05:08:35 PDT
Created attachment 54280 [details]
Patch

Updated patch - [hopefully] ready for landing. Adam, could you please r+ if this LGTY?
Comment 24 Dimitri Glazkov (Google) 2010-04-26 09:45:27 PDT
Comment on attachment 54280 [details]
Patch

ok.
Comment 25 Evan Martin 2010-04-26 09:56:50 PDT
Committed r58259: <http://trac.webkit.org/changeset/58259>
Comment 26 Evan Martin 2010-04-26 10:03:25 PDT
Committed r58260: <http://trac.webkit.org/changeset/58260>