Summary: | Switch paintOutline, paintContinuationOutlines, and paintOutlineForLine to use IntPoint | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Levi Weintraub <leviw> | ||||
Component: | Layout and Rendering | Assignee: | Levi Weintraub <leviw> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin, eae, eric, simon.fraser, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 60318 | ||||||
Attachments: |
|
Description
Levi Weintraub
2011-06-06 11:55:34 PDT
Created attachment 96107 [details]
Patch
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 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. (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 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. I'll file another bug for the cleanup of this function and do it separately. Thanks for looking! Committed r88190: <http://trac.webkit.org/changeset/88190> Comment on attachment 96107 [details]
Patch
Clearing flags
|