Summary: | Switch paintReplaced to use IntPoint | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Levi Weintraub <leviw> | ||||||||||
Component: | Layout and Rendering | Assignee: | Levi Weintraub <leviw> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, darin, dglazkov, eae, eric, simon.fraser, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 61562 | ||||||||||||
Bug Blocks: | 60318, 61949 | ||||||||||||
Attachments: |
|
Description
Levi Weintraub
2011-06-01 16:22:43 PDT
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! |