Extracted inner class doing process dispatch.

Extracted inner class doing process dispatch.

The inner class WebResourceService::UnpackerClient is dispatching a json parsing job
to a subprocess. In iOS launching a subprocess is not supported. In order to replace
this class on this particular platform by one doing a thread dispatch the class has
been renamed and refactored in its own file. In doing so an abstract superclass was
crated to make the API more obvious.

The iOS subclass already exists in the iOS tree, and will be brought up with the
rest of the iOS browser changes.


BUG=b/6753678
TEST=No user visible changes.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145689

[noyau (Ping after 24h), 2012-07-09 13:44:03]

Fixing an initialization issue.

[Miranda Callahan, 2012-07-09 14:24:30]

Mostly a bunch of spelling nits, but one structural issue.

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:98: #endif  //
ENABLE_PROMO_RESOURCE_SERVICE
nice, thanks!

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
File chrome/browser/web_resource/json_asynchronous_unpacker.cc (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/json_asynchronous_unpacker.cc:10: #include
"chrome/common/web_resource/web_resource_unpacker.h"
nit: alphaorder

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
File chrome/browser/web_resource/json_asynchronous_unpacker.h (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/json_asynchronous_unpacker.h:27: // OnUnpack<xxx>
method.
nit: s/method/methods.

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/json_asynchronous_unpacker.h:31: // delegate must be
kept around until one of the delegate method is called.
nit: s/method/methods.

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/promo_resource_service.cc:95:
WebResourceService::OnUnpackFinished(parsed_json);
see comment in web_resource.cc; I think we should just call EndFetch here (and
below).

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
File chrome/browser/web_resource/web_resource_service.cc (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/web_resource_service.cc:46: void
WebResourceService::OnUnpackFinished(const DictionaryValue& parsed_json) {
Having these in both the WRS and the derived PRS is seems awkward, as all these
are doing is wrapping a method we already have defined -- and they're not using
the parameters passed in. I think it makes more sense to make these pure virtual
methods that the derived class must define. Then the derived class sensibly
calls "EndFetch" at the end of their unpackFinished and unpackError methods,
rather than awkwardly calling up to the superclass, and passing information that
is never used.

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
File chrome/browser/web_resource/web_resource_service.h (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/web_resource_service.h:80: // The tool that parse
and transform the json data. Weak reference as it
nit:s/parse/parses.

[noyau (Ping after 24h), 2012-07-09 15:23:56]

Revamped a bit the delegate implementation.

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
File chrome/browser/web_resource/json_asynchronous_unpacker.cc (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/json_asynchronous_unpacker.cc:10: #include
"chrome/common/web_resource/web_resource_unpacker.h"
On 2012/07/09 14:24:30, Miranda Callahan wrote:
> nit: alphaorder

Done.

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
File chrome/browser/web_resource/json_asynchronous_unpacker.h (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/json_asynchronous_unpacker.h:27: // OnUnpack<xxx>
method.
On 2012/07/09 14:24:30, Miranda Callahan wrote:
> nit: s/method/methods.

Done.

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/json_asynchronous_unpacker.h:31: // delegate must be
kept around until one of the delegate method is called.
On 2012/07/09 14:24:30, Miranda Callahan wrote:
> nit: s/method/methods.

Done.

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
File chrome/browser/web_resource/web_resource_service.cc (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/web_resource_service.cc:46: void
WebResourceService::OnUnpackFinished(const DictionaryValue& parsed_json) {
On 2012/07/09 14:24:30, Miranda Callahan wrote:
> Having these in both the WRS and the derived PRS is seems awkward, as all
these
> are doing is wrapping a method we already have defined -- and they're not
using
> the parameters passed in. I think it makes more sense to make these pure
virtual
> methods that the derived class must define. Then the derived class sensibly
> calls "EndFetch" at the end of their unpackFinished and unpackError methods,
> rather than awkwardly calling up to the superclass, and passing information
that
> is never used. 

EndFetch IMHO should stay private to the class. It seem awkward to ask a
subclass to implement two virtual method and not forget to call a method on the
superclass.

Instead I turned this around: only the superclass implements the delegate and I
returned the virtual version of the Unpack method.

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
File chrome/browser/web_resource/web_resource_service.h (right):

https://chromiumcodereview.appspot.com/10690096/diff/11002/chrome/browser/web...
chrome/browser/web_resource/web_resource_service.h:80: // The tool that parse
and transform the json data. Weak reference as it
On 2012/07/09 14:24:30, Miranda Callahan wrote:
> nit:s/parse/parses.

Done.

[Miranda Callahan, 2012-07-09 15:28:11]

LGTM with comment added.

https://chromiumcodereview.appspot.com/10690096/diff/14001/chrome/browser/web...
File chrome/browser/web_resource/web_resource_service.h (right):

https://chromiumcodereview.appspot.com/10690096/diff/14001/chrome/browser/web...
chrome/browser/web_resource/web_resource_service.h:55: virtual void Unpack(const
base::DictionaryValue& parsed_json) = 0;
Great solution. Could you just add a comment here that says something like every
subclass has to unpack the data in its own way?

[noyau (Ping after 24h), 2012-07-09 15:41:42]

https://chromiumcodereview.appspot.com/10690096/diff/14001/chrome/browser/web...
File chrome/browser/web_resource/web_resource_service.h (right):

https://chromiumcodereview.appspot.com/10690096/diff/14001/chrome/browser/web...
chrome/browser/web_resource/web_resource_service.h:55: virtual void Unpack(const
base::DictionaryValue& parsed_json) = 0;
On 2012/07/09 15:28:11, Miranda Callahan wrote:
> Great solution. Could you just add a comment here that says something like
every
> subclass has to unpack the data in its own way?

Done.

[commit-bot: I haz the power, 2012-07-09 15:42:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noyau@chromium.org/10690096/19002

[commit-bot: I haz the power, 2012-07-09 17:14:11]

Change committed as 145689