Bug 79283

Created attachment 128310 [details] Patch

Comment on attachment 128310 [details] Patch Attachment 128310 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11561657

Created attachment 128320 [details] Patch

Pinging reviewers.

Comment on attachment 128320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128320&action=review > Source/WebCore/dom/MouseRelatedEvent.cpp:217 > + return roundToInt(m_offsetLocation.x()); I think we want this to be a member function once that's practical. x().round() reads better to me than roundToInt(x()). > Source/WebCore/rendering/RenderBoxModelObject.h:64 > + int pixelSnappedOffsetLeft() const { return roundToInt(offsetLeft()); } > + int pixelSnappedOffsetTop() const { return roundToInt(offsetTop()); } Are these really pixel snapped? For left/top these seem OK, but writing right/left accessors the same would clearly be wrong, no? Shouldn't we always work from a rect when possible isntead of individual accessors?

Comment on attachment 128320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128320&action=review >> Source/WebCore/dom/MouseRelatedEvent.cpp:217 >> + return roundToInt(m_offsetLocation.x()); > > I think we want this to be a member function once that's practical. x().round() reads better to me than roundToInt(x()). I agree, and am happy to volunteer to do that when this conversion is done. I'd be curious to hear Darin's opinion though, as he's felt strongly about these sorts of things in the past. >> Source/WebCore/rendering/RenderBoxModelObject.h:64 >> + int pixelSnappedOffsetTop() const { return roundToInt(offsetTop()); } > > Are these really pixel snapped? For left/top these seem OK, but writing right/left accessors the same would clearly be wrong, no? Shouldn't we always work from a rect when possible isntead of individual accessors? You're picking up correctly that this is okay explicitly because it's top/left. It's definitely worth spending some time fixing up callers to avoid breaking rects apart.

(In reply to comment #6) > (From update of attachment 128320 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128320&action=review > >> Source/WebCore/rendering/RenderBoxModelObject.h:64 > >> + int pixelSnappedOffsetTop() const { return roundToInt(offsetTop()); } > > > > Are these really pixel snapped? For left/top these seem OK, but writing right/left accessors the same would clearly be wrong, no? Shouldn't we always work from a rect when possible isntead of individual accessors? > > You're picking up correctly that this is okay explicitly because it's top/left. It's definitely worth spending some time fixing up callers to avoid breaking rects apart. We have this general problem all over the rendering tree... which is why it was so great that you and eae started by deploying rect/size wider. Unfortunately that work is still not really "done" and we'll need to do more of it after we flip over to fixed point. I suspect we may not even be able to get the snapping perfect until we plumb whole rects more thoroughly through the system?

Then again I may just be riding this pet issue too long, and we already have rect/size sufficiently deployed. You two would know much better than I these days.

(In reply to comment #8) > Then again I may just be riding this pet issue too long, and we already have rect/size sufficiently deployed. You two would know much better than I these days. I agree there's likely more work to do in preserving rects throughout rendering, but for what it's worth, offering pixelSnapped versions can potentially lessen misuse. We actually only use these methods in our branch in PrintContext, from outside rendering code. That sort of use is probably okay.

(In reply to comment #9) > (In reply to comment #8) > > Then again I may just be riding this pet issue too long, and we already have rect/size sufficiently deployed. You two would know much better than I these days. > > I agree there's likely more work to do in preserving rects throughout rendering, but for what it's worth, offering pixelSnapped versions can potentially lessen misuse. We actually only use these methods in our branch in PrintContext, from outside rendering code. That sort of use is probably okay. Is this going to prevent a positive review?

Nope, just one of many patches forgotten in the queue. I'll take another look this afternoon.

Comment on attachment 128320 [details] Patch OK.

Comment on attachment 128320 [details] Patch Thanks Eric!

Comment on attachment 128320 [details] Patch Rejecting attachment 128320 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11779522

The commit-queue encountered the following flaky tests while processing attachment 128320 [details]: perf/array-reverse.html bug 78593 (author: ojan@chromium.org) The commit-queue is continuing to process your patch.

(In reply to comment #14) > (From update of attachment 128320 [details]) > Rejecting attachment 128320 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 > > ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). > > Full output: http://queues.webkit.org/results/11779522 Whoa... it has the line to be filled in. Tools failure? I guess I'll land manually...

Comment on attachment 128320 [details] Patch Clearing flags on attachment: 128320 Committed r109378: <http://trac.webkit.org/changeset/109378>

All reviewed patches have been landed. Closing bug.