Bug 62137 - Switch paintOutline, paintContinuationOutlines, and paintOutlineForLine to use IntPoint
Summary: Switch paintOutline, paintContinuationOutlines, and paintOutlineForLine to us...
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: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks: 60318
  Show dependency treegraph
 
Reported: 2011-06-06 11:55 PDT by Levi Weintraub
Modified: 2011-06-06 15:52 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.06 KB, patch)
2011-06-06 12:15 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2011-06-06 11:55:34 PDT
Ongoing tx/ty removal.
Comment 1 Levi Weintraub 2011-06-06 12:15:20 PDT
Created attachment 96107 [details]
Patch
Comment 2 WebKit Review Bot 2011-06-06 12:16:56 PDT
Attachment 96107 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderInline.cpp:1435:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Eric Seidel (no email) 2011-06-06 13:57:43 PDT
Comment on attachment 96107 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:2681
> +        for ( ; block && block != this; block = block->containingBlock())
> +            accumulatedPaintOffset.move(block->location());

Really?  We don't have a function to accumulate these for us?  We should as smfr if this is the modern way to do this.  I suspect there may be a bug here.

> Source/WebCore/rendering/RenderInline.cpp:1503
> +                   (!nextline.isEmpty() && l - ow < paintOffset.x() + nextline.maxX()) ? -ow : ow,

Obviously there is a bunch of cleanup we could do to this function in a second pass.
Comment 4 Levi Weintraub 2011-06-06 14:11:54 PDT
(In reply to comment #3)
> (From update of attachment 96107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96107&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:2681
> > +        for ( ; block && block != this; block = block->containingBlock())
> > +            accumulatedPaintOffset.move(block->location());
> 
> Really?  We don't have a function to accumulate these for us?  We should as smfr if this is the modern way to do this.  I suspect there may be a bug here.

This surprised me too... Given all the discussion of how we arrive at tx/ty, this seems way too simplistic. I'd love a comment from smfr before landing as a sanity check.

> 
> > Source/WebCore/rendering/RenderInline.cpp:1503
> > +                   (!nextline.isEmpty() && l - ow < paintOffset.x() + nextline.maxX()) ? -ow : ow,
> 
> Obviously there is a bunch of cleanup we could do to this function in a second pass.
Comment 5 Simon Fraser (smfr) 2011-06-06 14:16:11 PDT
Comment on attachment 96107 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:2530
> +                inlineRenderer->paintOutline(paintInfo.context,  paintOffset - locationOffset() + inlineRenderer->containingBlock()->location());

You have two spaces after the first comma

>>> Source/WebCore/rendering/RenderBlock.cpp:2681
>>> +            accumulatedPaintOffset.move(block->location());
>> 
>> Really?  We don't have a function to accumulate these for us?  We should as smfr if this is the modern way to do this.  I suspect there may be a bug here.
> 
> This surprised me too... Given all the discussion of how we arrive at tx/ty, this seems way too simplistic. I'd love a comment from smfr before landing as a sanity check.

Yeah, I would expect a mapLocalToContainer() call here or something similar. OK to just convert now, since you're not changing behavior.

>> Source/WebCore/rendering/RenderInline.cpp:1435
>> +    int l = paintOffset.x() + thisline.x() - offset;
> 
> l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]

Yeah, feel free to rename the variables.
Comment 6 Levi Weintraub 2011-06-06 14:35:01 PDT
I'll file another bug for the cleanup of this function and do it separately. Thanks for looking!
Comment 7 Levi Weintraub 2011-06-06 14:36:03 PDT
Committed r88190: <http://trac.webkit.org/changeset/88190>
Comment 8 Levi Weintraub 2011-06-06 15:52:42 PDT
Comment on attachment 96107 [details]
Patch

Clearing flags