Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(144)

Issue 967213004: Removed FrameView's windowToContents and contentsToWindow methods. (Closed)

Created:
3 years, 11 months ago by bokan
Modified:
3 years, 11 months ago
CC:
aandrey+blink_chromium.org, aboxhall, apavlov+blink_chromium.org, blink-layers+watch_chromium.org, blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-events_chromium.org, blink-reviews-html_chromium.org, blink-reviews-rendering, bshe, caseq+blink_chromium.org, dcheng, devtools-reviews_chromium.org, dglazkov+blink, dmazzoni, Dominik Röttsches, Donn Denman, eae+blinkwatch, eustas+blink_chromium.org, jchaffraix+rendering, je_julie(Not used), kozyatinskiy+blink_chromium.org, leviw+renderwatch, loislo+blink_chromium.org, lushnikov+blink_chromium.org, malch+blink_chromium.org, mlamouri+watch-blink_chromium.org, nektarios, pdr+renderingwatchlist_chromium.org, pfeldman+blink_chromium.org, rwlbuis, sergeyv+blink_chromium.org, sof, yurys+blink_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Removed FrameView's windowToContents and contentsToWindow methods. This CL adds methods to FrameView for converting between the FrameView's content space and Blink's viewport space and then replaces |window| in all uses of contentsToWindow and windowToContents with either rootFrame or Viewport. |Window| is equivalent to |RootFrame| so it has no behavioral change. Changing |Window| to |Viewport| will change behavior under page scale. See http://www.chromium.org/developers/design-documents/blink-coordinate-spaces for details. BUG=371902, 455497, 455328 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192299

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 15

Patch Set 6 : Rebase #

Patch Set 7 : Addressed rbyers@ feedback (minus tests) #

Total comments: 6

Patch Set 8 : Fixed up documentation comments #

Patch Set 9 : Spelling correction #

Total comments: 10

Patch Set 10 : Rebase #

Patch Set 11 : Comments now link to the web page #

Patch Set 12 : Rebase #

Patch Set 13 : Fixed context menu, replaced FIXME in CopyImageAt/SaveImageAt/PerformMediaPlayerAction with fix #

Patch Set 14 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+444 lines, -387 lines) Patch
M Source/core/dom/Element.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -7 lines 0 comments Download
M Source/core/events/MouseRelatedEvent.h View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/events/MouseRelatedEvent.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +4 lines, -4 lines 0 comments Download
M Source/core/frame/FrameView.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +16 lines, -9 lines 0 comments Download
M Source/core/frame/FrameView.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +39 lines, -21 lines 0 comments Download
M Source/core/frame/LocalFrame.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/frame/PinchViewport.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +11 lines, -0 lines 0 comments Download
M Source/core/frame/PinchViewport.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +64 lines, -0 lines 0 comments Download
M Source/core/frame/SmartClip.h View 1 2 chunks +5 lines, -6 lines 0 comments Download
M Source/core/frame/SmartClip.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +18 lines, -28 lines 0 comments Download
M Source/core/html/forms/ColorInputType.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M Source/core/inspector/InspectorDOMAgent.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/layout/LayoutBox.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M Source/core/layout/LayoutBox.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +7 lines, -7 lines 0 comments Download
M Source/core/layout/LayoutMenuList.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/page/DragController.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +4 lines, -4 lines 0 comments Download
M Source/core/page/EventHandler.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 38 chunks +51 lines, -54 lines 0 comments Download
M Source/core/page/TouchAdjustment.cpp View 1 2 3 4 5 6 7 8 9 6 chunks +12 lines, -21 lines 0 comments Download
M Source/core/page/TouchDisambiguation.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -5 lines 0 comments Download
M Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M Source/core/testing/Internals.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +4 lines, -4 lines 0 comments Download
M Source/web/ChromeClientImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -2 lines 0 comments Download
M Source/web/ContextMenuClientImpl.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -8 lines 0 comments Download
M Source/web/ExternalPopupMenu.cpp View 1 chunk +1 line, -4 lines 0 comments Download
M Source/web/LinkHighlight.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -2 lines 0 comments Download
M Source/web/PopupContainer.cpp View 1 chunk +5 lines, -5 lines 0 comments Download
M Source/web/PopupListBox.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M Source/web/PopupListBox.cpp View 1 2 3 4 5 6 3 chunks +4 lines, -4 lines 0 comments Download
M Source/web/TextFinder.cpp View 1 2 3 4 5 6 7 8 9 4 chunks +4 lines, -4 lines 0 comments Download
M Source/web/ValidationMessageClientImpl.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/web/WebAXObject.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -9 lines 0 comments Download
M Source/web/WebDevToolsAgentImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +4 lines, -4 lines 0 comments Download
M Source/web/WebFrameWidgetImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +4 lines, -3 lines 0 comments Download
M Source/web/WebInputEventConversion.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +10 lines, -8 lines 0 comments Download
M Source/web/WebLocalFrameImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M Source/web/WebLocalFrameImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +18 lines, -22 lines 0 comments Download
M Source/web/WebPluginContainerImpl.h View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/web/WebPluginContainerImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +13 lines, -13 lines 0 comments Download
M Source/web/WebViewImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -3 lines 0 comments Download
M Source/web/WebViewImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 25 chunks +60 lines, -86 lines 0 comments Download
M Source/web/tests/PinchViewportTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +37 lines, -0 lines 0 comments Download
M Source/web/tests/TouchActionTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M Source/web/tests/WebPluginContainerTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +8 lines, -8 lines 0 comments Download
M Source/web/tests/WebViewTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -8 lines 0 comments Download
M public/web/WebFrame.h View 1 2 3 2 chunks +3 lines, -4 lines 0 comments Download
M public/web/WebPluginContainer.h View 1 chunk +4 lines, -4 lines 0 comments Download

Messages

Total messages: 23 (5 generated)
bokan
Rick, here's part 2, ptal.
3 years, 11 months ago (2015-03-04 23:00:54 UTC) #2
Rick Byers
This seems reasonable, thanks for driving all the improvements here! It seems our test coverage ...
3 years, 11 months ago (2015-03-05 17:56:21 UTC) #3
bokan
https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameView.cpp#newcode3666 Source/core/frame/FrameView.cpp:3666: IntRect frameRect = convertFromContainingWindow(rectInRootFrame); On 2015/03/05 17:56:20, Rick Byers ...
3 years, 11 months ago (2015-03-06 21:54:37 UTC) #4
bokan
On 2015/03/05 17:56:21, Rick Byers wrote: > This seems reasonable, thanks for driving all the ...
3 years, 11 months ago (2015-03-06 21:55:39 UTC) #5
bokan
https://codereview.chromium.org/967213004/diff/80001/Source/web/tests/PinchViewportTest.cpp File Source/web/tests/PinchViewportTest.cpp (right): https://codereview.chromium.org/967213004/diff/80001/Source/web/tests/PinchViewportTest.cpp#newcode1468 Source/web/tests/PinchViewportTest.cpp:1468: // Tests that the maximum scroll offset of the ...
3 years, 11 months ago (2015-03-06 22:07:20 UTC) #7
Rick Byers
Changes look good, thanks. https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameView.h File Source/core/frame/FrameView.h (left): https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameView.h#oldcode482 Source/core/frame/FrameView.h:482: // of containing window is. ...
3 years, 11 months ago (2015-03-07 15:44:09 UTC) #8
bokan
https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/FrameView.h File Source/core/frame/FrameView.h (right): https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/FrameView.h#newcode475 Source/core/frame/FrameView.h:475: // Coordinate space transform methods. On 2015/03/07 15:44:08, Rick ...
3 years, 11 months ago (2015-03-09 13:44:33 UTC) #9
Rick Byers
https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/FrameView.h File Source/core/frame/FrameView.h (right): https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/FrameView.h#newcode475 Source/core/frame/FrameView.h:475: // Coordinate space transform methods. Are you OK with ...
3 years, 11 months ago (2015-03-12 17:06:49 UTC) #10
bokan
I'm working on adding tests for what I can but, if you're ok with it, ...
3 years, 11 months ago (2015-03-14 01:12:57 UTC) #11
Rick Byers
On 2015/03/14 01:12:57, bokan wrote: > I'm working on adding tests for what I can ...
3 years, 11 months ago (2015-03-17 14:51:19 UTC) #12
bokan
On 2015/03/17 14:51:19, Rick Byers wrote: > On 2015/03/14 01:12:57, bokan wrote: > > I'm ...
3 years, 11 months ago (2015-03-17 14:54:15 UTC) #13
bokan
+Alexandre for Source/web I also went ahead and fixed a few of the spots I ...
3 years, 11 months ago (2015-03-17 16:12:39 UTC) #15
bokan
ping
3 years, 11 months ago (2015-03-20 17:20:22 UTC) #16
aelias_OOO_until_Jul13
lgtm. I'd like to see later followup on FIXMEs in WebFrameWidgetImpl::selectionBounds and WebViewImpl::applyPluginAction though.
3 years, 11 months ago (2015-03-20 18:54:54 UTC) #17
bokan
On 2015/03/20 18:54:54, aelias wrote: > lgtm. I'd like to see later followup on FIXMEs ...
3 years, 11 months ago (2015-03-20 18:57:00 UTC) #18
Rick Byers
Still LGTM with latest changes - land-away
3 years, 11 months ago (2015-03-20 19:01:00 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967213004/280001
3 years, 11 months ago (2015-03-20 20:08:08 UTC) #22
commit-bot: I haz the power
3 years, 11 months ago (2015-03-21 00:31:58 UTC) #23
Message was sent while issue was closed.
Committed patchset #14 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192299

Powered by Google App Engine
This is Rietveld 408576698