Generalize ScopedRegisteredURL as ScoedMockedURL*

Generalize ScopedRegisteredURL as ScopedMockedURL*

ImageResourceTest had ScopedRegisteredURL implementation, but this
is a useful scheme for every tests that use URLTestHelpers.

This patch creates ScopedMockedURL.{cpp|h} to have ScopedMockedURL
and its variants, and makes ImageResourceTest use one of them.

Eventually, we would like to obsolete URLTestHelpers.

BUG=655920
Review-Url: https://codereview.chromium.org/2680723002
Cr-Commit-Position: refs/heads/master@{#448974}
Committed: https://chromium.googlesource.com/chromium/src/+/9ad7d8b5293d72bbf08f72d65bb0e9d343437bad

[Takashi Toyoshima, 2017-02-07 08:01:47]

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

[commit-bot: I haz the power, 2017-02-07 08:01:57]

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-02-07 08:02:00]

Description was changed from

==========
Generalize ScopedRegisteredURL and have it in URLTestHelpers

ImageResourceTest had ScopedRegisteredURL implementation, but this
is a useful scheme for every tests that use URLTestHelpers.

This patch moves ScopedRegisteredURL to URLTestHelpers as
ScopedMockedURL, and change URLTestHelpers::register* functions
to return WebURL so that it can be used with ScopedMockedURL like

  ScopedMockedURL scopedMockedURL(
      URLTestHelpers::registerMockedURLLoad(...));

BUG=655920
==========

to

==========
Generalize ScopedRegisteredURL and have it in URLTestHelpers

ImageResourceTest had ScopedRegisteredURL implementation, but this
is a useful scheme for every tests that use URLTestHelpers.

This patch moves ScopedRegisteredURL to URLTestHelpers as
ScopedMockedURL, and change URLTestHelpers::register* functions
to return WebURL so that it can be used with ScopedMockedURL like

  ScopedMockedURL scopedMockedURL(
      URLTestHelpers::registerMockedURLLoad(...));

BUG=655920
==========

[Takashi Toyoshima, 2017-02-07 08:02:00]

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

[Takashi Toyoshima, 2017-02-07 08:06:46]

Kinuko-san.

What do you think about this generalizing approach?
ScopedMockedURL could have multiple constructors that correspond with
registerMockedURL... functions, but current set of register functions look a bit
ad-hoc thing. So, probably just taking WebURL, and use with existing registers
function will be one reasonable way to generalize it.

[commit-bot: I haz the power, 2017-02-07 09:55:14]

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

[commit-bot: I haz the power, 2017-02-07 09:55:14]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-07 11:17:38]

https://codereview.chromium.org/2680723002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/testing/URLTestHelpers.h (right):

https://codereview.chromium.org/2680723002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/testing/URLTestHelpers.h:74: WebURL
registerMockedErrorURLLoad(const WebURL& fullURL);
To me making these methods return the given fullURL doesn't feel very
intuitive... could we make the scoped class work without doing so?

[Takashi Toyoshima, 2017-02-08 05:56:41]

Description was changed from

==========
Generalize ScopedRegisteredURL and have it in URLTestHelpers

ImageResourceTest had ScopedRegisteredURL implementation, but this
is a useful scheme for every tests that use URLTestHelpers.

This patch moves ScopedRegisteredURL to URLTestHelpers as
ScopedMockedURL, and change URLTestHelpers::register* functions
to return WebURL so that it can be used with ScopedMockedURL like

  ScopedMockedURL scopedMockedURL(
      URLTestHelpers::registerMockedURLLoad(...));

BUG=655920
==========

to

==========
Generalize ScopedRegisteredURL as ScopedMockedURL*

ImageResourceTest had ScopedRegisteredURL implementation, but this
is a useful scheme for every tests that use URLTestHelpers.

This patch creates ScopedMockedURL.{cpp|h} to have ScopedMockedURL
and its variants, and makes ImageResourceTest use one of them.

Eventually, we would like to obsolete URLTestHelpers.

BUG=655920
==========

[Takashi Toyoshima, 2017-02-08 06:42:09]

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

[commit-bot: I haz the power, 2017-02-08 06:42:39]

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-02-08 06:47:20]

how about this second plan?

https://codereview.chromium.org/2680723002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/testing/URLTestHelpers.h (right):

https://codereview.chromium.org/2680723002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/testing/URLTestHelpers.h:74: WebURL
registerMockedErrorURLLoad(const WebURL& fullURL);
On 2017/02/07 11:17:38, kinuko wrote:
> To me making these methods return the given fullURL doesn't feel very
> intuitive... could we make the scoped class work without doing so?

Done.

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/ScopedMockedURL.h (right):

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/ScopedMockedURL.h:23: explicit
ScopedMockedURL(const WebURL&);
We could have multiple constructor instead of having subclasses, but argument
sets are similar, and may be easy to use unexpected constructor, .e.g.
-LoadFromBase takes 3 or 4 WebString, -Load takes 1 WebURL + 1 or 2 WebString,
and three string literal arguments could be vague expression here.

[kinuko, 2017-02-08 08:21:48]

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/ScopedMockedURL.h (right):

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/ScopedMockedURL.h:17: // unregisters
it on destruction. This allows for a test to use constructs like
nit: unregisters -> unregister

allows for a test to use constructors like ASSERT_TRUE() ->
allows tests to use the constructors, e.g. for ASSERT_TRUE(), without...

?

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/ScopedMockedURL.h:18: //
ASSERT_TRUE() without needing to worry abount unregistering the mocked URL
abount -> about

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/ScopedMockedURL.h:61: };
Could we avoid defining unused classes yet?  We can add these later when we need
them.

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/URLTestHelpers.h (right):

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/URLTestHelpers.h:48: // possible.
I think we could say

"Avoid directly using these methods, instead please use ScopedMockedURL (or
define a variant of ScopedMockedURL classes if necessary) in new code."

and remove unused code from this CL for now.

[commit-bot: I haz the power, 2017-02-08 08:33:02]

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

[commit-bot: I haz the power, 2017-02-08 08:33:02]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2017-02-08 10:33:14]

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/ScopedMockedURL.h (right):

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/ScopedMockedURL.h:17: // unregisters
it on destruction. This allows for a test to use constructs like
As discussed offline, this second sentence looks trivial for ScopedX users.
(and original intention is a little unclear, maybe constructs was typo of
contracts?)
Let's remove.

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/ScopedMockedURL.h:18: //
ASSERT_TRUE() without needing to worry abount unregistering the mocked URL
On 2017/02/08 08:21:48, kinuko wrote:
> abount -> about

Done.

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/ScopedMockedURL.h:61: };
On 2017/02/08 08:21:48, kinuko wrote:
> Could we avoid defining unused classes yet?  We can add these later when we
need
> them.

Done.

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/URLTestHelpers.h (right):

https://codereview.chromium.org/2680723002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/URLTestHelpers.h:48: // possible.
On 2017/02/08 08:21:48, kinuko wrote:
> I think we could say
> 
> "Avoid directly using these methods, instead please use ScopedMockedURL (or
> define a variant of ScopedMockedURL classes if necessary) in new code."
> 
> and remove unused code from this CL for now.

Done.

[Takashi Toyoshima, 2017-02-08 10:38:46]

Patchset #6 (id:100001) has been deleted

[kinuko, 2017-02-08 10:38:50]

lgtm

[kinuko, 2017-02-08 10:38:52]

lgtm

[Takashi Toyoshima, 2017-02-08 11:11:26]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2017-02-08 11:11:42]

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-02-08 13:22:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486552286388000,
"parent_rev": "08717bb5a034aa197be65cf035daddfe1b892404", "commit_rev":
"9ad7d8b5293d72bbf08f72d65bb0e9d343437bad"}

[commit-bot: I haz the power, 2017-02-08 13:22:39]

Description was changed from

==========
Generalize ScopedRegisteredURL as ScopedMockedURL*

ImageResourceTest had ScopedRegisteredURL implementation, but this
is a useful scheme for every tests that use URLTestHelpers.

This patch creates ScopedMockedURL.{cpp|h} to have ScopedMockedURL
and its variants, and makes ImageResourceTest use one of them.

Eventually, we would like to obsolete URLTestHelpers.

BUG=655920
==========

to

==========
Generalize ScopedRegisteredURL as ScopedMockedURL*

ImageResourceTest had ScopedRegisteredURL implementation, but this
is a useful scheme for every tests that use URLTestHelpers.

This patch creates ScopedMockedURL.{cpp|h} to have ScopedMockedURL
and its variants, and makes ImageResourceTest use one of them.

Eventually, we would like to obsolete URLTestHelpers.

BUG=655920

Review-Url: https://codereview.chromium.org/2680723002
Cr-Commit-Position: refs/heads/master@{#448974}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ad7d8b5293d72bbf08f72d65bb0...
==========

[commit-bot: I haz the power, 2017-02-08 13:22:40]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9ad7d8b5293d72bbf08f72d65bb0...