Bug 137704 - Use is<>() / downcast<>() for RenderInline
Summary: Use is<>() / downcast<>() for RenderInline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 137424
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-14 11:21 PDT by Chris Dumez
Modified: 2014-10-14 17:30 PDT (History)
5 users (show)

See Also:


Attachments
Patch (71.08 KB, patch)
2014-10-14 11:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (70.98 KB, patch)
2014-10-14 16:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-14 11:21:49 PDT
Use is<>() / downcast<>() for RenderInline.
Comment 1 Chris Dumez 2014-10-14 11:45:15 PDT
Created attachment 239812 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-14 11:48:07 PDT
Attachment 239812 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderInline.cpp:1111:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2014-10-14 15:37:44 PDT
Comment on attachment 239812 [details]
Patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:222
> -    toRenderInline(renderer()).lineBoxes().removeLineBox(this);
> +    downcast<RenderInline>(renderer()).lineBoxes().removeLineBox(this);

Seems like renderer() should return a RenderInline& if it’s guaranteed to be one. Maybe there’s some reason not to do this.

> Source/WebCore/rendering/InlineFlowBox.cpp:1011
> +    for (InlineBox* current = lastChild(); current; current = current->prevOnLine()) {

I think I would have called this “child” instead of “current”. I noticed one like this in a previous patch, and probably should have commented then.

> Source/WebCore/rendering/RenderBox.cpp:2013
> +    ASSERT(renderer == container() || is<RenderRegion>(*renderer));

Looks like this function should take a RenderElement& given that assertion. Maybe some day we can make that change.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:217
> +    while (is<RenderInline>(parent)) {

This would read better as a for loop. If we can’t fit it all on one line we could leave out the initializer, but the termination and “move to parent” would be fit nicely in a for.
Comment 4 Chris Dumez 2014-10-14 16:24:56 PDT
Created attachment 239832 [details]
Patch
Comment 5 WebKit Commit Bot 2014-10-14 16:27:46 PDT
Attachment 239832 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderInline.cpp:1111:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 WebKit Commit Bot 2014-10-14 17:29:57 PDT
Comment on attachment 239832 [details]
Patch

Clearing flags on attachment: 239832

Committed r174714: <http://trac.webkit.org/changeset/174714>
Comment 7 WebKit Commit Bot 2014-10-14 17:30:02 PDT
All reviewed patches have been landed.  Closing bug.