Disable uses of ProfileImportProcess on Android.

Disable uses of ProfileImportProcess on Android.

This is part of the unit_tests linking effort on Android.

This moves the profile import-related code excluded on Android to its own class.

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

[Yaron, 2012-07-16 19:33:39]

https://chromiumcodereview.appspot.com/10777012/diff/1/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://chromiumcodereview.appspot.com/10777012/diff/1/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:397: void
ChromeContentUtilityClient::OnImportStart(
Does any of this logic actually intermix with the rest of this class? A quick
pass looks like all the relevant member variables are currently only accessed in
this block. If that's the case, It might be clearer to split off a separate
ImportHandler (or something to that effect) and conditionally initialize it on
Android. I think it would be more maintainable that way.

[Philippe, 2012-07-17 09:48:27]

https://chromiumcodereview.appspot.com/10777012/diff/1/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://chromiumcodereview.appspot.com/10777012/diff/1/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:397: void
ChromeContentUtilityClient::OnImportStart(
On 2012/07/16 19:33:39, Yaron wrote:
> Does any of this logic actually intermix with the rest of this class? A quick
> pass looks like all the relevant member variables are currently only accessed
in
> this block. If that's the case, It might be clearer to split off a separate
> ImportHandler (or something to that effect) and conditionally initialize it on
> Android. I think it would be more maintainable that way.

Done. Good idea!

[Yaron, 2012-07-17 19:13:53]

https://chromiumcodereview.appspot.com/10777012/diff/5001/chrome/utility/chro...
File chrome/utility/chrome_content_utility_client.cc (right):

https://chromiumcodereview.appspot.com/10777012/diff/5001/chrome/utility/chro...
chrome/utility/chrome_content_utility_client.cc:61: class
ChromeContentUtilityClient::ImportHandler {
Why not split this out to a separate file and then gyp it out for Android?

https://chromiumcodereview.appspot.com/10777012/diff/5001/chrome/utility/chro...
chrome/utility/chrome_content_utility_client.cc:179: IPC_MESSAGE_FORWARD(
couldn't you also ask your ImportHandler to register for IPC messages that it
cares about (so it's all localized in one place)?

[Philippe, 2012-07-18 09:34:27]

https://chromiumcodereview.appspot.com/10777012/diff/5001/chrome/utility/chro...
File chrome/utility/chrome_content_utility_client.cc (right):

https://chromiumcodereview.appspot.com/10777012/diff/5001/chrome/utility/chro...
chrome/utility/chrome_content_utility_client.cc:61: class
ChromeContentUtilityClient::ImportHandler {
On 2012/07/17 19:13:53, Yaron wrote:
> Why not split this out to a separate file and then gyp it out for Android?

Good point. I kept the dummy implementation for Android which I moved to its own
file to avoid adding too many #ifdefs here.

https://chromiumcodereview.appspot.com/10777012/diff/5001/chrome/utility/chro...
chrome/utility/chrome_content_utility_client.cc:179: IPC_MESSAGE_FORWARD(
On 2012/07/17 19:13:53, Yaron wrote:
> couldn't you also ask your ImportHandler to register for IPC messages that it
> cares about (so it's all localized in one place)?

Good point.

[Yaron, 2012-07-18 15:30:39]

lgtm modulo nits but would prefer you get a look from someone related to profile
code

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/chro...
File chrome/utility/chrome_content_utility_client.h (right):

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/chro...
chrome/utility/chrome_content_utility_client.h:81:
scoped_ptr<ProfileImportHandler> import_handler_;
Why bother with a scoped_ptr? this has the same lifetime and lifecycle as
ChromeContentUtilityClient

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/prof...
File chrome/utility/profile_import_handler.h (right):

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/prof...
chrome/utility/profile_import_handler.h:31: class ProfileImportHandler : public
IPC::Listener {
Brief class comment

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/prof...
chrome/utility/profile_import_handler.h:46: ImporterCleanup();
move to cc file

[Philippe, 2012-07-18 16:01:17]

Thanks Yaron.

I'm adding John who seems to be the initial author.

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/chro...
File chrome/utility/chrome_content_utility_client.h (right):

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/chro...
chrome/utility/chrome_content_utility_client.h:81:
scoped_ptr<ProfileImportHandler> import_handler_;
On 2012/07/18 15:30:39, Yaron wrote:
> Why bother with a scoped_ptr? this has the same lifetime and lifecycle as
> ChromeContentUtilityClient

This was used initially to hide ProfileImportHandler which was in the cc file.
Now we don't need that anymore indeed.

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/prof...
File chrome/utility/profile_import_handler.h (right):

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/prof...
chrome/utility/profile_import_handler.h:31: class ProfileImportHandler : public
IPC::Listener {
On 2012/07/18 15:30:39, Yaron wrote:
> Brief class comment

Done.

https://chromiumcodereview.appspot.com/10777012/diff/8001/chrome/utility/prof...
chrome/utility/profile_import_handler.h:46: ImporterCleanup();
On 2012/07/18 15:30:39, Yaron wrote:
> move to cc file

Done.

[jam, 2012-07-18 16:28:32]

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/chrome_cont...
File chrome/utility/chrome_content_utility_client.h (right):

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/chrome_cont...
chrome/utility/chrome_content_utility_client.h:79: ProfileImportHandler
import_handler_;
if you make this in a scoped_ptr, and only create it if !android, then you don't
need profile_import_handler_android.cc

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/profile_imp...
File chrome/utility/profile_import_handler.h (right):

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/profile_imp...
chrome/utility/profile_import_handler.h:26: class Message;
nit: not needed  since you include ipc_listener.h

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/profile_imp...
chrome/utility/profile_import_handler.h:31: // Class used by
ChromeContentUtilityClient that handles profile import. This
this comment is just describing what the code does in a way that someone
scanning the file can see immediately, so it's not very useful. I would just say
"Dispatches IPCs for out of process profile import."

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/profile_imp...
chrome/utility/profile_import_handler.h:48: void OnImportCancel();
nit: put all the IPC message handlers beside each other, and no blank lines in
between

[Philippe, 2012-07-18 16:54:51]

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/chrome_cont...
File chrome/utility/chrome_content_utility_client.h (right):

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/chrome_cont...
chrome/utility/chrome_content_utility_client.h:79: ProfileImportHandler
import_handler_;
On 2012/07/18 16:28:33, John Abd-El-Malek wrote:
> if you make this in a scoped_ptr, and only create it if !android, then you
don't
> need profile_import_handler_android.cc

Done. Note that I still include profile_import_handler.h on all platforms to
avoid triggering the static assertion in scoped_ptr<T>::~scoped_ptr() when T =
incomplete type.

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/profile_imp...
File chrome/utility/profile_import_handler.h (right):

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/profile_imp...
chrome/utility/profile_import_handler.h:26: class Message;
On 2012/07/18 16:28:33, John Abd-El-Malek wrote:
> nit: not needed  since you include ipc_listener.h

Done.

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/profile_imp...
chrome/utility/profile_import_handler.h:31: // Class used by
ChromeContentUtilityClient that handles profile import. This
On 2012/07/18 16:28:33, John Abd-El-Malek wrote:
> this comment is just describing what the code does in a way that someone
> scanning the file can see immediately, so it's not very useful. I would just
say
> "Dispatches IPCs for out of process profile import."

Done.

http://codereview.chromium.org/10777012/diff/17001/chrome/utility/profile_imp...
chrome/utility/profile_import_handler.h:48: void OnImportCancel();
On 2012/07/18 16:28:33, John Abd-El-Malek wrote:
> nit: put all the IPC message handlers beside each other, and no blank lines in
> between

Done.

[jam, 2012-07-18 17:12:17]

lgtm

[Philippe, 2012-07-18 17:13:54]

On 2012/07/18 17:12:17, John Abd-El-Malek wrote:
> lgtm

Thanks John.

[commit-bot: I haz the power, 2012-07-18 17:14:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10777012/19001

[commit-bot: I haz the power, 2012-07-18 17:35:23]

Try job failure for 10777012-19001 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-18 17:41:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10777012/16004

[commit-bot: I haz the power, 2012-07-18 19:08:48]

Change committed as 147285