Bug 61891

Created attachment 95696 [details] Patch

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?

Maybe IntRect::moveBy(const IntPoint&)?

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 :)

Sounds fine.

Comment on attachment 95696 [details] Patch Cool. Clearing the flag pending that change.

Created attachment 95801 [details] Patch

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

Comment on attachment 95801 [details] Patch Looks reasonable to me. What's up with the cr bot?

Created attachment 95802 [details] Patch

Comment on attachment 95802 [details] Patch OK.

(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.

Created attachment 95809 [details] Patch for landing

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

Comment on attachment 95809 [details] Patch for landing Clearly we need to catch these timeout exceptions and not fail patches.

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

Comment on attachment 95809 [details] Patch for landing Clearing flags

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