Bug 130165 - RenderTextControl does not need OS X-specific behavior
Summary: RenderTextControl does not need OS X-specific behavior
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-12 16:26 PDT by Myles C. Maxfield
Modified: 2022-10-25 08:38 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.25 KB, patch)
2014-03-18 12:00 PDT, Myles C. Maxfield
dino: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (542.87 KB, application/zip)
2014-03-18 13:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (550.04 KB, application/zip)
2014-03-18 13:23 PDT, Build Bot
no flags Details
WIP (12.13 KB, patch)
2014-03-20 15:02 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-03-12 16:26:45 PDT
RenderTextControl inspects a family name to see if it starts with a '.' This is OSX-specific.
Comment 1 Myles C. Maxfield 2014-03-18 12:00:04 PDT
Created attachment 227085 [details]
Patch
Comment 2 Dean Jackson 2014-03-18 12:12:07 PDT
Comment on attachment 227085 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227085&action=review

> Source/WebCore/ChangeLog:3
> +        RenderTextControl switches on OSX behavior

Please change bug title to "RenderTextControl does not need iOS-specific behavior"

> Source/WebCore/ChangeLog:18
> +        * platform/graphics/Font.cpp:
> +        (WebCore::Font::hasValidAverageCharWidth):
> +        (WebCore::Font::fastAverageCharWidthIfAvailable):

You should describe what you did here, because it isn't removing platform-specific code, or at least doesn't look like it is.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:-354
> -    // For text inputs, IE adds some extra width.
> -    if (maxCharWidth > 0.f)
> -        result += maxCharWidth - charWidth;

How do we know we can get rid of this too?
Comment 3 Dean Jackson 2014-03-18 12:12:33 PDT
Comment on attachment 227085 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227085&action=review

>> Source/WebCore/ChangeLog:3
>> +        RenderTextControl switches on OSX behavior
> 
> Please change bug title to "RenderTextControl does not need iOS-specific behavior"

I meant OS X specific, of course :)
Comment 4 Myles C. Maxfield 2014-03-18 12:16:58 PDT
That code is guarded behind a check which is never true. Therefore, it wasn't getting hit. By unifying the two code paths, I was changing the behavior of the guard, which was making this code be (erroneously) hit, so I needed to take it out.
Comment 5 Build Bot 2014-03-18 13:19:06 PDT
Created attachment 227095 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 6 Build Bot 2014-03-18 13:23:30 PDT
Created attachment 227096 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 7 Myles C. Maxfield 2014-03-18 20:36:04 PDT
Looks like the OS/2 table has positive values for xAvgCharWidth, rather than negative values which I had anticipated. This patch is bad.
Comment 8 Myles C. Maxfield 2014-03-20 15:02:36 PDT
Reopening to attach new patch.
Comment 9 Myles C. Maxfield 2014-03-20 15:02:37 PDT
Created attachment 227341 [details]
WIP
Comment 10 Ahmad Saleem 2022-10-25 08:38:44 PDT
(In reply to Myles C. Maxfield from comment #9)
> Created attachment 227341 [details]
> WIP

@myles - We do have this code still:

https://github.com/WebKit/WebKit/blob/main/Source/WebCore/platform/graphics/FontCascade.cpp#L354

Is this patch needed anymore? Thanks!