Log to UMA when x-origin frames (1) are created and (2) become visible

third_party/WebKit/Source/core/frame/FrameView.cpp

Index: third_party/WebKit/Source/core/frame/FrameView.cpp
diff --git a/third_party/WebKit/Source/core/frame/FrameView.cpp b/third_party/WebKit/Source/core/frame/FrameView.cpp
index 3fa8e63c185b071c4f343796b89e2d2f3ac3c1be..dbf348daedac03f65fc542a7c6ea429f8a769f11 100644
--- a/third_party/WebKit/Source/core/frame/FrameView.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameView.cpp
@@ -4182,6 +4182,7 @@ void FrameView::updateViewportIntersectionsForSubtree(DocumentLifecycle::Lifecyc
     if (shouldNotify && !m_renderThrottlingObserverNotificationFactory->isPending())
         m_frame->frameScheduler()->unthrottledTaskRunner()->postTask(BLINK_FROM_HERE, m_renderThrottlingObserverNotificationFactory->cancelAndCreate());
 
+    // TODO(dgrogan): Document why we need this check. For cycles in the frame tree?

[Sami, 2016/08/08 11:09:47]

This is so that we don't traverse the frame tree unnecessarily.

[dgrogan, 2016/08/08 17:45:48]

It what cases will it be triggered though?

[ojan, 2016/08/08 22:01:53]

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...

It's setting dirty bits on the ancestor chain so it knows to walk down the
subtree to a specific frame. THis way we only traverse the subtrees of the frame
tree that have a dirty frame.

In either case, I'd leave out this TODO. I don't think committing a TODO to
leave a comment is very worthwhile. Feel free to add a comment by the member
variable definitions in the header, but I wouldn't bother adding a TODO.

     if (!m_needsUpdateViewportIntersectionInSubtree)
         return;
     m_needsUpdateViewportIntersectionInSubtree = false;
@@ -4197,6 +4198,7 @@ void FrameView::updateViewportIntersectionsForSubtree(DocumentLifecycle::Lifecyc
 void FrameView::updateThrottlingStatus()
 {
     // Only offscreen frames can be throttled.
+    // TODO(dgrogan): Change this to DCHECK(m_viewportIntersectionValid)?

[Sami, 2016/08/08 11:09:47]

Right, we should be able to require that.

[ojan, 2016/08/08 22:01:54]

Same as above, either add the DCHECK or leave the whole thing out. Adding
comments like this clutters the code.

[dgrogan, 2016/08/09 00:30:15]

Sorry, it wasn't clear at all, but these comments weren't meant to survive
through to the final patchset. They're more placeholders for us to discuss
stuff. I spun this one out to its own tiny patch already.

     m_hiddenForThrottling = m_viewportIntersectionValid && m_viewportIntersection.isEmpty();
 
     // We only throttle the rendering pipeline in cross-origin frames. This is
@@ -4235,6 +4237,8 @@ void FrameView::notifyRenderThrottlingObservers()
 
     updateThrottlingStatus();
 
+    frame().onVisibilityMaybeChanged(!m_hiddenForThrottling);
+
     bool becameThrottled = !wasThrottled && canThrottleRendering();
     bool becameUnthrottled = wasThrottled && !canThrottleRendering();
     ScrollingCoordinator* scrollingCoordinator = this->scrollingCoordinator();