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.

Will do!

>
> 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.
>

We really don't need to make that object copyable. Also making in un-copyable force us to use it through a pointer.

> > IconLoader::GetDefault().
>

IconLoader is used in dash/hud/launcher/switcher code. I think it's not a good idea to do it in this branch.

> 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 ...).

Here I'm testing the VolumeImp mocking GVolumeIface.

>
> 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.
>

Can we mock a GObject using Google Mock?

> 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.

Will fix it.