Integrate SMS service with Desktop iOS promotion

chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc

Index: chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
index 4979c58621c25bc718e1dfbeb3a0eb90f6bad678..58f89122ab3fb32e087820e800300768d8a1f59e 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
+++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
@@ -830,8 +830,8 @@ void ManagePasswordsBubbleView::CreateChild() {
 #if defined(OS_WIN)
   } else if (model_.state() ==
              password_manager::ui::CHROME_DESKTOP_IOS_PROMO_STATE) {
-    AddChildView(new DesktopIOSPromotionView(
-        desktop_ios_promotion::PromotionEntryPoint::SAVE_PASSWORD_BUBBLE));
+    AddChildView(

[sky, 2017/02/17 00:57:23]

What are the lifetimes here? You have the controller owned by
ManagePasswordsUIController, with a reference to the view and the view has a
reference back to ManagePasswordsUIController. You have Get always creating a
new view. Can the view outlive the controller? Can there be multiple views
created with the same controller?

[mrefaat, 2017/02/17 04:31:50]

The parent view (BubbleView) owns the view. and the password UI controller owns
the controller. 
the UI controller already outlive the Bubble view so there is no way that view
will outlive the controller.

[sky, 2017/02/17 19:06:26]

How does the lifetime of the password ui controller (is that
ManagePasswordsBubbleModel?) relate to that of the bubble? Is there a single
ManagePasswordsBubbleModel for every bubble? This should be documented. Your
code does never unsets the view from the controller, so I want to make sure
there isn't the possibility that the controller outlives the view (bubble).

Earlier I suggested the controller should likely own the view. That was based on
my understanding that you had some long running operations that needed to
outlive the view. That no longer seems to be the case. Should
DesktopIOSPromotionViewsView own and create the controller? That way lifetime is
clearer.

[mrefaat, 2017/02/17 21:53:14]

I think moving the controller to the model and made the password model it also
clear.
And it guarantees that the controller dies with the model that controls the
parent view & the view dies with the parent view.

[sky, 2017/02/17 22:49:32]

Sorry if I wasn't clear, too many models and controllers.
ManagePasswordsBubbleModel doesn't use DesktopIOSPromotionController at all. So,
why not have DesktopIOSPromotionBubbleView create and own
DesktopIOSPromotionController? Lifetime is even easier then.

[mrefaat, 2017/02/18 00:03:25]

Done.

+        static_cast<DesktopIOSPromotionView*>(model_.GetDesktopIOSPromotion()));

[sky, 2017/02/17 00:57:23]

This cast indicates a problem in your design. If GetDesktopIOSPromotion() has to
return a view, then DesktopIOSPromotion should *be* a View. Why is
DesktopIOSPromotion pure virtual in the first place?

[mrefaat, 2017/02/17 04:31:50]

Because we can't have dependency from from the controller to the view.
So i created a pure class that only has the function that is needed. from the
view and made the view implement it.
same for the dependency between the ui and ui/views i understood that ui/views
can use ui But the reverse is not encouraged.
 

[sky, 2017/02/17 19:06:26]

You changed it to create the view directly, which makes much more sense. thanks!

[mrefaat, 2017/02/17 21:53:14]

Done that

 #endif
   } else {
     AddChildView(new ManageView(this));