Bug 61891

Summary: Switch paintReplaced to use IntPoint
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Levi Weintraub 2011-06-01 16:22:43 PDT
Ongoing tx/ty removal.
Comment 1 Levi Weintraub 2011-06-01 18:24:37 PDT
Created attachment 95696 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-01 22:55:15 PDT
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?
Comment 3 Simon Fraser (smfr) 2011-06-02 08:04:11 PDT
Maybe IntRect::moveBy(const IntPoint&)?
Comment 4 Levi Weintraub 2011-06-02 11:21:53 PDT
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 :)
Comment 5 Eric Seidel (no email) 2011-06-02 11:23:28 PDT
Sounds fine.
Comment 6 Levi Weintraub 2011-06-02 11:33:04 PDT
Comment on attachment 95696 [details]
Patch

Cool. Clearing the flag pending that change.
Comment 7 Levi Weintraub 2011-06-02 13:40:41 PDT
Created attachment 95801 [details]
Patch
Comment 8 WebKit Review Bot 2011-06-02 13:57:17 PDT
Comment on attachment 95801 [details]
Patch

Attachment 95801 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8754855
Comment 9 Eric Seidel (no email) 2011-06-02 14:01:28 PDT
Comment on attachment 95801 [details]
Patch

Looks reasonable to me.  What's up with the cr bot?
Comment 10 Levi Weintraub 2011-06-02 14:02:33 PDT
Created attachment 95802 [details]
Patch
Comment 11 Eric Seidel (no email) 2011-06-02 14:09:09 PDT
Comment on attachment 95802 [details]
Patch

OK.
Comment 12 Levi Weintraub 2011-06-02 14:09:47 PDT
(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.
Comment 13 Levi Weintraub 2011-06-02 14:20:50 PDT
Created attachment 95809 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2011-06-02 19:42:42 PDT
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 15 Eric Seidel (no email) 2011-06-03 07:39:22 PDT
Comment on attachment 95809 [details]
Patch for landing

Clearly we need to catch these timeout exceptions and not fail patches.
Comment 16 WebKit Commit Bot 2011-06-03 07:41:21 PDT
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 17 Levi Weintraub 2011-06-03 10:58:04 PDT
Comment on attachment 95809 [details]
Patch for landing

Clearing flags
Comment 18 Levi Weintraub 2011-06-03 10:59:47 PDT
Landed in r87989.

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