Bug 61891 – Switch paintReplaced to use IntPoint

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

Ongoing tx/ty removal.

[Levi Weintraub]

Created attachment 95696 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 95696 [details]
Patch

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

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:60
> +    rect.move(paintOffset);

So does this move to the point, or treat the point as a size?  I think IntRect::move(IntPoint) is confusing and should probably be removed.  Since I believe this does not move to the point, but rahter by the size of the point, I think rect.move(toSize(paintOffset)) might end up clearer. This again is the tx/ty being used for different things in different places coming to bite us.

> Source/WebCore/rendering/RenderImage.cpp:254
> +            IntSize imageOffset;

Why is this way out here if it's only used in that one place?

[Simon Fraser (smfr)]

Maybe IntRect::moveBy(const IntPoint&)?

[Levi Weintraub]

Comment on attachment 95696 [details]
Patch

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

>> Source/WebCore/rendering/RenderHTMLCanvas.cpp:60
>> +    rect.move(paintOffset);
> 
> So does this move to the point, or treat the point as a size?  I think IntRect::move(IntPoint) is confusing and should probably be removed.  Since I believe this does not move to the point, but rahter by the size of the point, I think rect.move(toSize(paintOffset)) might end up clearer. This again is the tx/ty being used for different things in different places coming to bite us.

I like "moveBy" and that doesn't introduce an unnecessary copy. How does that sound to you, Eric?

>> Source/WebCore/rendering/RenderImage.cpp:254
>> +            IntSize imageOffset;
> 
> Why is this way out here if it's only used in that one place?

It's used in two places is why -- see L289 :)

[Eric Seidel (no email)]

Sounds fine.

[Levi Weintraub]

Comment on attachment 95696 [details]
Patch

Cool. Clearing the flag pending that change.

[Levi Weintraub]

Created attachment 95801 [details]
Patch

[WebKit Review Bot]

Comment on attachment 95801 [details]
Patch

Attachment 95801 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8754855

[Eric Seidel (no email)]

Comment on attachment 95801 [details]
Patch

Looks reasonable to me.  What's up with the cr bot?

[Levi Weintraub]

Created attachment 95802 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 95802 [details]
Patch

OK.

[Levi Weintraub]

(In reply to comment #11)
> (From update of attachment 95802 [details])
> OK.

Thanks! I'd missed a file not used in the Mac build. I'll wait for EWS to verify then land.

[Levi Weintraub]

Created attachment 95809 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 95809 [details]
Patch for landing

Rejecting attachment 95809 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8759402

[Eric Seidel (no email)]

Comment on attachment 95809 [details]
Patch for landing

Clearly we need to catch these timeout exceptions and not fail patches.

[WebKit Commit Bot]

Comment on attachment 95809 [details]
Patch for landing

Rejecting attachment 95809 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
g rejects to file Source/WebCore/rendering/RenderVideo.cpp.rej
patching file Source/WebCore/rendering/RenderVideo.h
Hunk #1 FAILED at 66.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/rendering/RenderVideo.h.rej
patching file Source/WebCore/rendering/RenderView.cpp
Hunk #1 FAILED at 273.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/rendering/RenderView.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8764121

[Levi Weintraub]

Comment on attachment 95809 [details]
Patch for landing

Clearing flags

[Levi Weintraub]

Landed in r87989.

Eric, Adam: The timed-out commit queue actually landed this patch but didn't make note of it!