Loading: Use MockResource in ResourceFetcherTest.cpp

Loading: Use MockResource in ResourceFetcherTest.cpp

Now tests in ResourceFetcherTest.cpp use ImageResource for testing,
but ImageResource will be moved to core/loader/resource/, and core/fetch
should not have a dependency to the core/loader/.

This patch introduces MockResource and uses it in several tests in
ResourceFetcherTest.cpp.

BUG=655920
TEST=webkit_unit_tests --gtest_filter='ResourceFetcherTest.*'

Committed: https://crrev.com/ab9d0bcb41c77781cf482f4937686cc604b41112
Cr-Commit-Position: refs/heads/master@{#438456}

[Takashi Toyoshima, 2016-12-07 11:53:46]

Description was changed from

==========
Loading: Use MockResource in ResourceFetcherTest.cpp

Now tests in ResourceFetcherTest.cpp use ImageResource for testing,
but ImageResource will be moved to core/loader/resource/, and core/fetch
should not have a dependency to the core/loader/.

This patch introduces MockResource and use it in several tests in
ResourceFetcherTest.cpp.

BUG=655920
TEST=webkit_unit_tests --gtest_filter='ResourceFetcherTest.*'
==========

to

==========
Loading: Use MockResource in ResourceFetcherTest.cpp

Now tests in ResourceFetcherTest.cpp use ImageResource for testing,
but ImageResource will be moved to core/loader/resource/, and core/fetch
should not have a dependency to the core/loader/.

This patch introduces MockResource and use it in several tests in
ResourceFetcherTest.cpp.

BUG=655920
TEST=webkit_unit_tests --gtest_filter='ResourceFetcherTest.*'
==========

[Takashi Toyoshima, 2016-12-07 11:53:46]

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

[Takashi Toyoshima, 2016-12-07 11:54:31]

ptal

[yhirano, 2016-12-08 07:57:52]

> This patch introduces MockResource and use it in several tests in
ResourceFetcherTest.cpp.

s/use/uses/

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MockResource.cpp (right):

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MockResource.cpp:27: Resource::Type m_type;
You can use ResourceFactory::type().

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MockResource.cpp:37:
WebURLRequest::RequestContextUnspecified;
With the below NOTREACHED, can you define this variable after the switch
statement and initialize it to RequestContextImage?

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right):

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:65: const char
testResourceFilename[] = "white-1x1.png";
Please use constexpr.

[yhirano, 2016-12-09 02:19:00]

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

[commit-bot: I haz the power, 2016-12-09 02:19:30]

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, 2016-12-09 02:23:58]

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

[commit-bot: I haz the power, 2016-12-09 02:23:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Takashi Toyoshima, 2016-12-09 05:12:06]

Description was changed from

==========
Loading: Use MockResource in ResourceFetcherTest.cpp

Now tests in ResourceFetcherTest.cpp use ImageResource for testing,
but ImageResource will be moved to core/loader/resource/, and core/fetch
should not have a dependency to the core/loader/.

This patch introduces MockResource and use it in several tests in
ResourceFetcherTest.cpp.

BUG=655920
TEST=webkit_unit_tests --gtest_filter='ResourceFetcherTest.*'
==========

to

==========
Loading: Use MockResource in ResourceFetcherTest.cpp

Now tests in ResourceFetcherTest.cpp use ImageResource for testing,
but ImageResource will be moved to core/loader/resource/, and core/fetch
should not have a dependency to the core/loader/.

This patch introduces MockResource and uses it in several tests in
ResourceFetcherTest.cpp.

BUG=655920
TEST=webkit_unit_tests --gtest_filter='ResourceFetcherTest.*'
==========

[Takashi Toyoshima, 2016-12-09 05:31:54]

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MockResource.cpp (right):

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MockResource.cpp:27: Resource::Type m_type;
On 2016/12/08 07:57:52, yhirano wrote:
> You can use ResourceFactory::type().

Done.

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MockResource.cpp:37:
WebURLRequest::RequestContextUnspecified;
I slightly prefer current style because it help us to understand what should be
revised if we add a support for another type. But I'm fine with both. I'd update
this as you suggest.

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right):

https://codereview.chromium.org/2549093008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:65: const char
testResourceFilename[] = "white-1x1.png";
On 2016/12/08 07:57:52, yhirano wrote:
> Please use constexpr.

Done.

[yhirano, 2016-12-09 05:39:41]

lgtm

[hiroshige, 2016-12-11 19:11:13]

Am I understanding below of the intent of this CL correctly?
  MockResource is a 'generic' Resource that behaves 'normally', without
  'something complicated' specific to a certain subclass.
  MockResource replaces Resources that are expected nothing special in tests
  like the ImageResources replaced in this CL.
  MockResource's implementation should remain mostly empty,
  and MockResource should be a single 'generic' Resource.
  Specific behaviors should be tested in core/loader with actual Resource
  subclasses.
If so:
  taking Resource::Type as an argument in MockResourceFactory and
  MockResource constructor looks confusing, because it looks there can be
  Image-type 'generic' Resource and other-type 'generic' Resource.
  I think we should prevent such Resources because they looks subtle mixture
  of generic mock and real subclass.

Alternatively:
  MockResource is a mock class of each Resource subclass without real code
  dependencies.
  If we want to test ImageResource's DecodeError, we can implement
  mock of DecodeError path in MockImage without including ImageResource.h.
  In this sense, MockResource can behave like ImageResource (in this CL),
  or other kind of Resources e.g. FontResource/RawResource in the future.
  (I think it's better to write more specific mocks for this purpose though)

[Takashi Toyoshima, 2016-12-13 06:26:36]

Yep, you understand my intention correctly.

I discussed with yhirano@ offline. So, how about adding new Resource::Type for
Mock and using it for MockResource because we do not have a Resource::Type for
vanilla Resource instance. With this type, I could stop taking Resource::Type in
MockResourceFactory.

[Takashi Toyoshima, 2016-12-13 06:57:04]

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

[Takashi Toyoshima, 2016-12-13 06:57:27]

+tkent for BUILD.gn

[Takashi Toyoshima, 2016-12-13 06:57:32]

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

[commit-bot: I haz the power, 2016-12-13 06:57:47]

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

[hiroshige, 2016-12-13 08:47:46]

> adding new Resource::Type
lgtm, provided it is fine with toyoshim@ and yhirano@.

I am slightly afraid adding a new Resource::Type might cause additional corner
cases or tests might test non-production code paths, but the code is quite
clear: MockResource tests 'generic' code paths, i.e. not taking branch with
conditions like "if (type == Image)", "if (isImage())", etc.

https://codereview.chromium.org/2549093008/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MockResource.h (right):

https://codereview.chromium.org/2549093008/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MockResource.h:18: // of Resource sub-class
in a simple way. You should not expect something
nit: s/something/anything/

[commit-bot: I haz the power, 2016-12-13 08:55:17]

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

[commit-bot: I haz the power, 2016-12-13 08:55:18]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2016-12-13 12:23:45]

Thanks.
One additional pros of this approach is that this change teaches me which code
path depends on resource type :)

https://codereview.chromium.org/2549093008/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MockResource.h (right):

https://codereview.chromium.org/2549093008/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MockResource.h:18: // of Resource sub-class
in a simple way. You should not expect something
On 2016/12/13 08:47:46, hiroshige wrote:
> nit: s/something/anything/

Done.

[Takashi Toyoshima, 2016-12-13 12:27:49]

https://codereview.chromium.org/2549093008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2549093008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1560: case
Resource::Mock:
Note: I could not catch this at compile time because it has 'default:', but
tests fire at NOTREACHED(). I notice we don't handle some kinds of resource
types here, but I assume it's intentional. Just in case.

[Takashi Toyoshima, 2016-12-13 17:36:45]

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

[tkent, 2016-12-13 23:15:20]

lgtm

[Takashi Toyoshima, 2016-12-14 03:56:47]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-12-14 03:56:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/2549093008/#ps100001
(title: "review #18")

[commit-bot: I haz the power, 2016-12-14 03:57:09]

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, 2016-12-14 03:58:53]

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

[commit-bot: I haz the power, 2016-12-14 03:58:54]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Takashi Toyoshima, 2016-12-14 05:28:54]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-12-14 05:28:55]

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

[commit-bot: I haz the power, 2016-12-14 05:29:19]

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, 2016-12-14 07:39:26]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481693334612300,
"parent_rev": "64ca1231c298ec097e0a398382d5184ee734e7b1", "commit_rev":
"77da6759869d460e035b30862fca141522c95e6d"}

[commit-bot: I haz the power, 2016-12-14 07:39:53]

Description was changed from

==========
Loading: Use MockResource in ResourceFetcherTest.cpp

Now tests in ResourceFetcherTest.cpp use ImageResource for testing,
but ImageResource will be moved to core/loader/resource/, and core/fetch
should not have a dependency to the core/loader/.

This patch introduces MockResource and uses it in several tests in
ResourceFetcherTest.cpp.

BUG=655920
TEST=webkit_unit_tests --gtest_filter='ResourceFetcherTest.*'
==========

to

==========
Loading: Use MockResource in ResourceFetcherTest.cpp

Now tests in ResourceFetcherTest.cpp use ImageResource for testing,
but ImageResource will be moved to core/loader/resource/, and core/fetch
should not have a dependency to the core/loader/.

This patch introduces MockResource and uses it in several tests in
ResourceFetcherTest.cpp.

BUG=655920
TEST=webkit_unit_tests --gtest_filter='ResourceFetcherTest.*'

Review-Url: https://codereview.chromium.org/2549093008
==========

[commit-bot: I haz the power, 2016-12-14 07:39:54]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-14 07:41:44]

Description was changed from

==========
Loading: Use MockResource in ResourceFetcherTest.cpp

Now tests in ResourceFetcherTest.cpp use ImageResource for testing,
but ImageResource will be moved to core/loader/resource/, and core/fetch
should not have a dependency to the core/loader/.

This patch introduces MockResource and uses it in several tests in
ResourceFetcherTest.cpp.

BUG=655920
TEST=webkit_unit_tests --gtest_filter='ResourceFetcherTest.*'

Review-Url: https://codereview.chromium.org/2549093008
==========

to

==========
Loading: Use MockResource in ResourceFetcherTest.cpp

Now tests in ResourceFetcherTest.cpp use ImageResource for testing,
but ImageResource will be moved to core/loader/resource/, and core/fetch
should not have a dependency to the core/loader/.

This patch introduces MockResource and uses it in several tests in
ResourceFetcherTest.cpp.

BUG=655920
TEST=webkit_unit_tests --gtest_filter='ResourceFetcherTest.*'

Committed: https://crrev.com/ab9d0bcb41c77781cf482f4937686cc604b41112
Cr-Commit-Position: refs/heads/master@{#438456}
==========

[commit-bot: I haz the power, 2016-12-14 07:41:45]

Patchset 7 (id:??) landed as
https://crrev.com/ab9d0bcb41c77781cf482f4937686cc604b41112
Cr-Commit-Position: refs/heads/master@{#438456}