Bug 131552

Summary: Wrap CSS length conversion arguments in an object
Product: WebKit Reporter: Bem Jones-Bey <bjonesbe>
Component: CSSAssignee: 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: P2    
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

Description Bem Jones-Bey 2014-04-11 11:42:16 PDT 
This is a port of a patch from Blink by timloh@chromium.org
Comment 1 Bem Jones-Bey 2014-04-11 11:53:15 PDT 
Created attachment 229146 [details]
Patch
Comment 2 WebKit Commit Bot 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.
Comment 3 Andreas Kling 2014-04-28 19:45:04 PDT 
Comment on attachment 229146 [details]
Patch

This look reasonable.
Comment 4 WebKit Commit Bot 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>
Comment 5 WebKit Commit Bot 2014-04-29 09:37:19 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 6 zalan 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)
Comment 7 Bem Jones-Bey 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.
Comment 8 zalan 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!