Removed local RefPtr objects created from PassRefPtr arguments.

Removed local RefPtr objects created from PassRefPtr arguments.

Removed local RefPtr objects created from PassRefPtr arguments and
converted the PassRefPtr arguments to RefPtr.

This patch deals with one specific corner case of copying PassRefPtr
objects to RefPtr objects, where PassRefPtr arguments are copied into
local RefPtr objects to be used throughout the function.

All instances of PassRefPtr objects being copied into RefPtr objects
need to be wrapped in std::move to avoid introducing ref churn in
future changes when all PassRefPtr objects will be replaced with RefPtr
objects. In these cases of creating a local RefPtr just to copy the
PassRefPtr argument into, a std::move wrap doesn't make sense as it
would result in a final state where a RefPtr argument is moved into a
local RefPtr for no reason. So to handle these cases I have converted
the PassRefPtr arguments to RefPtr in advance, and removed the local
RefPtr argumen, instead using the passed RefPtr throughout the
methods. In most cases the RefPtr argument name has been changed to
match the previousf name of the local RefPtr.

The syntax of the local RefPtr variables have also been changed to
use std::move wraps instead of calling RefPtr::Release in cases
where the RefPtr is no longer needed locally after passing.
Note that std::move wraps have been introduced in some cases where
RefPtr::Release() was not being called but should have been.

BUG=494719
Review-Url: https://codereview.chromium.org/2807073002
Cr-Commit-Position: refs/heads/master@{#463565}
Committed: https://chromium.googlesource.com/chromium/src/+/ad3dfab7e0e948209a3a68e1900a038c86182502

[Bugs Nash, 2017-04-10 05:46:43]

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

[commit-bot: I haz the power, 2017-04-10 05:46:55]

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

[Bugs Nash, 2017-04-10 05:49:07]

bugsnash@chromium.org changed reviewers:
+ haraken@chromium.org, yutak@chromium.org

[haraken, 2017-04-10 06:49:22]

LGTM

[Yuta Kitamura, 2017-04-10 07:00:11]

https://codereview.chromium.org/2807073002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp
(left):

https://codereview.chromium.org/2807073002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp:487:
RefPtr<SecurityOrigin> new_security_origin = current_security_origin;
I think the removal of new_security_origin causes a behavioral change,
specifically:
  * l.527 sets new_security_origin, without changing current_security_origin,
  * l.531 tests a condition based on current_security_origin,
  * l.533 uses new_security_origin.

I think those two variables are intentionally separated, so you probably
should keep this separation.

[commit-bot: I haz the power, 2017-04-10 07:08:40]

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

[commit-bot: I haz the power, 2017-04-10 07:08:41]

Dry run: This issue passed the CQ dry run.

[Bugs Nash, 2017-04-11 01:47:51]

https://codereview.chromium.org/2807073002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp
(left):

https://codereview.chromium.org/2807073002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp:487:
RefPtr<SecurityOrigin> new_security_origin = current_security_origin;
On 2017/04/10 at 07:00:11, Yuta Kitamura wrote:
> I think the removal of new_security_origin causes a behavioral change,

I'm not removing new_security_origin, I'm setting it to the passed RefPtr
instead of the local one (they are the same)

> specifically:
>   * l.527 sets new_security_origin, without changing current_security_origin,
>   * l.531 tests a condition based on current_security_origin,
>   * l.533 uses new_security_origin.
> 
> I think those two variables are intentionally separated, so you probably
> should keep this separation.

I don't see how this is a behavior change. Previously security_origin (argument)
was *only* used to set current_security_origin, then never used again. So having
both is redundant right? If I had of changed the argument to be called
current_security_origin and deleted the local RefPtr called
current_security_origin it should have no impact on behavior. This patch does
the same thing, except it also renames current_security_origin to
security_origin.

[Yuta Kitamura, 2017-04-11 05:49:39]

LGTM w/ nit.

https://codereview.chromium.org/2807073002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp
(left):

https://codereview.chromium.org/2807073002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp:487:
RefPtr<SecurityOrigin> new_security_origin = current_security_origin;
Ah I see, I now see you didn't change the behavior.

However, I'd propose renaming |security_origin| (in the function arguments)
to |current_security_origin|, since current_ and new_ are paired here,
and the code handles a tricky transition (current_ -> new_) situation.

[Bugs Nash, 2017-04-11 06:05:36]

On 2017/04/11 at 05:49:39, yutak wrote:
> LGTM w/ nit.
> 
>
https://codereview.chromium.org/2807073002/diff/1/third_party/WebKit/Source/p...
> File
third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp
(left):
> 
>
https://codereview.chromium.org/2807073002/diff/1/third_party/WebKit/Source/p...
>
third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp:487:
RefPtr<SecurityOrigin> new_security_origin = current_security_origin;
> Ah I see, I now see you didn't change the behavior.
> 
> However, I'd propose renaming |security_origin| (in the function arguments)
> to |current_security_origin|, since current_ and new_ are paired here,
> and the code handles a tricky transition (current_ -> new_) situation.

Done :)

[Bugs Nash, 2017-04-11 06:06:06]

The CQ bit was checked by bugsnash@chromium.org

[Bugs Nash, 2017-04-11 06:06:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/2807073002/#ps20001
(title: "addressed review")

[commit-bot: I haz the power, 2017-04-11 06:06:22]

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-04-11 07:30:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491890766956410,
"parent_rev": "12c17683df6cc8bbb32bcc6ab375c8d3901ee258", "commit_rev":
"ad3dfab7e0e948209a3a68e1900a038c86182502"}

[commit-bot: I haz the power, 2017-04-11 07:32:26]

Description was changed from

==========
Removed local RefPtr objects created from PassRefPtr arguments.

Removed local RefPtr objects created from PassRefPtr arguments and
converted the PassRefPtr arguments to RefPtr.

This patch deals with one specific corner case of copying PassRefPtr
objects to RefPtr objects, where PassRefPtr arguments are copied into
local RefPtr objects to be used throughout the function.

All instances of PassRefPtr objects being copied into RefPtr objects
need to be wrapped in std::move to avoid introducing ref churn in
future changes when all PassRefPtr objects will be replaced with RefPtr
objects. In these cases of creating a local RefPtr just to copy the
PassRefPtr argument into, a std::move wrap doesn't make sense as it
would result in a final state where a RefPtr argument is moved into a
local RefPtr for no reason. So to handle these cases I have converted
the PassRefPtr arguments to RefPtr in advance, and removed the local
RefPtr argumen, instead using the passed RefPtr throughout the
methods. In most cases the RefPtr argument name has been changed to
match the previousf name of the local RefPtr.

The syntax of the local RefPtr variables have also been changed to
use std::move wraps instead of calling RefPtr::Release in cases
where the RefPtr is no longer needed locally after passing.
Note that std::move wraps have been introduced in some cases where
RefPtr::Release() was not being called but should have been.

BUG=494719
==========

to

==========
Removed local RefPtr objects created from PassRefPtr arguments.

Removed local RefPtr objects created from PassRefPtr arguments and
converted the PassRefPtr arguments to RefPtr.

This patch deals with one specific corner case of copying PassRefPtr
objects to RefPtr objects, where PassRefPtr arguments are copied into
local RefPtr objects to be used throughout the function.

All instances of PassRefPtr objects being copied into RefPtr objects
need to be wrapped in std::move to avoid introducing ref churn in
future changes when all PassRefPtr objects will be replaced with RefPtr
objects. In these cases of creating a local RefPtr just to copy the
PassRefPtr argument into, a std::move wrap doesn't make sense as it
would result in a final state where a RefPtr argument is moved into a
local RefPtr for no reason. So to handle these cases I have converted
the PassRefPtr arguments to RefPtr in advance, and removed the local
RefPtr argumen, instead using the passed RefPtr throughout the
methods. In most cases the RefPtr argument name has been changed to
match the previousf name of the local RefPtr.

The syntax of the local RefPtr variables have also been changed to
use std::move wraps instead of calling RefPtr::Release in cases
where the RefPtr is no longer needed locally after passing.
Note that std::move wraps have been introduced in some cases where
RefPtr::Release() was not being called but should have been.

BUG=494719

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

[commit-bot: I haz the power, 2017-04-11 07:32:28]

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