Bug 131552

Summary:

Wrap CSS length conversion arguments in an object

Product:

WebKit

Reporter:

Bem Jones-Bey <bjonesbe>

Component:

CSS

Assignee:

Bem Jones-Bey <bjonesbe>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

allan.jensen, bunhere, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kling, kondapallykalyan, macpherson, menard, philipj, rakuco, sergio, zalan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

https://codereview.chromium.org/64293008/

Bug Depends on:

Bug Blocks:

87846

Attachments:

Description	Flags
Patch	none

Bem Jones-Bey

Reported 2014-04-11 11:42:16 PDT

This is a port of a patch from Blink by timloh@chromium.org

Attachments
Patch (131.45 KB, patch) 2014-04-11 11:53 PDT, Bem Jones-Bey	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Bem Jones-Bey

Comment 1 2014-04-11 11:53:15 PDT

Created attachment 229146 [details] Patch

WebKit Commit Bot

Comment 2 2014-04-11 11:55:46 PDT

Attachment 229146 [details] did not pass style-queue: ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:217: color_indexMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:260: aspect_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:274: device_aspect_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:318: device_pixel_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:365: device_heightMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:378: device_widthMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:427: min_colorMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:432: max_colorMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:437: min_color_indexMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:442: max_color_indexMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:447: min_monochromeMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:452: max_monochromeMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:457: min_aspect_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:462: max_aspect_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:467: min_device_aspect_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:472: max_device_aspect_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:477: min_device_pixel_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:482: max_device_pixel_ratioMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:487: min_heightMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:492: max_heightMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:497: min_widthMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:502: max_widthMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:507: min_device_heightMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:512: max_device_heightMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:517: min_device_widthMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:522: max_device_widthMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:527: min_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:532: max_resolutionMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:555: transform_2dMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:564: transform_3dMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:590: view_modeMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:635: video_playable_inlineMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 32 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

Andreas Kling

Comment 3 2014-04-28 19:45:04 PDT

Comment on attachment 229146 [details] Patch This look reasonable.

WebKit Commit Bot

Comment 4 2014-04-29 09:37:12 PDT

Comment on attachment 229146 [details] Patch Clearing flags on attachment: 229146 Committed r167937: <http://trac.webkit.org/changeset/167937>

WebKit Commit Bot

Comment 5 2014-04-29 09:37:19 PDT

All reviewed patches have been landed. Closing bug.

zalan

Comment 6 2014-04-29 09:38:00 PDT

>particular adding a RenderView for resolving viewport units at style My only concern was whether doing any unnecessary renderView->viewportSize() calls in order to eliminate the RenderView* parameter had any performance impact, but apparently this patch doesnt do that change at all (so not sure whether the changelog entry reflects what this patch does)

Bem Jones-Bey

Comment 7 2014-04-29 09:51:19 PDT

(In reply to comment #6) > >particular adding a RenderView for resolving viewport units at style > My only concern was whether doing any unnecessary renderView->viewportSize() calls in order to eliminate the RenderView* parameter had any performance impact, but apparently this patch doesnt do that change at all (so not sure whether the changelog entry reflects what this patch does) The RenderView change will happen in a follow up patch, the changelog here just mentions it as rationale for why we'd want to have a class for these arguments. I will be posting the followup change to Bug 87846. While the current patch up there passes around IntSize, given the performance concerns that you voiced, I will be changing that to be a RenderView and to only make the viewportSize() call when resolving the actual viewport units.

zalan

Comment 8 2014-04-29 09:52:35 PDT

(In reply to comment #7) > (In reply to comment #6) > > >particular adding a RenderView for resolving viewport units at style > > My only concern was whether doing any unnecessary renderView->viewportSize() calls in order to eliminate the RenderView* parameter had any performance impact, but apparently this patch doesnt do that change at all (so not sure whether the changelog entry reflects what this patch does) > > The RenderView change will happen in a follow up patch, the changelog here just mentions it as rationale for why we'd want to have a class for these arguments. > > I will be posting the followup change to Bug 87846. While the current patch up there passes around IntSize, given the performance concerns that you voiced, I will be changing that to be a RenderView and to only make the viewportSize() call when resolving the actual viewport units. ok, cool. Thanks!

Note You need to log in before you can comment on or make changes to this bug.