Add WasShown/WasHidden to RenderFrameObserver.

Add WasShown/WasHidden to RenderFrameObserver.

BUG=304341
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273375

[nasko, 2014-05-27 23:31:53]

Hey John,
Can you take a look at this patch? It implements what we discussed last week
about visibility at the WebContents level.
Also, I don't have an idea for a good end-to-end test to ensure
RenderFrameObservers are actually getting notified. I've added a check for the
method to collect the set of RWHs, but I don't know if there is a way to add
code in RFO. I could simulate the renderer site in a unit test, but wanted to
see if you know of a better way.
Thanks!
Nasko

[jam, 2014-05-28 17:55:37]

lgtm

On 2014/05/27 23:31:53, nasko wrote:
> Hey John,
> Can you take a look at this patch? It implements what we discussed last week
> about visibility at the WebContents level.
> Also, I don't have an idea for a good end-to-end test to ensure
> RenderFrameObservers are actually getting notified. I've added a check for the
> method to collect the set of RWHs, but I don't know if there is a way to add
> code in RFO. I could simulate the renderer site in a unit test, but wanted to
> see if you know of a better way.

I think the only way would be a browser test and also changes to content/shell
so that there is a new testing method which tells the rendererers to bounce back
all WasShown/WasHidden notifications. i.e. there's no way to do this just by
adding a browser test file, since it can't run C++ code in the renderer. As an
aside, there's PostTaskToInProcessRendererAndWait but that only works in
single-process mode, which is useless for testing oopif.


> Thanks!
> Nasko

https://codereview.chromium.org/298283003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/298283003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:17: #include
"content/browser/renderer_host/render_widget_host_impl.h"
nit: personally i wouldn't include such big headers in another header if i can
avoid it, and instead i would move the implementation of the method to the cc
file (and rename it to GetRenderWidgetHostImpl accordingly)

https://codereview.chromium.org/298283003/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/298283003/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:2126: 
nit: extra line

[nasko, 2014-05-28 18:26:11]

Fixes.

https://codereview.chromium.org/298283003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/298283003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:17: #include
"content/browser/renderer_host/render_widget_host_impl.h"
On 2014/05/28 17:55:37, jam wrote:
> nit: personally i wouldn't include such big headers in another header if i can
> avoid it, and instead i would move the implementation of the method to the cc
> file (and rename it to GetRenderWidgetHostImpl accordingly)

Done.

https://codereview.chromium.org/298283003/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/298283003/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:2126: 
On 2014/05/28 17:55:37, jam wrote:
> nit: extra line

Done.

[nasko, 2014-05-28 18:26:19]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-28 18:27:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nasko@chromium.org/298283003/40001

[nasko, 2014-05-28 18:51:30]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-28 18:53:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nasko@chromium.org/298283003/50011

[commit-bot: I haz the power, 2014-05-28 22:13:07]

Change committed as 273375