Make JSONParser a pure interface.

Make JSONParser a pure interface.

This allows stubbing it out in tests, and using platform-specific implementations in the future.

As a side effect, the fact that the default implementation is refcounted is not exposed to clients anymore. Instead, the (newly added) factory method returns a raw pointer, and the object will delete itself after either of the callbacks has been called. In practice, this should not make a difference, as no client currently requires the SafeJsonParser to live longer than that, and in one case it actually simplifies things for the client by breaking a reference cycle.

TBR=benwells@chromium.org,brettw@chromium.org

BUG=501333
Committed: https://crrev.com/64c5a3888f21acf96082e89764e5ee66cc3531b6
Cr-Commit-Position: refs/heads/master@{#337459}

[Bernhard Bauer, 2015-07-01 10:16:04]

bauerb@chromium.org changed reviewers:
+ rsesek@chromium.org

[Bernhard Bauer, 2015-07-01 10:16:05]

Please review.

[Robert Sesek, 2015-07-01 22:19:04]

https://codereview.chromium.org/1214903010/diff/60001/components/safe_json/sa...
File components/safe_json/safe_json_parser.h (right):

https://codereview.chromium.org/1214903010/diff/60001/components/safe_json/sa...
components/safe_json/safe_json_parser.h:33: static SafeJsonParser* Create(const
std::string& unsafe_json,
At minium, ditto the comment about ownership here.

But it seems like there's no point in separating Create() from Start() anymore.
And APIs that return pointers without explicit ownership make it hard to reason
about lifetimes. Could Create() just call Start() internally and return void
(and renamed to Parse() or something)?

https://codereview.chromium.org/1214903010/diff/60001/components/safe_json/sa...
components/safe_json/safe_json_parser.h:34: const SuccessCallback&
success_callback,
nit: alignment

[Bernhard Bauer, 2015-07-02 14:16:01]

https://codereview.chromium.org/1214903010/diff/60001/components/safe_json/sa...
File components/safe_json/safe_json_parser.h (right):

https://codereview.chromium.org/1214903010/diff/60001/components/safe_json/sa...
components/safe_json/safe_json_parser.h:33: static SafeJsonParser* Create(const
std::string& unsafe_json,
On 2015/07/01 22:19:04, Robert Sesek wrote:
> At minium, ditto the comment about ownership here.
> 
> But it seems like there's no point in separating Create() from Start()
anymore.
> And APIs that return pointers without explicit ownership make it hard to
reason
> about lifetimes. Could Create() just call Start() internally and return void
> (and renamed to Parse() or something)?

Done.

I also moved the Impl class to a separate file, to allow sharing the static code
between different implementations.

https://codereview.chromium.org/1214903010/diff/60001/components/safe_json/sa...
components/safe_json/safe_json_parser.h:34: const SuccessCallback&
success_callback,
On 2015/07/01 22:19:04, Robert Sesek wrote:
> nit: alignment

Done.

[Robert Sesek, 2015-07-02 17:28:19]

https://codereview.chromium.org/1214903010/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/webstore_install_helper.h (right):

https://codereview.chromium.org/1214903010/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/webstore_install_helper.h:99: bool started_;
Needs a comment (and maybe a more descriptive name. What is started?

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
File components/safe_json/safe_json_parser.cc (right):

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
components/safe_json/safe_json_parser.cc:28: : new
SafeJsonParserImpl(unsafe_json, success_callback,
Is this going to get released propery? Callers put this in a scoped_refptr<>
before, and now it is not. The testing factory uses DeleteSoon to clean itself
up. What about this?

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
File components/safe_json/safe_json_parser.h (right):

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
components/safe_json/safe_json_parser.h:5: #ifndef
COMPONENTS_SAFE_JSON_SAFE_JSON_PARSER_IMPL_H_
The include guards are swapped for this file and the _imp.h

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
components/safe_json/safe_json_parser.h:33: static void Parse(const std::string&
unsafe_json,
Since this is the only public method, it should have a comment.

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
components/safe_json/safe_json_parser_impl.cc:93:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
I don't know if this DCHECK is really that useful.

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/t...
File components/safe_json/testing_json_parser.h (right):

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/t...
components/safe_json/testing_json_parser.h:28: TestingJsonParser(const
std::string& unsafe_json,
Comment that this must be allocated with new and that it will delete itself
after Start is called.

[Bernhard Bauer, 2015-07-02 22:38:24]

https://codereview.chromium.org/1214903010/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/webstore_install_helper.h (right):

https://codereview.chromium.org/1214903010/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/webstore_install_helper.h:99: bool started_;
On 2015/07/02 17:28:19, Robert Sesek wrote:
> Needs a comment (and maybe a more descriptive name. What is started?

Y'know, we can probably remove it. It was only there to replace the old
|json_parser_| DCHECK, which isn't that important anymore anyway.

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
File components/safe_json/safe_json_parser.cc (right):

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
components/safe_json/safe_json_parser.cc:28: : new
SafeJsonParserImpl(unsafe_json, success_callback,
On 2015/07/02 17:28:19, Robert Sesek wrote:
> Is this going to get released propery? Callers put this in a scoped_refptr<>
> before, and now it is not. The testing factory uses DeleteSoon to clean itself
> up. What about this?

SafeJsonParserImpl is still refcounted, by virtue of inheriting from
UtilityProcessHostClient, and is being kept alive by the UtilityProcessHost and
the callbacks bound to itself. Once all of those are gone, it will delete
itself. The scoped_refptr<> it used to be in does not change that, as it would
be destroyed at the end of Start() anyway. (And SafeJsonParserImpl outlives
Start() because that's when the StartWorkOnIOThread callback is bound, which
creates the first reference to it.)

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
File components/safe_json/safe_json_parser.h (right):

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
components/safe_json/safe_json_parser.h:5: #ifndef
COMPONENTS_SAFE_JSON_SAFE_JSON_PARSER_IMPL_H_
On 2015/07/02 17:28:19, Robert Sesek wrote:
> The include guards are swapped for this file and the _imp.h

D'oh! Fixed.

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
components/safe_json/safe_json_parser.h:33: static void Parse(const std::string&
unsafe_json,
On 2015/07/02 17:28:19, Robert Sesek wrote:
> Since this is the only public method, it should have a comment.

Done.

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
components/safe_json/safe_json_parser_impl.cc:93:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
On 2015/07/02 17:28:19, Robert Sesek wrote:
> I don't know if this DCHECK is really that useful.

True. Removed.

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/t...
File components/safe_json/testing_json_parser.h (right):

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/t...
components/safe_json/testing_json_parser.h:28: TestingJsonParser(const
std::string& unsafe_json,
On 2015/07/02 17:28:19, Robert Sesek wrote:
> Comment that this must be allocated with new and that it will delete itself
> after Start is called.

Done.

[Robert Sesek, 2015-07-06 17:31:35]

LGTM

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
File components/safe_json/safe_json_parser.cc (right):

https://codereview.chromium.org/1214903010/diff/100001/components/safe_json/s...
components/safe_json/safe_json_parser.cc:28: : new
SafeJsonParserImpl(unsafe_json, success_callback,
On 2015/07/02 22:38:23, Bernhard Bauer wrote:
> On 2015/07/02 17:28:19, Robert Sesek wrote:
> > Is this going to get released propery? Callers put this in a scoped_refptr<>
> > before, and now it is not. The testing factory uses DeleteSoon to clean
itself
> > up. What about this?
> 
> SafeJsonParserImpl is still refcounted, by virtue of inheriting from
> UtilityProcessHostClient, and is being kept alive by the UtilityProcessHost
and
> the callbacks bound to itself. Once all of those are gone, it will delete
> itself. The scoped_refptr<> it used to be in does not change that, as it would
> be destroyed at the end of Start() anyway. (And SafeJsonParserImpl outlives
> Start() because that's when the StartWorkOnIOThread callback is bound, which
> creates the first reference to it.)

Ah, right. Our C++ RefCounted impl starts the ref-count at 0, and scoped_refptr
adds a ref rather than adopting an existing one.

[Bernhard Bauer, 2015-07-06 17:47:45]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2015-07-06 17:47:51]

The CQ bit was unchecked by bauerb@chromium.org

[Bernhard Bauer, 2015-07-06 17:49:06]

bauerb@chromium.org changed reviewers:
+ benwells@chromium.org

[Bernhard Bauer, 2015-07-06 18:10:55]

bauerb@chromium.org changed reviewers:
+ brettw@chromium.org

[Bernhard Bauer, 2015-07-06 18:16:12]

Adding OWNERS TBR (Ben for extensions and app_list, Brett for remaining files):
Build file changes and updating call sites.

[Bernhard Bauer, 2015-07-06 18:16:19]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2015-07-06 18:18:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1214903010/120001

[commit-bot: I haz the power, 2015-07-06 20:09:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-06 20:11:26]

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

[benwells, 2015-07-07 00:53:55]

nice change, lgtm

[brettw, 2015-07-07 22:17:21]

I noticed some problems with this component (not added in this patch). Can an
appropriate person take a look?

https://codereview.chromium.org/1214903010/diff/120001/components/safe_json/B...
File components/safe_json/BUILD.gn (right):

https://codereview.chromium.org/1214903010/diff/120001/components/safe_json/B...
components/safe_json/BUILD.gn:6: static_library("safe_json") {
This static library should be a source set. Static libraries should basically
never be used in GN unless you want a .a for some specific reason (it's slower).

https://codereview.chromium.org/1214903010/diff/120001/components/safe_json/B...
components/safe_json/BUILD.gn:14: defines = [ "SAFE_JSON_IMPLEMENTATION" ]
This does nothing and should be removed. GYP also. This is only for if you're
doing components and have a _export.h file that reads this define.

https://codereview.chromium.org/1214903010/diff/120001/components/safe_json/B...
components/safe_json/BUILD.gn:40:
static_library("safe_json_parser_message_filter") {
source_set

[Bernhard Bauer, 2015-07-08 07:53:48]

On 2015/07/07 22:17:21, brettw wrote:
> I noticed some problems with this component (not added in this patch). Can an
> appropriate person take a look?

Doing a followup in https://codereview.chromium.org/1211423004/.

FWIW, there are a *lot* of static_library targets in existing BUILD.gn files. Is
there a plan to a) get rid of all of them, and b) prevent hapless authors from
adding more?