Audited and renamed uses of methods and variables named RootView

Audited and renamed uses of methods and variables named RootView

This is the first in a series of coordinate transformation cleanups.
Chiefly, I'd like to include pinch coordinate transforms in the FrameView
contentsToWindow type methods. In this patch I've looked just at "RootView"
There shouldn't be any functional change, this is all disambiguation and
renaming.

I looked at all callers of FrameView's:

    IntPoint rootViewToContents(const IntPoint&) const;
    IntPoint contentsToRootView(const IntPoint&) const;
    IntRect rootViewToContents(const IntRect&) const;
    IntRect contentsToRootView(const IntRect&) const;

Currently these methods for converting to/from RootView and Window in
FrameView do the same transformation, so it seems they've been used
interchangeably. They no longer mean the same thing, RootView is the root
FrameView which doesn't include scale and the pinch viewport offset.
Window (which I'll be renaming to viewport shortly) is the visible
viewport so it should include the pinch transformation. I've changed the
rootView version to window where the caller really means "viewport".
Also renamed "view" in coordinate cases to "frame" as I think it is
less ambiguous. e.g a call to contentsToRootView that actually wants
coordinates in the viewport space (i.e. DIP screen coordinates on Android,
DIP window coordinates on desktops) is now contentsToWindow and will soon
be contentsToViewport. A call to contentsToRootView that actually wants
coordinates in page coordinates (i.e. CSS pixels relative to the root
document) is now contentsToRootFrame.

There were external APIs that got or set coordinates in the RootView.
Again, these sometimes really meant viewport so I've renamed those as
appropriate as well. I've also made a few other coordinate related
cleanups along the way:
openPagePopup wasn't using the element rect param so I removed it.
I've renamed parameters like 'rect' and 'point' as I see them to make clear
what their coordinate space is.

BUG=371902
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190693

[bokan, 2015-02-13 21:16:28]

bokan@chromium.org changed reviewers:
+ zeeshanq@chromium.org

[bokan, 2015-02-13 21:38:38]

ptal at when you have a chance.

[bokan, 2015-02-17 21:18:57]

Hey Zeeshan, I've removed the scrollIntoView refactor which didn't really belong
here. What's left should mostly be renames.

[bokan, 2015-02-17 21:19:11]

bokan@chromium.org changed reviewers:
+ dgozman@chromium.org

[bokan, 2015-02-17 21:21:42]

Hi Dmitry,

Could you please confirm the bits in Source/core/inspector are right? I've
renamed contentsToRootView to contentsToRootFrame, they still do the same thing
so that's not the question. I'm just making sure that's actually what was
desired here. Basically, should these be contentsToRootFrame or
contentsToWindow? Another way to ask this is do these methods today work when
the page is pinch-zoomed in?

[dgozman, 2015-02-18 14:05:58]

See below for InspectorOverlay. The rest of inspector looks good.

https://codereview.chromium.org/919423002/diff/100001/Source/core/inspector/I...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/919423002/diff/100001/Source/core/inspector/I...
Source/core/inspector/InspectorOverlay.cpp:242: // TODO: These look like they
should be RootFrame, dgozman to confirm
Situation here depends on whether WebPageOverlay is affected by viewport. If it
does, this should be toRootFrame (please rename this method to
|contentsQuadToRootFrame| then). If it doesn't, this is toViewport.

[bokan, 2015-02-18 14:10:45]

https://codereview.chromium.org/919423002/diff/100001/Source/core/inspector/I...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/919423002/diff/100001/Source/core/inspector/I...
Source/core/inspector/InspectorOverlay.cpp:242: // TODO: These look like they
should be RootFrame, dgozman to confirm
On 2015/02/18 14:05:58, dgozman wrote:
> Situation here depends on whether WebPageOverlay is affected by viewport. If
it
> does, this should be toRootFrame (please rename this method to
> |contentsQuadToRootFrame| then). If it doesn't, this is toViewport.

The WebPageOverlay is the highlights and size annotation over elements when
mousing over html in devtools?

[bokan, 2015-02-18 15:39:20]

bokan@chromium.org changed reviewers:
+ oysteine@chromium.org

[bokan, 2015-02-18 15:42:51]

Oystein, could you please confirm the change in Document.cpp related to
transition elements? 

transitionElement.rect = element->boundsInRootViewSpace();

What coordinate space is this rect supposed to be in? i.e. should pinch-zoom be
applied or is it expected to be in page coordinates (root FrameView)?

[bokan, 2015-02-18 19:47:42]

bokan@chromium.org changed reviewers:
+ rbyers@chromium.org
- zeeshanq@chromium.org

[bokan, 2015-02-18 20:12:31]

Rick, could you PTAL (or find help find someone who might be more appropriate)?
It's a long change but fairly mechanical.

[oystein (OOO til 10th of July), 2015-02-18 20:25:39]

On 2015/02/18 15:42:51, bokan wrote:
> Oystein, could you please confirm the change in Document.cpp related to
> transition elements? 
> 
> transitionElement.rect = element->boundsInRootViewSpace();
> 
> What coordinate space is this rect supposed to be in? i.e. should pinch-zoom
be
> applied or is it expected to be in page coordinates (root FrameView)?

Pinch-zoom should be applied, yes, IIRC.

[Rick Byers, 2015-02-20 20:29:23]

rbyers@chromium.org changed reviewers:
+ aelias@chromium.org

[Rick Byers, 2015-02-20 20:29:24]

Sorry for the delay.  LGTM.  +aelias for source/web (I know he previously
suggested we should do a major update/clarification like this).

I like your clarified taxonomy (frames = CSS pixels, viewport = DIPs, avoid
"view", etc.).  The only minor concern I've got is that we've put some effort
into educating folks about the two viewports (visual and layout) now we have
lots of methods named "viewport" which don't clarify which viewport they mean. 
That said, I think you'll be further clarifying this  with your co-ordinate
system and documentation you attach to that.  So this is a good step in that
direction.

Does this completely replace the use of "rootView" as a co-ordinate system name
in blink (i.e. did you search for all uses of that term)?

[aelias_OOO_until_Jul13, 2015-02-20 20:48:43]

Source/web lgtm

[bokan, 2015-02-23 15:00:46]

bokan@chromium.org changed reviewers:
+ vollick@chromium.org

[bokan, 2015-02-23 15:01:05]

+vollick@ for Source/platform/HostWindow

[Ian Vollick, 2015-02-23 15:05:36]

On 2015/02/23 15:01:05, bokan wrote:
> +vollick@ for Source/platform/HostWindow

Source/platform lgtm

[bokan, 2015-02-23 16:22:44]

New patchsets have been uploaded after l-g-t-m from
rbyers@chromium.org,aelias@chromium.org,vollick@chromium.org

[bokan, 2015-02-23 16:25:56]

On 2015/02/20 20:29:24, Rick Byers (Out until 3-3) wrote:
> Does this completely replace the use of "rootView" as a co-ordinate system
name
> in blink (i.e. did you search for all uses of that term)?

Yes, there's a few occurrences of "rootView" left but they're in DevTools
front-end code (unrelated to the FrameView hierarchy) and references to the
actual root FrameView which makes sense.

[bokan, 2015-02-23 16:26:07]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2015-02-23 16:26:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/919423002/180001

[commit-bot: I haz the power, 2015-02-23 18:15:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-23 18:15:37]

Failed to apply patch for Source/core/inspector/InspectorOverlay.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/inspector/InspectorOverlay.cpp
  Hunk #1 succeeded at 110 (offset -1 lines).
  Hunk #2 succeeded at 236 (offset -1 lines).
  Hunk #3 succeeded at 294 (offset -1 lines).
  Hunk #4 succeeded at 317 (offset -1 lines).
  Hunk #5 succeeded at 604 (offset -1 lines).
  Hunk #6 FAILED at 830.
  1 out of 6 hunks FAILED -- saving rejects to file
Source/core/inspector/InspectorOverlay.cpp.rej

Patch:       Source/core/inspector/InspectorOverlay.cpp
Index: Source/core/inspector/InspectorOverlay.cpp
diff --git a/Source/core/inspector/InspectorOverlay.cpp
b/Source/core/inspector/InspectorOverlay.cpp
index
b4f0044656650377b2ba2ad7de381cbde0347cbb..2cf9707c7ca836737fb477f4a1f23b3ea2cd390d
100644
--- a/Source/core/inspector/InspectorOverlay.cpp
+++ b/Source/core/inspector/InspectorOverlay.cpp
@@ -111,7 +111,7 @@ protected:
     virtual FloatPoint translatePoint(const FloatPoint& point)
     {
         FloatPoint rendererPoint =
m_shapeOutsideInfo.shapeToRendererPoint(point);
-        return
m_view.contentsToRootView(roundedIntPoint(m_renderer.localToAbsolute(rendererPoint)));
+        return
m_view.contentsToRootFrame(roundedIntPoint(m_renderer.localToAbsolute(rendererPoint)));
     }
 
 private:
@@ -237,12 +237,12 @@ private:
     RefPtr<JSONArray> m_highlightPaths;
 };
 
-static void contentsQuadToScreen(const FrameView* view, FloatQuad& quad)
+static void contentsQuadToRootFrame(const FrameView* view, FloatQuad& quad)
 {
-    quad.setP1(view->contentsToRootView(roundedIntPoint(quad.p1())));
-    quad.setP2(view->contentsToRootView(roundedIntPoint(quad.p2())));
-    quad.setP3(view->contentsToRootView(roundedIntPoint(quad.p3())));
-    quad.setP4(view->contentsToRootView(roundedIntPoint(quad.p4())));
+    quad.setP1(view->contentsToRootFrame(roundedIntPoint(quad.p1())));
+    quad.setP2(view->contentsToRootFrame(roundedIntPoint(quad.p2())));
+    quad.setP3(view->contentsToRootFrame(roundedIntPoint(quad.p3())));
+    quad.setP4(view->contentsToRootFrame(roundedIntPoint(quad.p4())));
 }
 
 static bool buildNodeQuads(LayoutObject* renderer, FloatQuad* content,
FloatQuad* padding, FloatQuad* border, FloatQuad* margin)
@@ -295,10 +295,10 @@ static bool buildNodeQuads(LayoutObject* renderer,
FloatQuad* content, FloatQuad
     *border = renderer->localToAbsoluteQuad(FloatRect(borderBox));
     *margin = renderer->localToAbsoluteQuad(FloatRect(marginBox));
 
-    contentsQuadToScreen(containingView, *content);
-    contentsQuadToScreen(containingView, *padding);
-    contentsQuadToScreen(containingView, *border);
-    contentsQuadToScreen(containingView, *margin);
+    contentsQuadToRootFrame(containingView, *content);
+    contentsQuadToRootFrame(containingView, *padding);
+    contentsQuadToRootFrame(containingView, *border);
+    contentsQuadToRootFrame(containingView, *margin);
 
     return true;
 }
@@ -318,7 +318,7 @@ static void buildNodeHighlight(Node& node, const
HighlightConfig& highlightConfi
         FrameView* containingView = renderer->frameView();
         for (size_t i = 0; i < quads.size(); ++i) {
             if (containingView)
-                contentsQuadToScreen(containingView, quads[i]);
+                contentsQuadToRootFrame(containingView, quads[i]);
             highlight->appendQuad(quads[i], highlightConfig.content,
highlightConfig.contentOutline);
         }
         return;
@@ -605,7 +605,7 @@ static const ShapeOutsideInfo* shapeOutsideInfoForNode(Node*
node, Shape::Displa
 
     LayoutRect shapeBounds =
shapeOutsideInfo->computedShapePhysicalBoundingBox();
     *bounds = renderBox->localToAbsoluteQuad(FloatRect(shapeBounds));
-    contentsQuadToScreen(containingView, *bounds);
+    contentsQuadToRootFrame(containingView, *bounds);
 
     return shapeOutsideInfo;
 }
@@ -830,7 +830,7 @@ bool InspectorOverlay::getBoxModel(Node* node,
RefPtr<TypeBuilder::DOM::BoxModel
     if (!buildNodeQuads(node->renderer(), &content, &padding, &border,
&margin))
         return false;
 
-    IntRect boundingBox =
pixelSnappedIntRect(view->contentsToRootView(renderer->absoluteBoundingBoxRect()));
+    IntRect boundingBox =
pixelSnappedIntRect(view->contentsToRootFrame(renderer->absoluteBoundingBoxRect()));
     RenderBoxModelObject* modelObject = renderer->isBoxModelObject() ?
toRenderBoxModelObject(renderer) : nullptr;
 
     model = TypeBuilder::DOM::BoxModel::create()

[commit-bot: I haz the power, 2015-02-23 18:15:47]

The CQ bit was unchecked by commit-bot@chromium.org

[bokan, 2015-02-23 18:59:52]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2015-02-23 19:00:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/919423002/200001

[commit-bot: I haz the power, 2015-02-23 22:07:51]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190693