ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver.

ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver.

To implement respondWith() in PaymentRequestEvent, it's better to factor out
the FetchEvent related logics from RespondWithObserver and then make it as
common class to share common logic between FetchEvent and PaymentRequestEvent.

This CL doesn't change any existing behavior.

See the other CLs in this series:
  [1/3] This patch
  [2/3] https://codereview.chromium.org/2705293010/ (blink side)
  [3/3] https://codereview.chromium.org/2718013004/ (content side and test)

BUG=661608
Review-Url: https://codereview.chromium.org/2715663002
Cr-Commit-Position: refs/heads/master@{#456760}
Committed: https://chromium.googlesource.com/chromium/src/+/87bcdeadc1c7bdeb590472d36fc1458e9641274d

[zino, 2017-02-23 08:39:35]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 08:39:51]

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-02-23 10:12:25]

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

[commit-bot: I haz the power, 2017-02-23 10:12:26]

Dry run: This issue passed the CQ dry run.

[zino, 2017-03-07 15:26:24]

Description was changed from

==========
ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver.

BUG=
==========

to

==========
ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver.

To implement respondWith() in PaymentRequestEvent, it's better to factor out
the FetchEvent related logics from RespondWithObserver and then make it as
common class to share common logic between FetchEvent and PaymentRequestEvent.

This CL doesn't change any existing behavior.

See the other CLs in this series:
  [1/3] This patch
  [2/3] https://codereview.chromium.org/2705293010/ (blink side)
  [3/3] https://codereview.chromium.org/2718013004/ (content side and test)

BUG=661608
==========

[zino, 2017-03-07 15:29:26]

jinho.bang@samsung.com changed reviewers:
+ nhiroki@chromium.org, rouslan@chromium.org, tommyt@opera.com

[zino, 2017-03-07 15:29:48]

jinho.bang@samsung.com changed reviewers:
+ haraken@chromium.org
- tommyt@opera.com

[zino, 2017-03-07 15:51:39]

PTAL

[zino, 2017-03-07 15:56:48]

jinho.bang@samsung.com changed reviewers:
+ tommyt@opera.com

[zino, 2017-03-07 15:57:05]

jinho.bang@samsung.com changed reviewers:
- tommyt@opera.com

[shimazu, 2017-03-08 01:39:02]

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

[shimazu, 2017-03-08 01:39:03]

drive-by comments:

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(left):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:233:
ServiceWorkerGlobalScopeClient::from(getExecutionContext())
Could you add a virtual method to notify no respondWith was called and call the
method here instead of just overriding didDispatchEvent in FetchEventDispatcher?

It could be simpler for implementers to understand what they should implement. 
I think the following three are the methods to return the result of respondWith:
- responseWasFulfilled
- responseWasRejected
- noRespondWithWasCalled (new one called here)

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:23: //
This class observes the service worker's handling of events which have
I feel it's better to rewrite this comment to describe how to use this class.
For example;

"This is a base class to implement respondWith. The respondWith has the three
types of results: fulfilled, rejected and not called. Derived classes for each
event should implement the procedure of the three behaviors by overriding
responseWasFulfilled, responseWasRejected and ...."

[nhiroki, 2017-03-08 01:49:04]

Looks good overall. Added some comments about layering.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:144:
FetchRespondWithObserver::~FetchRespondWithObserver() {}
= default;

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:175:
m_observer.clear();
Is it possible to move line 174-175 into RespondWithObserver::didDispatchEvent
and call it here? (see my comment in RespondWithObserver::didDispatchEvent)

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:85:
void RespondWithObserver::didDispatchEvent(DispatchEventResult dispatchResult) {
How about removing line 86-95 like this?

void RespondWithObserver::didDispatchEvent(DispatchEventResult dispatchResult) {
  m_state = Done;
  m_observer.clear();
}

...then, make FetchRespondWithObserver::didDispatchEvent() call this:

void FetchRespondWithObserver::didDispatchEvent(DispatchEventResult
dispatchResult result) {
  // ...
  RespondWithObserver::didDiapatchEvent(result);
}

This encapsulates state management (m_state/m_observer) in RespondWithObserver
class.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:167:
RespondWithObserver* respondWithObserver = FetchRespondWithObserver::create(
Can you replace RespondWithObserver* with FetchRespondWithObserver*?

It'd be better to use the concrete class name instead of the base class name as
much as possible in terms of readability and compilability. If this change
affects wide areas, a separate CL is ok.

[falken, 2017-03-09 04:58:59]

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

[falken, 2017-03-09 04:59:00]

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(left):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:233:
ServiceWorkerGlobalScopeClient::from(getExecutionContext())
On 2017/03/08 01:39:03, shimazu (slow) wrote:
> Could you add a virtual method to notify no respondWith was called and call
the
> method here instead of just overriding didDispatchEvent in
FetchEventDispatcher?
> 
> It could be simpler for implementers to understand what they should implement.

> I think the following three are the methods to return the result of
respondWith:
> - responseWasFulfilled
> - responseWasRejected
> - noRespondWithWasCalled (new one called here)

I like something like this. See also my comment below.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:120:
m_observer.clear();
It seems unfortunate that the subclass has to remember to call this method. It's
also weird that responseWasRejected and responseWasFulfilled have the same body.
What if there were pure virtual functions onResponseRejected(),
onResponseFulfilled(), onNoResponse() that the subclasses must implemented, that
this base class calls, and then itself does lines 118-120.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:23: //
This class observes the service worker's handling of events which have
On 2017/03/08 01:39:03, shimazu (slow) wrote:
> I feel it's better to rewrite this comment to describe how to use this class.
> For example;
> 
> "This is a base class to implement respondWith. The respondWith has the three
> types of results: fulfilled, rejected and not called. Derived classes for each
> event should implement the procedure of the three behaviors by overriding
> responseWasFulfilled, responseWasRejected and ...."

+1. Is this base class ever supposed to be instantiated? If not can we make it
an abstract class?

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:35:
WaitUntilObserver*);
Is this function used?

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:42: //
Observes the promise and delays calling didHandle*Event() until the given
This comment may be confusing since this class doesn't have a didHandle*Event
function.

[zino, 2017-03-10 14:45:48]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 14:46:02]

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-10 16:47:52]

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

[commit-bot: I haz the power, 2017-03-10 16:47:53]

Dry run: This issue passed the CQ dry run.

[zino, 2017-03-10 17:57:18]

Thank you for input and reviews!

I addressed your comments. PTAL.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:144:
FetchRespondWithObserver::~FetchRespondWithObserver() {}
On 2017/03/08 01:49:04, nhiroki (slow) wrote:
> = default;

Done.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:175:
m_observer.clear();
On 2017/03/08 01:49:04, nhiroki (slow) wrote:
> Is it possible to move line 174-175 into RespondWithObserver::didDispatchEvent
> and call it here? (see my comment in RespondWithObserver::didDispatchEvent)

Anyway done.
The didDispatchEvent() is no longer virtual function.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(left):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:233:
ServiceWorkerGlobalScopeClient::from(getExecutionContext())
On 2017/03/09 04:59:00, falken wrote:
> On 2017/03/08 01:39:03, shimazu (slow) wrote:
> > Could you add a virtual method to notify no respondWith was called and call
> the
> > method here instead of just overriding didDispatchEvent in
> FetchEventDispatcher?
> > 
> > It could be simpler for implementers to understand what they should
implement.
> 
> > I think the following three are the methods to return the result of
> respondWith:
> > - responseWasFulfilled
> > - responseWasRejected
> > - noRespondWithWasCalled (new one called here)
> 
> I like something like this. See also my comment below.

Done.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:85:
void RespondWithObserver::didDispatchEvent(DispatchEventResult dispatchResult) {
On 2017/03/08 01:49:04, nhiroki (slow) wrote:
> How about removing line 86-95 like this?
> 
> void RespondWithObserver::didDispatchEvent(DispatchEventResult dispatchResult)
{
>   m_state = Done;
>   m_observer.clear();
> }
> 
> ...then, make FetchRespondWithObserver::didDispatchEvent() call this:
> 
> void FetchRespondWithObserver::didDispatchEvent(DispatchEventResult
> dispatchResult result) {
>   // ...
>   RespondWithObserver::didDiapatchEvent(result);
> }
> 
> This encapsulates state management (m_state/m_observer) in RespondWithObserver
> class.

Anyway done.
The didDispatchEvent() was removed in this class :)

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:120:
m_observer.clear();
On 2017/03/09 04:59:00, falken wrote:
> It seems unfortunate that the subclass has to remember to call this method.
It's
> also weird that responseWasRejected and responseWasFulfilled have the same
body.
> What if there were pure virtual functions onResponseRejected(),
> onResponseFulfilled(), onNoResponse() that the subclasses must implemented,
that
> this base class calls, and then itself does lines 118-120.

Done.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:23: //
This class observes the service worker's handling of events which have
On 2017/03/09 04:59:00, falken wrote:
> On 2017/03/08 01:39:03, shimazu (slow) wrote:
> > I feel it's better to rewrite this comment to describe how to use this
class.
> > For example;
> > 
> > "This is a base class to implement respondWith. The respondWith has the
three
> > types of results: fulfilled, rejected and not called. Derived classes for
each
> > event should implement the procedure of the three behaviors by overriding
> > responseWasFulfilled, responseWasRejected and ...."
> 
> +1. Is this base class ever supposed to be instantiated? If not can we make it
> an abstract class?

Done.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:35:
WaitUntilObserver*);
On 2017/03/09 04:59:00, falken wrote:
> Is this function used?

Done.

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:167:
RespondWithObserver* respondWithObserver = FetchRespondWithObserver::create(
On 2017/03/08 01:49:04, nhiroki (slow) wrote:
> Can you replace RespondWithObserver* with FetchRespondWithObserver*?
> 
> It'd be better to use the concrete class name instead of the base class name
as
> much as possible in terms of readability and compilability. If this change
> affects wide areas, a separate CL is ok.

Done.

[falken, 2017-03-14 07:33:20]

lgtm

https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
nit: I believe we only use the current year for a totally new file that wasn't
copied from elsewhere.

https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:45:
virtual void onResponseRejected(WebServiceWorkerResponseError) = 0;
Can you add lightweight comments for these:

// Called when the respondWith() promise was rejected.

// Called when the respondWith() promise was fulfilled.

// Called when the the event handler finished without calling respondWith().

[haraken, 2017-03-14 09:01:10]

LGTM

[zino, 2017-03-14 14:10:05]

Thank you for review.

https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/03/14 07:33:20, falken wrote:
> nit: I believe we only use the current year for a totally new file that wasn't
> copied from elsewhere.

Done.

https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:45:
virtual void onResponseRejected(WebServiceWorkerResponseError) = 0;
On 2017/03/14 07:33:20, falken wrote:
> Can you add lightweight comments for these:
> 
> // Called when the respondWith() promise was rejected.
> 
> // Called when the respondWith() promise was fulfilled.
> 
> // Called when the the event handler finished without calling respondWith().

Done.

[please use gerrit instead, 2017-03-14 14:11:39]

No payments related changes in here, so I'm removing myself from reviewers list.

[please use gerrit instead, 2017-03-14 14:31:48]

rouslan@chromium.org changed reviewers:
- rouslan@chromium.org

[nhiroki, 2017-03-14 15:10:38]

lgtm

https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h (right):

https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h:22: class
FetchRespondWithObserver;
This is not necessary because FetchRespondWithObserver.h is already included.

https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:51: //
Called when the the event handler finished without calling respondWith().
s/the the/the

[zino, 2017-03-14 15:56:15]

Thank you for review.

https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h (right):

https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h:22: class
FetchRespondWithObserver;
On 2017/03/14 15:10:38, nhiroki (slow) wrote:
> This is not necessary because FetchRespondWithObserver.h is already included.

Good catch :P

I think forward declartion is better.
I'll move the 'include' macro to .cpp file.

https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:51: //
Called when the the event handler finished without calling respondWith().
On 2017/03/14 15:10:38, nhiroki (slow) wrote:
> s/the the/the

Done.

[zino, 2017-03-14 15:56:24]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2017-03-14 15:56:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, falken@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2715663002/#ps80001
(title: "ServiceWorker: Factor out FetchEvent related logics from
RespondWithObserver.")

[commit-bot: I haz the power, 2017-03-14 15:57:01]

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-14 18:01:53]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489506984180730,
"parent_rev": "d43d808612d46fae29f53b14c9e1dc212a823e44", "commit_rev":
"87bcdeadc1c7bdeb590472d36fc1458e9641274d"}

[commit-bot: I haz the power, 2017-03-14 18:03:41]

Description was changed from

==========
ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver.

To implement respondWith() in PaymentRequestEvent, it's better to factor out
the FetchEvent related logics from RespondWithObserver and then make it as
common class to share common logic between FetchEvent and PaymentRequestEvent.

This CL doesn't change any existing behavior.

See the other CLs in this series:
  [1/3] This patch
  [2/3] https://codereview.chromium.org/2705293010/ (blink side)
  [3/3] https://codereview.chromium.org/2718013004/ (content side and test)

BUG=661608
==========

to

==========
ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver.

To implement respondWith() in PaymentRequestEvent, it's better to factor out
the FetchEvent related logics from RespondWithObserver and then make it as
common class to share common logic between FetchEvent and PaymentRequestEvent.

This CL doesn't change any existing behavior.

See the other CLs in this series:
  [1/3] This patch
  [2/3] https://codereview.chromium.org/2705293010/ (blink side)
  [3/3] https://codereview.chromium.org/2718013004/ (content side and test)

BUG=661608

Review-Url: https://codereview.chromium.org/2715663002
Cr-Commit-Position: refs/heads/master@{#456760}
Committed:
https://chromium.googlesource.com/chromium/src/+/87bcdeadc1c7bdeb590472d36fc1...
==========

[commit-bot: I haz the power, 2017-03-14 18:03:42]

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