Use MockResource as much as possibler in MemoryCacheCorrectnessTest

Use MockResource as much as possibler in MemoryCacheCorrectnessTest

This patch modifies MemoryCacheCorrectnessTest to use MockResource
instead of RawResource as much as possible, and to use MockResource*
or RawResource* rather than Resource* to hold references so that
EXPECT_EQ can force to use a right resource type for testing.

Now we have two unit tests that require RawResource to use
fetchSynchronously() and willFollowRedirect().

Also, since we do not have any other tests that use
MemorycacheCorrectnessTestHelper any more, this patch merges the file
to MemoryCacheCorrectnessTest.

TEST=webkit_unit_tests --gtest_filter=MemoryCacheCorrectnessTest.\*
BUG=655920
Review-Url: https://codereview.chromium.org/2637063002
Cr-Commit-Position: refs/heads/master@{#444664}
Committed: https://chromium.googlesource.com/chromium/src/+/7b6c57a4011db4ae08c5b70c9029633796802582

[Takashi Toyoshima, 2017-01-17 11:13:05]

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

[commit-bot: I haz the power, 2017-01-17 11:13:13]

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

[Takashi Toyoshima, 2017-01-17 11:13:23]

Description was changed from

==========
Merge MemoryCacheCorrectnessTestHelper to MemoryCacheCorrectnessTest

Since we do not have any other tests that use
MemorycacheCorrectnessTestHelper any more, let's merge it to
MemoryCacheCorrectnessTest.

Also this patch fixes existing TODO to return Resource sub-classes,
RawResource and MockResource respectively so that we are forced to use
a right Resource type.

Now we have two unit tests that require RawResource to use
fetchSynchronously() and willFollowRedirect().

TEST=webkit_unit_tests --gtest_filter=MemoryCacheCorrectnessTest.\*
BUG=655920
==========

to

==========
Merge MemoryCacheCorrectnessTestHelper to MemoryCacheCorrectnessTest

Since we do not have any other tests that use
MemorycacheCorrectnessTestHelper any more, let's merge it to
MemoryCacheCorrectnessTest.

Also this patch fixes existing TODO to return Resource sub-classes,
RawResource and MockResource respectively so that we are forced to use
a right Resource type.

Now we have two unit tests that require RawResource to use
fetchSynchronously() and willFollowRedirect().

TEST=webkit_unit_tests --gtest_filter=MemoryCacheCorrectnessTest.\*
BUG=655920
==========

[Takashi Toyoshima, 2017-01-17 11:13:23]

toyoshim@chromium.org changed reviewers:
+ hiroshige@chromium.org, yhirano@chromium.org

[Takashi Toyoshima, 2017-01-17 11:14:44]

yhirano: PTAL
hiroshige: FYI since I left this TODO in a CL that you reviewed

[commit-bot: I haz the power, 2017-01-17 12:58:32]

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

[commit-bot: I haz the power, 2017-01-17 12:58:33]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-17 19:28:43]

lgtm with comments.

CL title/description: optional: for me the main point of this CL looks replacing
RawResource with MockResource (which makes tests to execute different code
paths) rather than merging MemoryCacheCorrectnessTestHelper (which is pure
refactoring that doesn't affect behavior).

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/RawResource.cpp (right):

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/RawResource.cpp:283: bool
RawResource::willFollowRedirect(const ResourceRequest& newRequest,
What is the reason for moving this funtion definition?

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/RawResource.h (right):

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/RawResource.h:66: bool
willFollowRedirect(const ResourceRequest&,
Could you add a comment that states this is exposed only for tests?
(Probably also a TODO to make this private again if you'd like to remove
dependencies from tests to willFollowRedirect)

[yhirano, 2017-01-18 05:58:37]

lgtm

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/MemoryCacheCorrectnessTest.cpp
(right):

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/MemoryCacheCorrectnessTest.cpp:48: const
char kResourceURL[] = "http://resource.com/";
constexpr
ditto below

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/RawResource.h (right):

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/RawResource.h:66: bool
willFollowRedirect(const ResourceRequest&,
On 2017/01/17 19:28:42, hiroshige wrote:
> Could you add a comment that states this is exposed only for tests?
> (Probably also a TODO to make this private again if you'd like to remove
> dependencies from tests to willFollowRedirect)

Hmm, I don't see much value in hidding functions that are already exposed in the
parent class.

[Takashi Toyoshima, 2017-01-19 03:55:38]

Description was changed from

==========
Merge MemoryCacheCorrectnessTestHelper to MemoryCacheCorrectnessTest

Since we do not have any other tests that use
MemorycacheCorrectnessTestHelper any more, let's merge it to
MemoryCacheCorrectnessTest.

Also this patch fixes existing TODO to return Resource sub-classes,
RawResource and MockResource respectively so that we are forced to use
a right Resource type.

Now we have two unit tests that require RawResource to use
fetchSynchronously() and willFollowRedirect().

TEST=webkit_unit_tests --gtest_filter=MemoryCacheCorrectnessTest.\*
BUG=655920
==========

to

==========
Use MockResource as much as possibler in MemoryCacheCorrectnessTest

This patch modifies MemoryCacheCorrectnessTest to use MockResource
instead of RawResource as much as possible, and to use MockResource*
or RawResource* rather than Resource* to hold references so that
EXPECT_EQ can force to use a right resource type for testing.

Now we have two unit tests that require RawResource to use
fetchSynchronously() and willFollowRedirect().

Also, since we do not have any other tests that use
MemorycacheCorrectnessTestHelper any more, this patch merges the file
to MemoryCacheCorrectnessTest.

TEST=webkit_unit_tests --gtest_filter=MemoryCacheCorrectnessTest.\*
BUG=655920
==========

[Takashi Toyoshima, 2017-01-19 04:08:20]

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/MemoryCacheCorrectnessTest.cpp
(right):

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/MemoryCacheCorrectnessTest.cpp:48: const
char kResourceURL[] = "http://resource.com/";
On 2017/01/18 05:58:37, yhirano wrote:
> constexpr
> ditto below

Done.

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/RawResource.cpp (right):

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/RawResource.cpp:283: bool
RawResource::willFollowRedirect(const ResourceRequest& newRequest,
I just prefer to have definitions in the same order with the header
declarations. That's why I moved this to the place after the canReuse().

But... it looks this file contains definitions in completely different order and
moving this method here does not make much sense. I'd revert this change in the
next patch set.

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/RawResource.h (right):

https://codereview.chromium.org/2637063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/RawResource.h:66: bool
willFollowRedirect(const ResourceRequest&,
Agreed with yhirano here.
I'd submit this patch as is for this.
Please let me follow up this topic in the next patch
(https://codereview.chromium.org/2631383003/) if hiroshige@ still have some
concerns or opinions about this.

[Takashi Toyoshima, 2017-01-19 04:14:00]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-01-19 04:14:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2637063002/#ps40001
(title: "[rebase]")

[Takashi Toyoshima, 2017-01-19 04:14:26]

The CQ bit was unchecked by toyoshim@chromium.org

[commit-bot: I haz the power, 2017-01-19 04:14:26]

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

[Takashi Toyoshima, 2017-01-19 04:14:35]

The CQ bit was unchecked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-01-19 04:14:40]

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

[Takashi Toyoshima, 2017-01-19 04:14:57]

toyoshim@chromium.org changed reviewers:
+ haraken@chromium.org

[commit-bot: I haz the power, 2017-01-19 04:15:04]

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

[Takashi Toyoshima, 2017-01-19 04:15:16]

+haraken for BUILD.gn

[kinuko, 2017-01-19 04:18:37]

drive-by lgtm

[haraken, 2017-01-19 04:22:02]

LGTM

[kinuko, 2017-01-19 04:23:24]

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

[kinuko, 2017-01-19 04:23:24]

https://codereview.chromium.org/2637063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/RawResource.h (right):

https://codereview.chromium.org/2637063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResource.h:65: 
nit: no empty line needed between overrides from the same base class

[Takashi Toyoshima, 2017-01-19 04:48:34]

thanks

https://codereview.chromium.org/2637063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/RawResource.h (right):

https://codereview.chromium.org/2637063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResource.h:65: 
On 2017/01/19 04:23:24, kinuko wrote:
> nit: no empty line needed between overrides from the same base class

Done.

[Takashi Toyoshima, 2017-01-19 04:48:42]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-01-19 04:48:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, haraken@chromium.org, yhirano@chromium.org,
kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2637063002/#ps60001
(title: "override related style nits")

[commit-bot: I haz the power, 2017-01-19 04:49:08]

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-01-19 06:21:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484801322231910,
"parent_rev": "fe5b80deb66d94a110ef347739f3f7e8d7d164c5", "commit_rev":
"7b6c57a4011db4ae08c5b70c9029633796802582"}

[commit-bot: I haz the power, 2017-01-19 06:21:51]

Description was changed from

==========
Use MockResource as much as possibler in MemoryCacheCorrectnessTest

This patch modifies MemoryCacheCorrectnessTest to use MockResource
instead of RawResource as much as possible, and to use MockResource*
or RawResource* rather than Resource* to hold references so that
EXPECT_EQ can force to use a right resource type for testing.

Now we have two unit tests that require RawResource to use
fetchSynchronously() and willFollowRedirect().

Also, since we do not have any other tests that use
MemorycacheCorrectnessTestHelper any more, this patch merges the file
to MemoryCacheCorrectnessTest.

TEST=webkit_unit_tests --gtest_filter=MemoryCacheCorrectnessTest.\*
BUG=655920
==========

to

==========
Use MockResource as much as possibler in MemoryCacheCorrectnessTest

This patch modifies MemoryCacheCorrectnessTest to use MockResource
instead of RawResource as much as possible, and to use MockResource*
or RawResource* rather than Resource* to hold references so that
EXPECT_EQ can force to use a right resource type for testing.

Now we have two unit tests that require RawResource to use
fetchSynchronously() and willFollowRedirect().

Also, since we do not have any other tests that use
MemorycacheCorrectnessTestHelper any more, this patch merges the file
to MemoryCacheCorrectnessTest.

TEST=webkit_unit_tests --gtest_filter=MemoryCacheCorrectnessTest.\*
BUG=655920

Review-Url: https://codereview.chromium.org/2637063002
Cr-Commit-Position: refs/heads/master@{#444664}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b6c57a4011db4ae08c5b70c9029...
==========

[commit-bot: I haz the power, 2017-01-19 06:21:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7b6c57a4011db4ae08c5b70c9029...