Stub out ServiceWorkerRegistration and Version classes

Stub out ServiceWorkerRegistration and Version classes

This stubs out the basic registration classes, spun out of http://crrev.com/62203007. Tests are included which make sure that shutdown is reliable. A single, simplistic ActivateInWaitingVersion() exists as a way to demonstrate the relationship between in-waiting and active versions.

BUG=
TBR=jam@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235639

[michaeln, 2013-11-13 01:53:23]

i mostly have style comments

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:22: // This class
acts as a persistent instance
This class comment and the other could use some work.

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:30: int64 id()
const { return registration_id_; }
maybe a const getter for 'pattern' too and co-locate the unix-hacker-style
getters

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:37:
DCHECK(!is_shutdown_);
dcheck'ing the setters seems useful, we don't dcheck in the other one-line
getters..

ServiceWorkerVersion* active_version() const { return active_version_; }

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:41:
scoped_refptr<ServiceWorkerVersion> in_waiting_version() const {
Terminology... wdyt of using the term "pending" instead of "in_waiting"?

And does ServiceWorkerVersion* work as a return value here?

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:46: void
set_active_version(scoped_refptr<ServiceWorkerVersion> version) {
Does ServiceWorkerVersion* work as a parameter here?

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:67: bool
IsShutdown() const { return is_shutdown_; }
why is this one not unix-hacker-style? also since its a private method consumers
could just test the is_shutdown_ member w/o the having a getter

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration_unittest.cc:29: 
style nit: blank line not needed (ditto below)

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration_unittest.cc:67:
registration->ActivateInWaitingVersion();
lets DCHECK_EQ(version_2, registration->active_version());

[alecflett, 2013-11-13 18:28:23]

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:30: int64 id()
const { return registration_id_; }
On 2013/11/13 01:53:24, michaeln wrote:
> maybe a const getter for 'pattern' too and co-locate the unix-hacker-style
> getters

Done.

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:37:
DCHECK(!is_shutdown_);
On 2013/11/13 01:53:24, michaeln wrote:
> dcheck'ing the setters seems useful, we don't dcheck in the other one-line
> getters..

I suspect that we might want access to things like the id / script_url / pattern
during shutdown, but accessing the various ServiceWorkerVersions seems wrong.
I'd rather ad additional DCHECKs than remove the ones that are there

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:41:
scoped_refptr<ServiceWorkerVersion> in_waiting_version() const {
On 2013/11/13 01:53:24, michaeln wrote:
> Terminology... wdyt of using the term "pending" instead of "in_waiting"?
SGTM

> 
> And does ServiceWorkerVersion* work as a return value here?

Why? I mean if we're using refcounting, we should just use it right? I found
lots of precedent  for it:
https://code.google.com/p/chromium/codesearch#search/&q=%5E%5C%20*scoped_refp...

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:46: void
set_active_version(scoped_refptr<ServiceWorkerVersion> version) {
On 2013/11/13 01:53:24, michaeln wrote:
> Does ServiceWorkerVersion* work as a parameter here?

I did however switch the setters to take a const scoped_refptr<> to avoid
refcount churn...

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration.h:67: bool
IsShutdown() const { return is_shutdown_; }
On 2013/11/13 01:53:24, michaeln wrote:
> why is this one not unix-hacker-style? also since its a private method
consumers
> could just test the is_shutdown_ member w/o the having a getter

good point. I moved this to be public, which let me remove the FRIEND_TEST*
declarations.

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration_unittest.cc:29: 
On 2013/11/13 01:53:24, michaeln wrote:
> style nit: blank line not needed (ditto below)

Done.

https://codereview.chromium.org/70683002/diff/1/content/browser/service_worke...
content/browser/service_worker/service_worker_registration_unittest.cc:67:
registration->ActivateInWaitingVersion();
On 2013/11/13 01:53:24, michaeln wrote:
> lets DCHECK_EQ(version_2, registration->active_version());

Done.

[michaeln, 2013-11-14 00:27:33]

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:51: return
active_version_.get();
> And does ServiceWorkerVersion* work as a return value here?
You said... "I mean if we're using refcounting, we should just use it right?"

The very big assumption in that comment is that passing and returning these
structs is somehow "right". I'm not so sure about that. This style decreases
readability and encourages refcount churn. Given two correct answers, i'll take
the simpler more readable of the two. Of course... ymmv :)

also, this getter uses .get() while the next does not

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:78: const GURL&
script_url_;
these &'s are neat looking characters, but please delete them :)

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:31: //            
during unregistration.
This is a helpful comment. I think there's some blurring of lines in the states
you listed, they're blurring the notion of running/notrunning vs the
"installstate".

I think these states you listed are orthogonal to running, I'd call them
"InstallStates". Shutdown is oddly named for that last install state. The method
as coded is more about unwinding the object graph than transitioning install
states.

InstallState install_state_;  // pending -> active -> obsolete(?)
RunState run_state_;   // stopped --> starting -> running -> stopping

bool is_shutdown_;   // implies nothing of install_state_ or run_state_, just
object teardown gunk

wdyt?

[alecflett, 2013-11-14 18:35:46]

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:51: return
active_version_.get();
On 2013/11/14 00:27:33, michaeln wrote:
> > And does ServiceWorkerVersion* work as a return value here?
> You said... "I mean if we're using refcounting, we should just use it right?"
> 
> The very big assumption in that comment is that passing and returning these
> structs is somehow "right". I'm not so sure about that. This style decreases
> readability and encourages refcount churn. Given two correct answers, i'll
take
> the simpler more readable of the two. Of course... ymmv :)

While I disagree on both of these points, I defer to chromium style, which was
resolved here:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/uuaKaTzaCwE

> 
> also, this getter uses .get() while the next does not

Done.

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:78: const GURL&
script_url_;
On 2013/11/14 00:27:33, michaeln wrote:
> these &'s are neat looking characters, but please delete them :)

woah. Good catch. Done.

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/70683002/diff/120001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:31: //            
during unregistration.
On 2013/11/14 00:27:33, michaeln wrote:
> This is a helpful comment. I think there's some blurring of lines in the
states
> you listed, they're blurring the notion of running/notrunning vs the
> "installstate".
> 
> I think these states you listed are orthogonal to running, I'd call them
> "InstallStates". Shutdown is oddly named for that last install state. The
method
> as coded is more about unwinding the object graph than transitioning install
> states.
> 
> InstallState install_state_;  // pending -> active -> obsolete(?)
> RunState run_state_;   // stopped --> starting -> running -> stopping

Yeah this is a good breakdown. I'll clean up this comment. (But I won't add the
member variables just yet because compilers will complain if we declare and
don't use them.

> 
> bool is_shutdown_;   // implies nothing of install_state_ or run_state_, just
> object teardown gunk
> 
> wdyt?

[michaeln, 2013-11-14 22:30:55]

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:23: Shutdown();
not sure this call needs to be here, if there is an active or pending version,
then we wouldn't be here because of the ref the version holds on 'this'

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:26: void
ServiceWorkerRegistration::Shutdown() {
maybe if (shutdown_) return

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:59: void
set_active_version(const scoped_refptr<ServiceWorkerVersion>& version) {
i'd prefer to go all the way and use raw ptrs here to for the sake of
consistency and simplicity

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:64: void
set_pending_version(const scoped_refptr<ServiceWorkerVersion>& version) {
and here

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:55:
scoped_refptr<ServiceWorkerRegistration> registration);
and here

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:57: void Shutdown();
I think we expect the only callsite for Shutdown() to be in SWRegistration (is
that right)?

I'm not sure we've thought thru the who owns what completely, but we'll edit to
taste going forward.

[michaeln, 2013-11-14 22:44:08]

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:17: 
nit: extra blank line

[alecflett, 2013-11-15 01:51:39]

While I personally believe this makes our code less safe and more error prone,
in the interest of moving forward, I've removed all parameters of type 'const
scoped_refptr<X>&.

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:26: void
ServiceWorkerRegistration::Shutdown() {
On 2013/11/14 22:30:55, michaeln wrote:
> maybe if (shutdown_) return

I'll DCHECK() and if we ever get into a situation where we're calling Shutdown()
twice, we can re-check our assumptions.

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/70683002/diff/190001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:17: 
On 2013/11/14 22:44:08, michaeln wrote:
> nit: extra blank line

Done.

[michaeln, 2013-11-15 20:25:00]

lgtm (thnx)

[commit-bot: I haz the power, 2013-11-15 22:56:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/70683002/270001

[commit-bot: I haz the power, 2013-11-16 00:08:10]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-16 03:13:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/70683002/270001

[kinuko, 2013-11-18 02:18:24]

lgtm2, and a few nits while it's stuck in CQ :)

https://codereview.chromium.org/70683002/diff/270001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/70683002/diff/270001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:51: class CONTENT_EXPORT
ServiceWorkerVersion
nit: Do we really need to export all these classes?

https://codereview.chromium.org/70683002/diff/270001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:61: friend class
base::RefCounted<ServiceWorkerVersion>;
style-nit: can you move this line right after private: / before dtor

[commit-bot: I haz the power, 2013-11-18 06:14:42]

Change committed as 235639

[alecflett, 2013-11-18 17:50:07]

https://codereview.chromium.org/70683002/diff/270001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/70683002/diff/270001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:51: class CONTENT_EXPORT
ServiceWorkerVersion
On 2013/11/18 02:18:24, kinuko wrote:
> nit: Do we really need to export all these classes?

Yep, in order to write unit tests..I feel like CONTENT_EXPORT really needs to be
split into CONTENT_EXPORT and CONTENT_UNITTEST_EXPORT or something, since I've
done this with every class!

https://codereview.chromium.org/70683002/diff/270001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:61: friend class
base::RefCounted<ServiceWorkerVersion>;
On 2013/11/18 02:18:24, kinuko wrote:
> style-nit: can you move this line right after private: / before dtor

Sure, I'll take care of this in the next patch, hopefully up for review today
(http://crrev.com/62203007)