Code review comment for lp:~unity-team/qtmir/shell_chrome

Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

> As I said in the unity-api MP, I don't understand what you mean by "shell
> chrome" and suspect there might be better terminology.
>
> Am happy to see the ExecFlags stuff going away.
>
>
> +void MirSurfaceManager::onSurfaceModified(const
> std::shared_ptr<mir::scene::Surface> & surface,
> nitpick - have the & beside the variable name.

done.

>
> In this method, I'm not convinced why mutex needed. This will be called on the
> GUI thread, since it a slot connected to via a queued connection.

I didn't add the mutex, but it's protecting the "surface to qml surface" hash in other methods, so just doing the same. Either keep, or remove all.

>
> You've a blank line after the if statement here too.
>

Done.

> +++ src/modules/Unity/Application/mirsurfacemanager.h
> + const QVariant& value);
> nitpick - & beside the var name please.
>

Done.

> +++ src/platforms/mirserver/windowmanagerlistener.h

Removed this class. Not used.

> +++ src/platforms/mirserver/mirwindowmanager.h
> In both files:
> + void surfaceMofidied(const std::shared_ptr<mir::scene::Surface>& surface,
> Type. Plus & beside var name.

Done.

« Back to merge proposal