Code review comment for lp://qastaging/~gordallott/unity/dash-legal-link

> Looks good and works well...
>
> 29 + glib::String cachedir(g_strdup(g_get_user_cache_dir()));
>
> No need to strdup it, just assign that to std::string... It should be never
> NULL, in case just check for that.
>
> 30 + legal_seen_file_path_ = cachedir.Str() + "/unitydashlegalseen";
>
> Since we have an unity .cache dir, I'd just add there... i.e.
> cachedir/unity/dashlegalseen

we don't have a unity cache directory, not anymore.

>
> 68 + QueueRelayout();
> 69 + QueueDraw();
>
> The first is enough, as does also redraw.
>
> 64 + info_icon_->SetVisible(info_previously_shown_);
> 65 + legal_->SetVisible(!info_previously_shown_);
> 67 + DoOpenLegalise();
>
> Wouldn't be better to open the legalise (and close the dash) and then switch
> the icon visibility?
>
> 95 + std::string legal_file_path = "file://";
> 96 + legal_file_path.append(PKGDATADIR);
> 97 + legal_file_path.append("/searchingthedashlegalnotice.html");
>
> Why not just doing:
> std::string legal_file_path = "file://" PKGDATADIR
> "/searchingthedashlegalnotice.html";

personal preference

>
> 221 + nux::LayeredLayout* layered_layout_;
> 222 + nux::HLayout *legal_layout_;
>
> It seems you don't need to keep these in class.
>
> Finally, both the icon and the text are always built, only their visibility is
> switched, couldn't avoid this (and then only allocate only the needed
> resources)?

they could, but there is little reason to, its less than 2kb of memory.