Code review comment for lp:~azzar1/unity/devices-cleanup-and-test

> Forgive me for the long diff. Refactoring code to make it testable is hard without breaking legacy code.

There is no need to apologize for this - a large diff that gets code under test and makes it cleaner is better than a short diff that does not include test cases.

235 +class DeviceNotificationShower : private boost::noncopyable
236 +{
237 +public:
238 + typedef std::shared_ptr<DeviceNotificationShower> Ptr;
239 +

This class can probably be renamed to DeviceNotificationDisplay - the word "Shower" has two meanings in the english langauge and although it was silly of me to confuse one for the other, I did so anyways.

235 +class DeviceNotificationShower : private boost::noncopyable
236 +{
237 +public:
238 + typedef std::shared_ptr<DeviceNotificationShower> Ptr;
239 +

I'm also not entirely certain about making the interface noncopyable. Generally speaking the object should be noncopyable.

> IconLoader::GetDefault().

Can we remove that singleton?

3019 + g_mock_volume_set_can_eject(gvolume_, TRUE);
3020 + EXPECT_TRUE(volume_->CanBeEjected());

Generally speaking I prefer to operate mocked objects through their interfaces, eg

g_volume_set_can_eject (...); (I assume that you're testing the effect of external GIO code calling set_can_eject through the interface ...).

Also it might be useful for you to hook up your mock implementation of GDrive to a Google Mock directly. That way you can expect that your VolumeLauncherImp is calling the correct function and also determine what the return value is directly within the test code without having to add new api to GMockDrive to set them.

3068 + EXPECT_CALL(*file_manager_opener_, Open("file:///some/directory/testfile"))
3069 + .Times(1);
3070 +
3071 + volume_->MountAndOpenInFileManager();
3072 + EXPECT_TRUE(volume_->IsMounted());

It was a bit confusing as to where: "file:///some/directory/testfile" came from - better make it a constant.