Bug 37977 – [Chromium] Font size in suggestions popup menu should be correlated with the font size of its text field.

• Component: New Bugs
• Status: RESOLVED FIXED

[Roman]

[Chromium] Font size in suggestions popup menu should be correlated with the font size of its text field.

[Jeremy Moskovich]

In Chromium bug tracker @ http://crbug.com/7376

[Evan Martin]

I don't see a patch.  Did something go wrong with webkit-patch upload?

[Roman]

Created attachment 54061 [details]
patch

[Roman]

I just uploaded the patch.

[Evan Martin]

Comment on attachment 54061 [details]
patch

>  
>  2010-04-16  Jarkko Sakkinen  <[e-mail]>
> - 
> +

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?)

[Evan Martin]

Sorry, I meant: testing discussion belongs in the ChangeLog.

[Roman]

Created attachment 54064 [details]
patch

Added a comment about the testing situation.

[Dimitri Glazkov (Google)]

Comment on attachment 54064 [details]
patch

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 58098)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Roman Gershman  <[e-mail]>
> +
> +        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?

[Roman]

I uploaded the correct patch. It should be ok now.

[Dimitri Glazkov (Google)]

Comment on attachment 54064 [details]
patch

ok. I was looking at a wrong diff. Also, let's let Jay look at it.

[Dimitri Glazkov (Google)]

Jay, can you take a look?

[Evan Martin]

This looks good to me.  (I am not a reviewer.)

[Evan Martin]

BTW, http://ponderer.org/tests/inputsize.html is a useful test.  I wonder if it can be checked in somewhere.

[Ojan Vafai]

(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.

[Adam Barth]

Comment on attachment 54064 [details]
patch

Bad indent in ChangeLog.  Maybe you have a tab?

Please add the manual test requested above.

[Roman]

Created attachment 54235 [details]
patch

[Roman]

Created attachment 54241 [details]
patch

[Roman]

(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.

[Jeremy Moskovich]

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?

[Evan Martin]

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?  :)

[Adam Barth]

Comment on attachment 54241 [details]
patch

okiedokes

[Roman]

(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.

[Jeremy Moskovich]

Created attachment 54280 [details]
Patch

Updated patch - [hopefully] ready for landing. Adam, could you please r+ if this LGTY?

[Dimitri Glazkov (Google)]

Comment on attachment 54280 [details]
Patch

ok.

[Evan Martin]

Committed r58259: <http://trac.webkit.org/changeset/58259>

[Evan Martin]

Committed r58260: <http://trac.webkit.org/changeset/58260>