Remove RWHV::SetBounds() from the public API, and make comments clearer.

content/public/browser/render_widget_host_view.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_PUBLIC_BROWSER_RENDER_WIDGET_HOST_VIEW_H_
 #define CONTENT_PUBLIC_BROWSER_RENDER_WIDGET_HOST_VIEW_H_
 
 #include <memory>
 
 #include "base/strings/string16.h"
@@ skipping 36 matching lines @@
   virtual ~RenderWidgetHostView() {}
 
   // Initialize this object for use as a drawing area.  |parent_view| may be
   // left as nullptr on platforms where a parent view is not required to
   // initialize a child window.
   virtual void InitAsChild(gfx::NativeView parent_view) = 0;
 
   // Returns the associated RenderWidgetHost.
   virtual RenderWidgetHost* GetRenderWidgetHost() const = 0;
 
-  // Tells the View to size itself to the specified size.
+  // Tells the View to size itself, or its top-level window (impl-dependent), to
+  // the specified size in screen coordinates. Note that this will not take
+  // effect permanently if this View's native view is being managed by the auto-
+  // layout heuristics of the native UI toolkit.
   virtual void SetSize(const gfx::Size& size) = 0;
 
-  // Tells the View to size and move itself to the specified size and point in
-  // screen space.
-  virtual void SetBounds(const gfx::Rect& rect) = 0;
-
   // Retrieves the last known scroll position.
   virtual gfx::Vector2dF GetLastScrollOffset() const = 0;
 
   // Sets a flag that indicates if it is in virtual reality mode.
   virtual void SetIsInVR(bool is_in_vr) = 0;
 
   // Coordinate points received from a renderer process need to be transformed
   // to the top-level frame's coordinate space. For coordinates received from
   // the top-level frame's renderer this is a no-op as they are already
   // properly transformed; however, coordinates received from an out-of-process
@@ skipping 34 matching lines @@
   virtual bool IsShowing() = 0;
 
   // Indicates if the view is currently occluded (e.g, not visible because it's
   // covered up by other windows), and as a result the view's renderer may be
   // suspended. If Show() is called on a view then its state should be re-set to
   // being un-occluded (an explicit WasUnOccluded call will not be made for
   // that). These calls are not necessarily made in pairs.
   virtual void WasUnOccluded() = 0;
   virtual void WasOccluded() = 0;
 
-  // Retrieve the bounds of the View, in screen coordinates.
+  // Retrieves the bounds of the View, in screen coordinates. Note that this
+  // reflects the region the view occupies on-screen, accounting for any scaling
+  // performed on the native view by the platform or UI toolkit. Callers should
+  // NOT assume the size of this region is equal to to the View's actual
+  // rendering size, and NOT assume a device scale factor will resolve this

[Nico, 2017/02/17 22:05:11]

I have a hard time understanding this comment. Maybe an example with actual
numbers might help?

[miu, 2017/02/17 23:50:59]

Example on Mac: The RWHV's native view (NSView) will be added as a subview in
the view hierarchy in the browser window. Somewhere, one of its ancestor NSViews
may have a coordinate transform. One way this can happen is automatic: when UI
code sets a NSView's "bounds" to have a different size than that of its "frame."
In such cases, the RWHV's NSView, a descendant of the "scaled view," will have a
coordinate space being some scale factor of the screen coordinate space. Also,
note that this "scaling" is totally unrelated to the DIP<-->DevicePixels
transforms we do all over the place.

Example on Aura: Similarly, an ancestor aura::Window of the RWHV's aura::Window
could have a gfx::Transform applied to it (via aura::Window::SetTransform()). In
this case, the size from RWHV's aura::Window::GetBounds() will be in a
coordinate system that is not 1:1 with the screen coordinate system.
RWHV::GetViewBounds() *does* already account for this, returning position+size
in screen coordinates.

So far, the above "scaling transforms" are only theoretically possible. Meaning,
[AFAIK] we don't do this with RWHV's or any ancestor native view in a Chrome
browser. However, I know of two (separate) teams working on new UI features that
*do* want to leverage this "transform" functionality provided by the the UI
frameworks to improve user experiences in the media space. For example:
http://crbug.com/692790.

Also, I've found, and begun documenting in http://crbug.com/73362, several areas
of the browser UI code that currently mis-interpret the semantics around
GetViewBounds() and SetSize(). And, there are examples of code that does
correctly interpret the sizes, but is fragile because it employs its own
customized coordinate transforms around these APIs. So, I'm trying to clean all
that up too. :)

+  // discrepancy.

[ncarter (slow), 2017/02/15 23:35:02]

How does this differ from (or relate to) GetVisibleViewportSize()? Should that
comment be clarified?

[miu, 2017/02/16 00:07:07]

Took a look. It seems that there is Ash UI code assuming it's in "app window"
coordinates, while render_widget_host_view_child_frame.cc assumes it's in screen
coordinates, and then render_widget_host_impl.cc assumes it's in view
coordinates (what it sends to Blink in the resize_params).

Ugh, so this needs to be resolved. I'll note this in the crbug for tracking, so
I can address it in a future CL. For now, I won't change the public API comments
because I don't know yet which of the 3 coordinate systems it's supposed to be.

+  //
+  // TODO(miu): Provide a GetRenderingSize() method for use cases that need the
+  // actual DIP rendering size. http://crbug.com/73362
   virtual gfx::Rect GetViewBounds() const = 0;
 
   // Returns true if the View's context menu is showing.
   virtual bool IsShowingContextMenu() const = 0;
 
   // Tells the View whether the context menu is showing.
   virtual void SetShowingContextMenu(bool showing) = 0;
 
   // Returns the currently selected text.
   virtual base::string16 GetSelectedText() = 0;
@@ skipping 63 matching lines @@
   // Returns |true| if text is currently being spoken by Mac OS X.
   virtual bool IsSpeaking() const = 0;
   // Stops speaking, if it is currently in progress.
   virtual void StopSpeaking() = 0;
 #endif  // defined(OS_MACOSX)
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_RENDER_WIDGET_HOST_VIEW_H_