Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727263004:
Add some const to content/. (Closed)
DescriptionAdd some const to content/.
I think this would be nice as documentation, as these classes can be
quite confusing to people outside of content/ experts.
BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2727263004
Cr-Commit-Position: refs/heads/master@{#455768}
Committed: https://chromium.googlesource.com/chromium/src/+/e8442a0169c58e21b309b0afd48ba540b804e5ab
Patch Set 1 #
Total comments: 3
Messages
Total messages: 18 (11 generated)
Description was changed from ========== Add some const to content/. I think this would be nice as documentation, as these classes can be quite confusing to people outside of content/ experts. BUG=none ========== to ========== Add some const to content/. I think this would be nice as documentation, as these classes can be quite confusing to people outside of content/ experts. BUG=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add some const to content/. I think this would be nice as documentation, as these classes can be quite confusing to people outside of content/ experts. BUG=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add some const to content/. I think this would be nice as documentation, as these classes can be quite confusing to people outside of content/ experts. BUG=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
maxmorin@chromium.org changed reviewers: + clamy@chromium.org
Clamy: PTAL.
clamy@chromium.org changed reviewers: + nasko@chromium.org
This looks ok to me, but I'd like to add Nasko as a second reviewer, especially since I'm less familiar with RenderProcessHost. https://codereview.chromium.org/2727263004/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2727263004/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:900: // Cache a pointer to avoid unnecessary process creation. @nasko: the comment here seems weird. It says that we want to avoid calling site_instance_->GetProcess(), but we initialize this member precisely by calling this function. Does it still make sense to keep this member around?
https://codereview.chromium.org/2727263004/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2727263004/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:900: // Cache a pointer to avoid unnecessary process creation. On 2017/03/06 14:38:44, clamy wrote: > @nasko: the comment here seems weird. It says that we want to avoid calling > site_instance_->GetProcess(), but we initialize this member precisely by calling > this function. Does it still make sense to keep this member around? I think the goal here is not to avoid it during initialization. It is perfectly possible for the renderer process to crash and be gone. Any code trying to use the process_ after this point should not accidentally recreate it, as it might not actually be used. If you think this is useful piece of data to put in the comment, I'm more than happy to approve a CL : ).
Thanks! Lgtm. https://codereview.chromium.org/2727263004/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2727263004/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:900: // Cache a pointer to avoid unnecessary process creation. On 2017/03/06 19:46:15, nasko (slow) wrote: > On 2017/03/06 14:38:44, clamy wrote: > > @nasko: the comment here seems weird. It says that we want to avoid calling > > site_instance_->GetProcess(), but we initialize this member precisely by > calling > > this function. Does it still make sense to keep this member around? > > I think the goal here is not to avoid it during initialization. It is perfectly > possible for the renderer process to crash and be gone. Any code trying to use > the process_ after this point should not accidentally recreate it, as it might > not actually be used. > If you think this is useful piece of data to put in the comment, I'm more than > happy to approve a CL : ). I see. We should probably update the comment then.
The CQ bit was checked by maxmorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I'll leave the commenting to you two, since you know this better.
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1489074542381190, "parent_rev":
"f28225ec53772397fd733419aae63268b8219fdf", "commit_rev":
"e8442a0169c58e21b309b0afd48ba540b804e5ab"}
Message was sent while issue was closed.
Description was changed from ========== Add some const to content/. I think this would be nice as documentation, as these classes can be quite confusing to people outside of content/ experts. BUG=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add some const to content/. I think this would be nice as documentation, as these classes can be quite confusing to people outside of content/ experts. BUG=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2727263004 Cr-Commit-Position: refs/heads/master@{#455768} Committed: https://chromium.googlesource.com/chromium/src/+/e8442a0169c58e21b309b0afd48b... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/e8442a0169c58e21b309b0afd48b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
