Rename RFO::FrameWillClose() to reflect its actual purpose.

Rename RFO::FrameWillClose() to reflect its actual purpose.

Frame detach and frame close don't really have any relationship. Update
the naming and comments to make this clear.

BUG=472140
Committed: https://crrev.com/dbf1570978e01431602502e73b19ed69d8f503a2
Cr-Commit-Position: refs/heads/master@{#422865}

[dcheng, 2016-09-30 05:37:57]

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

[commit-bot: I haz the power, 2016-09-30 05:38:16]

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

[dcheng, 2016-09-30 05:40:28]

Description was changed from

==========
Rename RFO::FrameWillCLose() to reflect its actual purpose.

Frame detach and frame close don't really have any relationship. Update
the naming and comments to make this clear.

BUG=none
==========

to

==========
Rename RFO::FrameWillClose() to reflect its actual purpose.

Frame detach and frame close don't really have any relationship. Update
the naming and comments to make this clear.

BUG=none
==========

[dcheng, 2016-09-30 05:46:31]

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

[commit-bot: I haz the power, 2016-09-30 05:46:56]

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-09-30 06:08:23]

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

[commit-bot: I haz the power, 2016-09-30 06:08:24]

Dry run: Exceeded global retry quota

[dcheng, 2016-09-30 06:14:59]

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

[commit-bot: I haz the power, 2016-09-30 06:15:15]

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

[dcheng, 2016-09-30 06:18:21]

Description was changed from

==========
Rename RFO::FrameWillClose() to reflect its actual purpose.

Frame detach and frame close don't really have any relationship. Update
the naming and comments to make this clear.

BUG=none
==========

to

==========
Rename RFO::FrameWillClose() to reflect its actual purpose.

Frame detach and frame close don't really have any relationship. Update
the naming and comments to make this clear.

BUG=472140
==========

[dcheng, 2016-09-30 06:18:21]

dcheng@chromium.org changed reviewers:
+ csharrison@chromium.org, jochen@chromium.org, miu@chromium.org,
vabr@chromium.org

[dcheng, 2016-09-30 06:20:42]

+csharrison for chrome/renderer/page_load_histograms.*
+vabr for components/autofill
+miu for content/renderer/media
+jochen for chrome/renderer/chrome_content_renderer_client.*, content/public,
and content/renderer/render_frame_impl.* changes.

https://codereview.chromium.org/2379293002/diff/40001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2379293002/diff/40001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:542: DCHECK(frame && !frame->parent());
In theory this should always be true. Hopefully the trybots agree.

https://codereview.chromium.org/2379293002/diff/40001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.h (right):

https://codereview.chromium.org/2379293002/diff/40001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.h:30: void FrameDetached() override;
Frame detached should be called before a WebView is closed, so this should be
OK. That being said...are there any tests that I can use to verify that this is
OK?

https://codereview.chromium.org/2379293002/diff/40001/content/public/renderer...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/2379293002/diff/40001/content/public/renderer...
content/public/renderer/render_frame_observer.h:63: virtual void
WillCommitProvisionalLoad() {}
Looking at how this is used, I think these could all just be folded into
DidCommitProvisionalLoad. I'd like to try that in a followup CL, if it makes
sense to everyone.

[miu, 2016-09-30 06:41:30]

content/renderer/media lgtm

[commit-bot: I haz the power, 2016-09-30 06:50:24]

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

[commit-bot: I haz the power, 2016-09-30 06:50:25]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dcheng, 2016-09-30 08:17:18]

Unfortunately, it looks like PageLoadHistograms tries to read a bunch of random
values off the frame.

I'm going to have to think about how to handle this properly: I might just punt
on the problem by making it both a RenderFrameObserver and RenderViewObserver,
keeping its current dependency on ClosePage() =/

[vabr (Chromium), 2016-09-30 11:47:51]

Hi dcheng@,
Based on your last update, I'm not sure if you are still asking for review or
not. The renaming itself looks good to me for password_autofill_agent.* However,
I am confused by the different meaning hinted on by the old and new name of the
method. FrameWillClose indicates that it is called every time a frame is closed
(with the exception of child frames mentioned in the comment).
WillCommitProvisionalLoad sounds like being called only when the frame is
replaced due to a navigation. Could you please help me understand this?

Cheers,
Vaclav

[Charlie Harrison, 2016-09-30 12:20:51]

On 2016/09/30 08:17:18, dcheng wrote:
> Unfortunately, it looks like PageLoadHistograms tries to read a bunch of
random
> values off the frame.
> 
> I'm going to have to think about how to handle this properly: I might just
punt
> on the problem by making it both a RenderFrameObserver and RenderViewObserver,
> keeping its current dependency on ClosePage() =/

A while ago I tried to make PageLoadHistograms a RFO and hit the same problem :(
I punted on the whole CL.

Fortunately we are planning on deleting this entire class soon (see
crbug.com/384330)

[dcheng, 2016-09-30 15:30:22]

On 2016/09/30 11:47:51, vabr (Chromium) wrote:
> Hi dcheng@,
> Based on your last update, I'm not sure if you are still asking for review or
> not. The renaming itself looks good to me for password_autofill_agent.*
However,
> I am confused by the different meaning hinted on by the old and new name of
the
> method. FrameWillClose indicates that it is called every time a frame is
closed
> (with the exception of child frames mentioned in the comment).
> WillCommitProvisionalLoad sounds like being called only when the frame is
> replaced due to a navigation. Could you please help me understand this?
> 
> Cheers,
> Vaclav

Yes, I am still asking for a review. I'm restructing the PageLoadHistograms
portion of the change to make this work, but it will be isolated to that code.

As for the naming... the naming is accurate. The old comments were incorrect.
There's been no recent functional change in this callback: the comment has just
been wrong for a very long time. The password autofill code really just wants to
know when a document is going away, which means it needs to listen to
WillCommitProvisionalLoad (for navigations) and FrameDetached (for when the
entire frame is detached)

[dcheng, 2016-09-30 15:30:59]

On 2016/09/30 12:20:51, Charlie Harrison OOO Sep 28-29 wrote:
> On 2016/09/30 08:17:18, dcheng wrote:
> > Unfortunately, it looks like PageLoadHistograms tries to read a bunch of
> random
> > values off the frame.
> > 
> > I'm going to have to think about how to handle this properly: I might just
> punt
> > on the problem by making it both a RenderFrameObserver and
RenderViewObserver,
> > keeping its current dependency on ClosePage() =/
> 
> A while ago I tried to make PageLoadHistograms a RFO and hit the same problem
:(
> I punted on the whole CL.
> 
> Fortunately we are planning on deleting this entire class soon (see
> crbug.com/384330)

I'm just going to embed a small RVO inside: removing this method from RVO still
seems like an overall win, unless PageLoadHistograms is going to be deleted
really really soon now =)

[Charlie Harrison, 2016-09-30 15:34:27]

On 2016/09/30 15:30:59, dcheng wrote:
> On 2016/09/30 12:20:51, Charlie Harrison OOO Sep 28-29 wrote:
> > On 2016/09/30 08:17:18, dcheng wrote:
> > > Unfortunately, it looks like PageLoadHistograms tries to read a bunch of
> > random
> > > values off the frame.
> > > 
> > > I'm going to have to think about how to handle this properly: I might just
> > punt
> > > on the problem by making it both a RenderFrameObserver and
> RenderViewObserver,
> > > keeping its current dependency on ClosePage() =/
> > 
> > A while ago I tried to make PageLoadHistograms a RFO and hit the same
problem
> :(
> > I punted on the whole CL.
> > 
> > Fortunately we are planning on deleting this entire class soon (see
> > crbug.com/384330)
> 
> I'm just going to embed a small RVO inside: removing this method from RVO
still
> seems like an overall win, unless PageLoadHistograms is going to be deleted
> really really soon now =)

SGTM. We're going to try to delete this class post M55 branch.

[vabr (Chromium), 2016-09-30 15:36:13]

On 2016/09/30 15:30:22, dcheng wrote:
> On 2016/09/30 11:47:51, vabr (Chromium) wrote:
> > Hi dcheng@,
> > Based on your last update, I'm not sure if you are still asking for review
or
> > not. The renaming itself looks good to me for password_autofill_agent.*
> However,
> > I am confused by the different meaning hinted on by the old and new name of
> the
> > method. FrameWillClose indicates that it is called every time a frame is
> closed
> > (with the exception of child frames mentioned in the comment).
> > WillCommitProvisionalLoad sounds like being called only when the frame is
> > replaced due to a navigation. Could you please help me understand this?
> > 
> > Cheers,
> > Vaclav
> 
> Yes, I am still asking for a review. I'm restructing the PageLoadHistograms
> portion of the change to make this work, but it will be isolated to that code.
> 
> As for the naming... the naming is accurate. The old comments were incorrect.
> There's been no recent functional change in this callback: the comment has
just
> been wrong for a very long time. The password autofill code really just wants
to
> know when a document is going away, which means it needs to listen to
> WillCommitProvisionalLoad (for navigations) and FrameDetached (for when the
> entire frame is detached)

Thanks for the explanation!
In that case LGTM, having the renaming is good for clarity.
And the autofill code indeed observes both WillCommitProvisionalLoad and
FrameDetached to detect frames going away, so all looks good.

[dcheng, 2016-09-30 17:13:57]

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

[dcheng, 2016-09-30 17:14:35]

PTAL, added the RVO

[commit-bot: I haz the power, 2016-09-30 17:14:38]

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-09-30 17:56:59]

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

[commit-bot: I haz the power, 2016-09-30 17:57:00]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-09-30 18:56:56]

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:629: void
PageLoadHistograms::Helper::OnDestruct() {
Why is this necessary? Might make sense to add a comment.

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.h (right):

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.h:58: // to inherit from both. Embed the
RVO in a small helper class instead.
"both RVO and RFO"

[jochen (gone - plz use gerrit), 2016-10-04 09:32:44]

lgtm with comment

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.h (right):

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.h:61: explicit Helper(PageLoadHistograms*
histograms);
should there be a virtual dtor as well?

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.h:70: };
disallow_copy_and_assign

[dcheng, 2016-10-04 17:16:29]

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:629: void
PageLoadHistograms::Helper::OnDestruct() {
On 2016/09/30 18:56:56, Charlie Harrison wrote:
> Why is this necessary? Might make sense to add a comment.

RVO defines this as pure virtual? Not sure that I should comment that, since
that seems like a property of the RVO interface?

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.h (right):

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.h:58: // to inherit from both. Embed the
RVO in a small helper class instead.
On 2016/09/30 18:56:56, Charlie Harrison wrote:
> "both RVO and RFO"

Done.

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.h:61: explicit Helper(PageLoadHistograms*
histograms);
On 2016/10/04 09:32:44, jochen (slow) wrote:
> should there be a virtual dtor as well?

RVO already has a virtual dtor, so it's not necessary to repeat it here.

clang (usually) warns if you forget to have a virtual dtor here.

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.h:70: };
On 2016/10/04 09:32:44, jochen (slow) wrote:
> disallow_copy_and_assign

Done.

[dcheng, 2016-10-04 17:17:23]

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

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

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

[Charlie Harrison, 2016-10-04 17:20:43]

LGTM thanks

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2379293002/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:629: void
PageLoadHistograms::Helper::OnDestruct() {
On 2016/10/04 17:16:28, dcheng wrote:
> On 2016/09/30 18:56:56, Charlie Harrison wrote:
> > Why is this necessary? Might make sense to add a comment.
> 
> RVO defines this as pure virtual? Not sure that I should comment that, since
> that seems like a property of the RVO interface?

Oops yes that's my mistake.

[dcheng, 2016-10-04 17:23:08]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2016-10-04 17:23:11]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-10-04 17:23:12]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
miu@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2379293002/#ps80001
(title: "Address comments.")

[dcheng, 2016-10-04 17:23:18]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-10-04 17:23:48]

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-10-04 17:30:53]

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

[commit-bot: I haz the power, 2016-10-04 17:30:55]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2016-10-04 17:42:20]

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

[dcheng, 2016-10-04 17:42:27]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-10-04 17:42:27]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
jochen@chromium.org, miu@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2379293002/#ps100001
(title: "Fix silly typo")

[commit-bot: I haz the power, 2016-10-04 17:42:52]

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-10-04 18:38:52]

Description was changed from

==========
Rename RFO::FrameWillClose() to reflect its actual purpose.

Frame detach and frame close don't really have any relationship. Update
the naming and comments to make this clear.

BUG=472140
==========

to

==========
Rename RFO::FrameWillClose() to reflect its actual purpose.

Frame detach and frame close don't really have any relationship. Update
the naming and comments to make this clear.

BUG=472140
==========

[commit-bot: I haz the power, 2016-10-04 18:38:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-04 18:42:14]

Description was changed from

==========
Rename RFO::FrameWillClose() to reflect its actual purpose.

Frame detach and frame close don't really have any relationship. Update
the naming and comments to make this clear.

BUG=472140
==========

to

==========
Rename RFO::FrameWillClose() to reflect its actual purpose.

Frame detach and frame close don't really have any relationship. Update
the naming and comments to make this clear.

BUG=472140

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

[commit-bot: I haz the power, 2016-10-04 18:42:16]

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