Be a little more precise in the wording of comments for process death.

Be a little more precise in the wording of comments for process death.

BUG=508652
TEST=no change to functionality

Committed: https://crrev.com/3360120bb302ba6f63f1d07800f0110ea6f75e7a
Cr-Commit-Position: refs/heads/master@{#338422}

[Avi (use Gerrit), 2015-07-09 22:08:28]

avi@chromium.org changed reviewers:
+ paulmeyer@chromium.org

[Avi (use Gerrit), 2015-07-09 22:08:29]

Note that you're not going to get a clean close notification from the WCO,
because by that time the renderer will be switched out.

I tweaked the comments to be clearer.

[nasko, 2015-07-10 07:48:54]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2015-07-10 07:48:55]

Unsolicited drive-by comments.

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
File content/public/browser/render_process_host_observer.h (right):

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
content/public/browser/render_process_host_observer.h:25: // active renderer
processes; for code that needs to be a WebContentsObserver
This isn't quite correct I think. Last time I poked at it,
WCO::RenderProcessGone is called only for the top-level frame, even with
--site-per-process. It isn't quite clear what the behavior in that mode should
be and whether we need overlap between these two observer interfaces.

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:100: // This method is invoked
when the process for the current RenderView exits
"the process for the current main RenderFrameHost". Also, can we go with
s/RenderViewHost/RenderFrameHost/ in this entire comment, since you are updating
it anyway?

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:108: // otherwise need to be a
WebContentsObserver, consider whether that API might
I'd word it a bit more strongly. Unless they have to be WCO, they are better off
monitoring for process crashing through the process observer :).

[paulmeyer, 2015-07-10 13:33:42]

On 2015/07/09 22:08:29, Avi wrote:
> Note that you're not going to get a clean close notification from the WCO,
> because by that time the renderer will be switched out.
> 
> I tweaked the comments to be clearer.

Is that the only case when RPHO will notify but WCO won't? I think it could
still be clearer in the comments exactly when they deviate.

[Avi (use Gerrit), 2015-07-10 15:18:05]

On 2015/07/10 13:33:42, Paul Meyer wrote:
> On 2015/07/09 22:08:29, Avi wrote:
> > Note that you're not going to get a clean close notification from the WCO,
> > because by that time the renderer will be switched out.
> > 
> > I tweaked the comments to be clearer.
> 
> Is that the only case when RPHO will notify but WCO won't? I think it could
> still be clearer in the comments exactly when they deviate.

Here's the problem: I don't know, and I'm pretty sure no one else does either.

These comments are as precise as I can make them, to the best of my knowledge.
I'm _pretty sure_ that that's the difference, but I've just spent the past few
months rewiring navigation, and I'm 100% certain that there are some weird
edge-cases in the wild that I'm not considering.

My advice is: write to the comments. That's what everyone else does, so when we
find a weird edge case, we'll fix your code along with everyone else's.

[paulmeyer, 2015-07-10 15:22:30]

On 2015/07/10 15:18:05, Avi wrote:
> On 2015/07/10 13:33:42, Paul Meyer wrote:
> > On 2015/07/09 22:08:29, Avi wrote:
> > > Note that you're not going to get a clean close notification from the WCO,
> > > because by that time the renderer will be switched out.
> > > 
> > > I tweaked the comments to be clearer.
> > 
> > Is that the only case when RPHO will notify but WCO won't? I think it could
> > still be clearer in the comments exactly when they deviate.
> 
> Here's the problem: I don't know, and I'm pretty sure no one else does either.
> 
> These comments are as precise as I can make them, to the best of my knowledge.
> I'm _pretty sure_ that that's the difference, but I've just spent the past few
> months rewiring navigation, and I'm 100% certain that there are some weird
> edge-cases in the wild that I'm not considering.
> 
> My advice is: write to the comments. That's what everyone else does, so when
we
> find a weird edge case, we'll fix your code along with everyone else's.

Okay, LGTM.

[Avi (use Gerrit), 2015-07-10 15:29:20]

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
File content/public/browser/render_process_host_observer.h (right):

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
content/public/browser/render_process_host_observer.h:25: // active renderer
processes; for code that needs to be a WebContentsObserver
On 2015/07/10 07:48:54, nasko (paris) wrote:
> This isn't quite correct I think. Last time I poked at it,
> WCO::RenderProcessGone is called only for the top-level frame, even with
> --site-per-process. It isn't quite clear what the behavior in that mode should
> be and whether we need overlap between these two observer interfaces.

I see that. WebContentsImpl::RenderViewTerminated does a check at the top to
eliminate the pending RVH, but that will also eliminate all non-top-level RVHs.
I will adjust the comment.

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:100: // This method is invoked
when the process for the current RenderView exits
On 2015/07/10 07:48:54, nasko (paris) wrote:
> "the process for the current main RenderFrameHost". Also, can we go with
> s/RenderViewHost/RenderFrameHost/ in this entire comment, since you are
updating
> it anyway?

Done.

https://codereview.chromium.org/1228993005/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:108: // otherwise need to be a
WebContentsObserver, consider whether that API might
On 2015/07/10 07:48:55, nasko (paris) wrote:
> I'd word it a bit more strongly. Unless they have to be WCO, they are better
off
> monitoring for process crashing through the process observer :).

Done.

[Avi (use Gerrit), 2015-07-11 00:49:20]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-07-11 00:49:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
paulmeyer@chromium.org
Link to the patchset: https://codereview.chromium.org/1228993005/#ps40001
(title: "updates")

[commit-bot: I haz the power, 2015-07-11 00:49:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228993005/40001

[commit-bot: I haz the power, 2015-07-11 01:39:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-11 01:40:28]

Patchset 3 (id:??) landed as
https://crrev.com/3360120bb302ba6f63f1d07800f0110ea6f75e7a
Cr-Commit-Position: refs/heads/master@{#338422}