Don't reset URL for CAN_COMMIT_URL_BLOCKED renderer kill.

Don't reset URL for CAN_COMMIT_URL_BLOCKED renderer kill.

This is no longer necessary now that we have an early return.

BUG=622385
TEST=Non-blank URL in crash dumps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2758143002
Cr-Commit-Position: refs/heads/master@{#458509}
Committed: https://chromium.googlesource.com/chromium/src/+/cc3210aac06fd0f9437a66b6d9d784cb80b481d9

[Charlie Reis, 2017-03-19 05:13:16]

Description was changed from

==========
Reset URL after CAN_COMMIT_URL_BLOCKED renderer kill.

BUG=622385
TEST=Non-blank URL in crash dumps.
==========

to

==========
Reset URL after CAN_COMMIT_URL_BLOCKED renderer kill.

BUG=622385
TEST=Non-blank URL in crash dumps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-03-19 05:13:24]

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

[commit-bot: I haz the power, 2017-03-19 05:13:37]

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, 2017-03-19 06:06:38]

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

[commit-bot: I haz the power, 2017-03-19 06:06:39]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-03-20 18:06:21]

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

[Charlie Reis, 2017-03-20 18:06:21]

Just a minor change to avoid resetting the URL before the crash dump is
generated.

[nasko, 2017-03-20 21:37:14]

The move itself LGTM, but I did some digging and it might be safe to just remove
that line, which also would be LGTM :).

https://codereview.chromium.org/2758143002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2758143002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1227: validated_params.url
= GURL(url::kAboutBlankURL);
Doing some code archeology, it looks like we might actually not need that line.
We don't have it in the CanCommitOrigin check below, so we should be consistent.

We had to have it before, as there was no return statement in this if block,
which meant that the URL had to be rewritten to be safe. However as part of
https://codereview.chromium.org/1775543002, the return statement is added and
this seems to no longer be needed.

The ordering of dates back from https://chromiumcodereview.appspot.com/15017018,
so it looks like we didn't really get meaningful URLs from this kill all along.

[Charlie Reis, 2017-03-21 18:31:48]

Description was changed from

==========
Reset URL after CAN_COMMIT_URL_BLOCKED renderer kill.

BUG=622385
TEST=Non-blank URL in crash dumps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't reset URL for CAN_COMMIT_URL_BLOCKED renderer kill.

This is no longer necessary now that we have an early return.

BUG=622385
TEST=Non-blank URL in crash dumps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-03-21 18:32:11]

https://codereview.chromium.org/2758143002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2758143002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1227: validated_params.url
= GURL(url::kAboutBlankURL);
On 2017/03/20 21:37:14, nasko (slow) wrote:
> Doing some code archeology, it looks like we might actually not need that
line.
> We don't have it in the CanCommitOrigin check below, so we should be
consistent.
> 
> We had to have it before, as there was no return statement in this if block,
> which meant that the URL had to be rewritten to be safe. However as part of
> https://codereview.chromium.org/1775543002, the return statement is added and
> this seems to no longer be needed.
> 
> The ordering of dates back from
https://chromiumcodereview.appspot.com/15017018?_ga=1.13635511.1028966180.148...,
> so it looks like we didn't really get meaningful URLs from this kill all
along.

Good call!  Done.

[Charlie Reis, 2017-03-21 18:32:14]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2017-03-21 18:32:15]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2758143002/#ps20001
(title: "Remove line.")

[commit-bot: I haz the power, 2017-03-21 18:33:44]

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, 2017-03-21 19:29:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490121134741820,
"parent_rev": "cd7193637823047ec88226019bb1d987215a35db", "commit_rev":
"cc3210aac06fd0f9437a66b6d9d784cb80b481d9"}

[commit-bot: I haz the power, 2017-03-21 19:30:37]

Description was changed from

==========
Don't reset URL for CAN_COMMIT_URL_BLOCKED renderer kill.

This is no longer necessary now that we have an early return.

BUG=622385
TEST=Non-blank URL in crash dumps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't reset URL for CAN_COMMIT_URL_BLOCKED renderer kill.

This is no longer necessary now that we have an early return.

BUG=622385
TEST=Non-blank URL in crash dumps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2758143002
Cr-Commit-Position: refs/heads/master@{#458509}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc3210aac06fd0f9437a66b6d9d7...
==========

[commit-bot: I haz the power, 2017-03-21 19:30:42]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/cc3210aac06fd0f9437a66b6d9d7...