Code review comment for lp:~mandel/unity/generic-payment-preview

You have some text conflicts in dash/DashView.cpp & UnityCore/Preview.cpp

27 + * Copyright (C) 2011-2012 Canonical Ltd

41 + * Authored by: Neil Jagdish Patel <email address hidden>
42 + * Michal Hruby <email address hidden>

Please update all headers to include 2013 and your contact details

277 {
278 +
279 if (!preview_displaying_)

extra line.

407 +const std::string MusicPaymentPreview::DATA_INFOHINT_ID = "album_purchase_preview";
408 +const std::string MusicPaymentPreview::DATA_PASSWORD_KEY = "password";
409 +const std::string MusicPaymentPreview::CHANGE_PAYMENT_ACTION = "change_payment_method";
410 +const std::string MusicPaymentPreview::FORGOT_PASSWORD_ACTION = "forgot_password";
411 +const std::string MusicPaymentPreview::CANCEL_PURCHASE_ACTION = "cancel_purchase";
412 +const std::string MusicPaymentPreview::PURCHASE_ALBUM_ACTION = "purchase_album";

Is there a reason why these are scoped to MusicPaymentPreview class? Can you put them in global namespace?

936 + std::map<std::string, nux::ObjectPtr<nux::AbstractButton>> sorted_buttons_;

What is sorted about a map ? ;)

701 + actions_layout->AddView(
702 + sorted_buttons_[MusicPaymentPreview::CHANGE_PAYMENT_ACTION].GetPointer(),
703 + 1, nux::MINOR_POSITION_START, nux::MINOR_SIZE_FULL,
704 + 100.0f, nux::NUX_LAYOUT_END);
705 + actions_layout->AddView(
706 + sorted_buttons_[MusicPaymentPreview::FORGOT_PASSWORD_ACTION].GetPointer(),
707 + 1, nux::MINOR_POSITION_START, nux::MINOR_SIZE_FULL,
708 + 100.0f, nux::NUX_LAYOUT_END);

728 + buttons_data_layout->AddView(sorted_buttons_[MusicPaymentPreview::CANCEL_PURCHASE_ACTION].GetPointer(),
729 + 1, nux::MINOR_POSITION_CENTER, nux::MINOR_SIZE_FULL, 100.0f,
730 + nux::NUX_LAYOUT_END);
731 + buttons_data_layout->AddView(sorted_buttons_[MusicPaymentPreview::PURCHASE_ALBUM_ACTION].GetPointer(),
732 + 1, nux::MINOR_POSITION_CENTER, nux::MINOR_SIZE_FULL, 100.0f,
733 + nux::NUX_LAYOUT_END);

should check if sorted_buttons_ contains the actions before adding them.

784 + dash::Preview::InfoHintPtrList hints = preview_model_->GetInfoHints();
785 + GVariant *preview_data = NULL;
786 + dash::Preview::InfoHintPtr data_info_hint_ = NULL;
787 + if (!hints.empty())
788 + {
789 + for (dash::Preview::InfoHintPtr info_hint : hints)
790 + {
791 + if (info_hint->id == MusicPaymentPreview::DATA_INFOHINT_ID)
792 + {
793 + preview_data = info_hint->value;
794 + }
795 + if (preview_data != NULL)
796 + {
797 + error_message_ = GetErrorMessage(preview_data);
798 + }
799 + }
800 + }

1. data_info_hint_ doesn't seem to be used.
2. Don't need to check if the hints are empty if you're iterating over them.
3. dash::Preview::InfoHintPtr const& info_hint. even though it's a pointer, you're still constructing the wrapper.
4. Not sure what this is doing, but i might be missing something. looping over the info hints. if it is a speicifc id, then set the preview data and get an error message; otherwise continue looping, but dont update the data, even though you are continuing to do a GetErrorMessage for it?

765 + // set the tab ordering
766 + SetFirstInTabOrder(password_entry_->text_entry());
767 + SetLastInTabOrder(sorted_buttons_[MusicPaymentPreview::CANCEL_PURCHASE_ACTION].GetPointer());
768 + SetLastInTabOrder(sorted_buttons_[MusicPaymentPreview::PURCHASE_ALBUM_ACTION].GetPointer());
769 + SetLastInTabOrder(sorted_buttons_[MusicPaymentPreview::CHANGE_PAYMENT_ACTION].GetPointer());
770 + SetLastInTabOrder(sorted_buttons_[MusicPaymentPreview::FORGOT_PASSWORD_ACTION].GetPointer());

Why are you setting the tab order in inside PreLayoutManagement? Shouldn't it be done only once when you create the views?

1431 + virtual void PreLayoutManagement() = 0;

This is a virtual method in nux::Area I think. Why are you making it pure virtual in PaymentPreview?

1735 +
1736 +

extra lines

1778 +using namespace testing;

can you move this to after the includes please.