Fix a bug where an infinite stream of BeginFrame messages are sent to extensions.

Fix a bug where an infinite stream of BeginFrame messages are sent to extensions.

|needs_begin_frames| is a parameter that needs to be synchronized between the
browser and renderers. A logic mistake caused this parameter to be out of sync,
causing the browser to think that every extension required an infinite stream of
BeginFrame messages.

BUG=673021
Committed: https://crrev.com/e33ff53b1fe079c6243d6beaa4ec7e68cfec35fd
Cr-Commit-Position: refs/heads/master@{#438930}

[erikchen, 2016-12-15 01:19:59]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[erikchen, 2016-12-15 01:20:17]

erikchen@chromium.org changed reviewers:
+ avi@chromium.org, danakj@chromium.org

[erikchen, 2016-12-15 01:20:17]

avi, danakj: Please review.

[commit-bot: I haz the power, 2016-12-15 01:20:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 01:27:30]

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

[commit-bot: I haz the power, 2016-12-15 01:27:32]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[danakj, 2016-12-15 15:56:59]

https://codereview.chromium.org/2577953002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2577953002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:488: if (rvh) {
So, when is this null? I got as far RenderViewHostImpl seems to always call
set_owner_delegate() which is how we get this |rvh| here..
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_vie...).
When do we have a RWHVMac but there's not the owner delegate?

https://codereview.chromium.org/2577953002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:504: // A renderer
wants begin frames initially if and only if
IsNeverVisible should not change, thats why its name includes "Never". I would
reword this a bit. Something like "Any renderer that will produce frames needs
to have begin frames sent to it. So unless it is never visible, start this value
at true here to avoid startup raciness and decrease latency."?

[erikchen, 2016-12-15 16:51:11]

https://codereview.chromium.org/2577953002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2577953002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:488: if (rvh) {
On 2016/12/15 15:56:59, danakj_OOO_and_slow wrote:
> So, when is this null? I got as far RenderViewHostImpl seems to always call
> set_owner_delegate() which is how we get this |rvh| here..
>
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_vie...).
> When do we have a RWHVMac but there's not the owner delegate?

I agree that I don't see how rvh could be nullptr. At the same time, note
"""ignore_result(rvh->GetWebkitPreferences());""" seems pretty suspicious too.
That line should perhaps be removed as well. I've added a comment to the
original CL:
https://codereview.chromium.org/2163953002/#msg25

If you think I should remove the conditional, I can do that, but I would want to
do it in a follow up CL, since I intend to merge this to M-56 and I want this CL
to be simple as possible.

https://codereview.chromium.org/2577953002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:504: // A renderer
wants begin frames initially if and only if
On 2016/12/15 15:56:59, danakj_OOO_and_slow wrote:
> IsNeverVisible should not change, thats why its name includes "Never". I would
> reword this a bit. Something like "Any renderer that will produce frames needs
> to have begin frames sent to it. So unless it is never visible, start this
value
> at true here to avoid startup raciness and decrease latency."?

Done.

[erikchen, 2016-12-15 16:51:27]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 16:51:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 17:10:53]

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

[commit-bot: I haz the power, 2016-12-15 17:10:54]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-12-15 17:30:10]

ok thanks, let's remove it in another CL then. LGTM

https://codereview.chromium.org/2577953002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2577953002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:488: if (rvh) {
Can you add a TODO to remove this condition?

https://codereview.chromium.org/2577953002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:503: //
nit: just one line between paragraphs

[danakj, 2016-12-15 17:30:25]

btw there's a similar block in RWHVAura also, with the same TODO

[erikchen, 2016-12-15 19:28:22]

https://codereview.chromium.org/2577953002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2577953002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:488: if (rvh) {
On 2016/12/15 17:30:10, danakj_OOO_and_slow wrote:
> Can you add a TODO to remove this condition?

Done.

https://codereview.chromium.org/2577953002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:503: //
On 2016/12/15 17:30:10, danakj_OOO_and_slow wrote:
> nit: just one line between paragraphs

Done.

[erikchen, 2016-12-15 19:28:27]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 19:28:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-12-15 21:03:48]

lgtm

[commit-bot: I haz the power, 2016-12-15 21:03:50]

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

[commit-bot: I haz the power, 2016-12-15 21:03:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-12-15 21:10:48]

On 2016/12/15 21:03:48, Avi wrote:
> lgtm

removed TODOs, as conditionals are necessary.

[erikchen, 2016-12-15 21:10:53]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-12-15 21:10:53]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2577953002/#ps60001
(title: "Remove TODOs.")

[commit-bot: I haz the power, 2016-12-15 21:11:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 21:39:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481836253226840,
"parent_rev": "55fcae67c65ebd92b8c950efd5fc81e9bf26f823", "commit_rev":
"eda3372da421b7edf9bb9c70bb952489b2ecc085"}

[commit-bot: I haz the power, 2016-12-15 21:40:26]

Description was changed from

==========
Fix a bug where an infinite stream of BeginFrame messages are sent to
extensions.

|needs_begin_frames| is a parameter that needs to be synchronized between the
browser and renderers. A logic mistake caused this parameter to be out of sync,
causing the browser to think that every extension required an infinite stream of
BeginFrame messages.

BUG=673021
==========

to

==========
Fix a bug where an infinite stream of BeginFrame messages are sent to
extensions.

|needs_begin_frames| is a parameter that needs to be synchronized between the
browser and renderers. A logic mistake caused this parameter to be out of sync,
causing the browser to think that every extension required an infinite stream of
BeginFrame messages.

BUG=673021

Review-Url: https://codereview.chromium.org/2577953002
==========

[commit-bot: I haz the power, 2016-12-15 21:40:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-15 21:42:39]

Description was changed from

==========
Fix a bug where an infinite stream of BeginFrame messages are sent to
extensions.

|needs_begin_frames| is a parameter that needs to be synchronized between the
browser and renderers. A logic mistake caused this parameter to be out of sync,
causing the browser to think that every extension required an infinite stream of
BeginFrame messages.

BUG=673021

Review-Url: https://codereview.chromium.org/2577953002
==========

to

==========
Fix a bug where an infinite stream of BeginFrame messages are sent to
extensions.

|needs_begin_frames| is a parameter that needs to be synchronized between the
browser and renderers. A logic mistake caused this parameter to be out of sync,
causing the browser to think that every extension required an infinite stream of
BeginFrame messages.

BUG=673021

Committed: https://crrev.com/e33ff53b1fe079c6243d6beaa4ec7e68cfec35fd
Cr-Commit-Position: refs/heads/master@{#438930}
==========

[commit-bot: I haz the power, 2016-12-15 21:42:40]

Patchset 4 (id:??) landed as
https://crrev.com/e33ff53b1fe079c6243d6beaa4ec7e68cfec35fd
Cr-Commit-Position: refs/heads/master@{#438930}