Create Bookmark Footnote desktop iOS promotion

Create Bookmark Footnote desktop iOS promotion.

Create a footnote that promote for chrome on iOS on the bookmark bubble,
when the footnote link is clicked the desktop iOS promotion Bubble will appear.

Introduce new entry point: FootnoteFollowupBubble this is used to distinguish in logging between bubble used on the original bookmark promo and the bubble that appear when link on footnote is clicked.

Update BubbleFrameView to allow removing the footnote in the transition between clicking the footnote link and showing the bookmark promotion bubble.

example: https://drive.google.com/a/google.com/file/d/0B9T1TKmsVG94YVZRUkJzdDltS2M/view
Review-Url: https://codereview.chromium.org/2803293002
Cr-Commit-Position: refs/heads/master@{#474593}
Committed: https://chromium.googlesource.com/chromium/src/+/862a261b9f735eabd3b17b08fe4e7e3873c8c14e

[mrefaat, 2017-05-11 20:53:15]

Patchset #1 (id:1) has been deleted

[mrefaat, 2017-05-11 21:53:39]

Description was changed from

==========
Add footnote desktop to iOS promotion

This is not a complete CL, the problem is the footnote still appears on the
promotion bubble it self.



BUG=676655
==========

to

==========
Create Bookmark Footnote desktop iOS promotion.

Create a footnote that promote for chrome on iOS on the bookmark bubble,
when the footnote link is clicked the desktop iOS promotion Bubble will appear.

Update BubbleFrameView to allow removing the footnote in the transition between
clicking the footnote link and showing the bookmark promotion bubble.

BUG=676655
==========

[mrefaat, 2017-05-16 15:19:37]

Patchset #3 (id:60001) has been deleted

[mrefaat, 2017-05-16 15:19:42]

Patchset #2 (id:40001) has been deleted

[mrefaat, 2017-05-16 15:19:51]

Patchset #1 (id:20001) has been deleted

[mrefaat, 2017-05-16 15:29:18]

Description was changed from

==========
Create Bookmark Footnote desktop iOS promotion.

Create a footnote that promote for chrome on iOS on the bookmark bubble,
when the footnote link is clicked the desktop iOS promotion Bubble will appear.

Update BubbleFrameView to allow removing the footnote in the transition between
clicking the footnote link and showing the bookmark promotion bubble.

BUG=676655
==========

to

==========
Create Bookmark Footnote desktop iOS promotion.

Create a footnote that promote for chrome on iOS on the bookmark bubble,
when the footnote link is clicked the desktop iOS promotion Bubble will appear.

Introduce new entry point: FootnoteFollowupBubble this is used to distinguish in
logging between bubble used on the original bookmark promo and the bubble that
appear when link on footnote is clicked.

Update BubbleFrameView to allow removing the footnote in the transition between
clicking the footnote link and showing the bookmark promotion bubble.

example:
https://drive.google.com/a/google.com/file/d/0B9T1TKmsVG94YVZRUkJzdDltS2M/view
BUG=676655
==========

[mrefaat, 2017-05-16 15:34:31]

mrefaat@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2017-05-17 16:00:41]

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/bookmarks_st...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/bookmarks_st...
chrome/app/bookmarks_strings.grdp:248: To get your bookmarks on all your
devices, <ph name="GET_IOS_APP_LINK">$1<ex>get the application on your
iPhone</ex></ph>.
Why are we saying 'all your devices' here, when the promotion is only for the
iPhone? See line 245 where it says iPhone explicitly.

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1204: get Chromium on your iPhone
Are we really going with a lower case 'g' here?

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/google_chrom...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/google_chrom...
chrome/app/google_chrome_strings.grd:1214: get Chrome on your iPhone
Are we really going with a lower case 'g' here?

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:58:
// Update histograms.
Remove this comment as it just documents what the next line does and that next
line is rather obvious.

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:25:
class DesktopIOSPromotionController {
I'm having a hard time seeing why you want to share this class with the new
code. I say this because the two code paths, for the most part, seem completely
different. Why bother?

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:178: View*
footnote_view_;
AFAICT this it never used (other than to return it). Unless I'm missing
something you shouldn't need a member variable for this.

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_footnote_view.cc
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_footnote_view.cc:45:
promo_style.color = SkColorSetRGB(102, 102, 102);
Where do these colors come from?

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_footnote_view.h
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_footnote_view.h:1:
// Copyright (c) 2017 The Chromium Authors. All rights reserved.
no (c) (see style guide). Please check all new files for this.

https://codereview.chromium.org/2803293002/diff/80001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2803293002/diff/80001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:411: void
BubbleFrameView::RemoveFootnoteView() {
Instead of an explicit function like this, override ViewHierarchyChanged() and
if the view is the single child of footnote_container_ then delete
footnote_container_. If you do this, it means you can just delete the
footnote_view (or remove it from its parent), and the right thing will happen.
Add test coverage of this.

[mrefaat, 2017-05-17 19:44:57]

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/bookmarks_st...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/bookmarks_st...
chrome/app/bookmarks_strings.grdp:248: To get your bookmarks on all your
devices, <ph name="GET_IOS_APP_LINK">$1<ex>get the application on your
iPhone</ex></ph>.
On 2017/05/17 16:00:40, sky wrote:
> Why are we saying 'all your devices' here, when the promotion is only for the
> iPhone? See line 245 where it says iPhone explicitly.

I reached to the PM and they changed it to "your mobile device" now

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1204: get Chromium on your iPhone
On 2017/05/17 16:00:40, sky wrote:
> Are we really going with a lower case 'g' here?

yes

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/google_chrom...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/google_chrom...
chrome/app/google_chrome_strings.grd:1214: get Chrome on your iPhone
On 2017/05/17 16:00:40, sky wrote:
> Are we really going with a lower case 'g' here?

yes

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.cc:58:
// Update histograms.
On 2017/05/17 16:00:41, sky wrote:
> Remove this comment as it just documents what the next line does and that next
> line is rather obvious.

Done.

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:25:
class DesktopIOSPromotionController {
On 2017/05/17 16:00:41, sky wrote:
> I'm having a hard time seeing why you want to share this class with the new
> code. I say this because the two code paths, for the most part, seem
completely
> different. Why bother?
Both are promotions surfaces and need logging & and updating sync, the
difference is that footnote promotion first surface doesn't have send SMS
button, and the second surface shouldn't update Sync but it should log.

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:178: View*
footnote_view_;
On 2017/05/17 16:00:41, sky wrote:
> AFAICT this it never used (other than to return it). Unless I'm missing
> something you shouldn't need a member variable for this.

correct, forgot to remove it.

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_footnote_view.cc
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_footnote_view.cc:45:
promo_style.color = SkColorSetRGB(102, 102, 102);
On 2017/05/17 16:00:41, sky wrote:
> Where do these colors come from?

I'm using same colors used by the signing promotion footnote. ( i can move that
to anonymous namespace but not sure if this will make a difference)

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_footnote_view.h
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_footnote_view.h:1:
// Copyright (c) 2017 The Chromium Authors. All rights reserved.
On 2017/05/17 16:00:41, sky wrote:
> no (c) (see style guide). Please check all new files for this.

Done.

https://codereview.chromium.org/2803293002/diff/80001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2803293002/diff/80001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:411: void
BubbleFrameView::RemoveFootnoteView() {
On 2017/05/17 16:00:41, sky wrote:
> Instead of an explicit function like this, override ViewHierarchyChanged() and
> if the view is the single child of footnote_container_ then delete
> footnote_container_. If you do this, it means you can just delete the
> footnote_view (or remove it from its parent), and the right thing will happen.
> Add test coverage of this.
I remember that we talked about that and you said its also acceptable to have an
explicit function to do the footnote remove, is there an advantage for the
viewHierarchyChanged change over having explicit function to do that ?

[sky, 2017-05-18 02:55:39]

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/bookmarks_st...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/app/bookmarks_st...
chrome/app/bookmarks_strings.grdp:248: To get your bookmarks on all your
devices, <ph name="GET_IOS_APP_LINK">$1<ex>get the application on your
iPhone</ex></ph>.
On 2017/05/17 19:44:56, mrefaat wrote:
> On 2017/05/17 16:00:40, sky wrote:
> > Why are we saying 'all your devices' here, when the promotion is only for
the
> > iPhone? See line 245 where it says iPhone explicitly.
> 
> I reached to the PM and they changed it to "your mobile device" now

Ok. In that case should the others be updated as well?

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:25:
class DesktopIOSPromotionController {
On 2017/05/17 19:44:56, mrefaat wrote:
> On 2017/05/17 16:00:41, sky wrote:
> > I'm having a hard time seeing why you want to share this class with the new
> > code. I say this because the two code paths, for the most part, seem
> completely
> > different. Why bother?
> Both are promotions surfaces and need logging & and updating sync, the
> difference is that footnote promotion first surface doesn't have send SMS
> button, and the second surface shouldn't update Sync but it should log.
> 

How about refactoring the SMS code out so that this class only contains the
parts that are needed by both?

https://codereview.chromium.org/2803293002/diff/80001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2803293002/diff/80001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:411: void
BubbleFrameView::RemoveFootnoteView() {
On 2017/05/17 19:44:56, mrefaat wrote:
> On 2017/05/17 16:00:41, sky wrote:
> > Instead of an explicit function like this, override ViewHierarchyChanged()
and
> > if the view is the single child of footnote_container_ then delete
> > footnote_container_. If you do this, it means you can just delete the
> > footnote_view (or remove it from its parent), and the right thing will
happen.
> > Add test coverage of this.
> I remember that we talked about that and you said its also acceptable to have
an
> explicit function to do the footnote remove, is there an advantage for the
> viewHierarchyChanged change over having explicit function to do that ?

Sorry if I mislead you. It's a similar argument to why you don't have to remove
a view from its parent before deleting it. To do otherwise is error prone. It's
a common expectation that if you remove/delete the necessary cleanup is just
done.

[jasonkliu1, 2017-05-19 16:45:26]

On 2017/05/18 02:55:39, sky wrote:
>
https://codereview.chromium.org/2803293002/diff/80001/chrome/app/bookmarks_st...
> File chrome/app/bookmarks_strings.grdp (right):
> 
>
https://codereview.chromium.org/2803293002/diff/80001/chrome/app/bookmarks_st...
> chrome/app/bookmarks_strings.grdp:248: To get your bookmarks on all your
> devices, <ph name="GET_IOS_APP_LINK">$1<ex>get the application on your
> iPhone</ex></ph>.
> On 2017/05/17 19:44:56, mrefaat wrote:
> > On 2017/05/17 16:00:40, sky wrote:
> > > Why are we saying 'all your devices' here, when the promotion is only for
> the
> > > iPhone? See line 245 where it says iPhone explicitly.
> > 
> > I reached to the PM and they changed it to "your mobile device" now
> 
> Ok. In that case should the others be updated as well?

Our initial exploration of which strings to use (with srahim@) is here:
https://docs.google.com/document/d/1blFQYR5jcjzj_F9s43h7DSpMP3QkCN6fJrSpiQyx2...

The footnote text is unique in that when it's clicked, the user will see this
promo bubble:
https://drive.google.com/file/d/0B3dPCXKQYa2dQUoyZ1VIVzZyaG8/view?pli=1

In a separate iteration, the above bubble is shown immediately after clicking
'Done' when adding a bookmark.

I'm fine with the footnote text being unique in this case as we want to learn
if the footnote is useful to click on or if simply showing the promo after
clicking 'Done' is more effective.  Thanks!

[mrefaat1, 2017-05-23 22:07:44]

Patchset #5 (id:160001) has been deleted

[mrefaat1, 2017-05-23 22:10:49]

Patchset #4 (id:140001) has been deleted

[mrefaat1, 2017-05-23 22:15:33]

mrefaat@google.com changed reviewers:
+ mrefaat@google.com

[mrefaat1, 2017-05-23 22:15:34]

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:25:
class DesktopIOSPromotionController {
On 2017/05/18 02:55:38, sky wrote:
> On 2017/05/17 19:44:56, mrefaat wrote:
> > On 2017/05/17 16:00:41, sky wrote:
> > > I'm having a hard time seeing why you want to share this class with the
new
> > > code. I say this because the two code paths, for the most part, seem
> > completely
> > > different. Why bother?
> > Both are promotions surfaces and need logging & and updating sync, the
> > difference is that footnote promotion first surface doesn't have send SMS
> > button, and the second surface shouldn't update Sync but it should log.
> > 
> 
> How about refactoring the SMS code out so that this class only contains the
> parts that are needed by both?

Done.

https://codereview.chromium.org/2803293002/diff/80001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2803293002/diff/80001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:411: void
BubbleFrameView::RemoveFootnoteView() {
On 2017/05/18 02:55:38, sky wrote:
> On 2017/05/17 19:44:56, mrefaat wrote:
> > On 2017/05/17 16:00:41, sky wrote:
> > > Instead of an explicit function like this, override ViewHierarchyChanged()
> and
> > > if the view is the single child of footnote_container_ then delete
> > > footnote_container_. If you do this, it means you can just delete the
> > > footnote_view (or remove it from its parent), and the right thing will
> happen.
> > > Add test coverage of this.
> > I remember that we talked about that and you said its also acceptable to
have
> an
> > explicit function to do the footnote remove, is there an advantage for the
> > viewHierarchyChanged change over having explicit function to do that ?
> 
> Sorry if I mislead you. It's a similar argument to why you don't have to
remove
> a view from its parent before deleting it. To do otherwise is error prone.
It's
> a common expectation that if you remove/delete the necessary cleanup is just
> done.

Done.

[sky, 2017-05-24 00:03:35]

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
File
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_bubble_controller_unittest.cc:167:
EXPECT_EQ(true, local_state_->GetBoolean(
EXPECT_TRUE

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h
(right):

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:47:
protected:
Style guide says no protected members.

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
File
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_footnote_delegate.h
(right):

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_footnote_delegate.h:10:
virtual ~DesktopIOSPromotionFootnoteDelegate() {}
Move the destructor to the protected section.

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:437: if
(footnote_view_) {
You don't actually need this conditional (delete null does nothing).

https://codereview.chromium.org/2803293002/diff/180001/ui/views/bubble/bubble...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2803293002/diff/180001/ui/views/bubble/bubble...
ui/views/bubble/bubble_frame_view.cc:368: if (!details.is_add && details.parent
== footnote_container_) {
Combine these two ifs into 1. Also, add a comment as to why it's safe to null
out footnote_container_.

[mrefaat1, 2017-05-24 02:27:17]

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
File
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_bubble_controller_unittest.cc:167:
EXPECT_EQ(true, local_state_->GetBoolean(
On 2017/05/24 00:03:35, sky wrote:
> EXPECT_TRUE

Done.

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h
(right):

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:47:
protected:
On 2017/05/24 00:03:35, sky wrote:
> Style guide says no protected members.

Done.

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:437: if
(footnote_view_) {
On 2017/05/24 00:03:35, sky wrote:
> You don't actually need this conditional (delete null does nothing).

Done.

https://codereview.chromium.org/2803293002/diff/180001/ui/views/bubble/bubble...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2803293002/diff/180001/ui/views/bubble/bubble...
ui/views/bubble/bubble_frame_view.cc:368: if (!details.is_add && details.parent
== footnote_container_) {
On 2017/05/24 00:03:35, sky wrote:
> Combine these two ifs into 1. Also, add a comment as to why it's safe to null
> out footnote_container_.

Done.

[sky, 2017-05-24 16:49:54]

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h
(right):

https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:47:
protected:
On 2017/05/24 02:27:17, mrefaat1 wrote:
> On 2017/05/24 00:03:35, sky wrote:
> > Style guide says no protected members.
> 
> Done.

Now you have all the members public.

[mrefaat, 2017-05-24 19:04:13]

On 2017/05/24 16:49:54, sky wrote:
>
https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
> File
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h
> (right):
> 
>
https://codereview.chromium.org/2803293002/diff/180001/chrome/browser/ui/desk...
> chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:47:
> protected:
> On 2017/05/24 02:27:17, mrefaat1 wrote:
> > On 2017/05/24 00:03:35, sky wrote:
> > > Style guide says no protected members.
> > 
> > Done.
> 
> Now you have all the members public.

created accessors

[mrefaat, 2017-05-24 19:04:19]

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h
(right):

https://codereview.chromium.org/2803293002/diff/80001/chrome/browser/ui/deskt...
chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_controller.h:25:
class DesktopIOSPromotionController {
On 2017/05/18 02:55:38, sky wrote:
> On 2017/05/17 19:44:56, mrefaat wrote:
> > On 2017/05/17 16:00:41, sky wrote:
> > > I'm having a hard time seeing why you want to share this class with the
new
> > > code. I say this because the two code paths, for the most part, seem
> > completely
> > > different. Why bother?
> > Both are promotions surfaces and need logging & and updating sync, the
> > difference is that footnote promotion first surface doesn't have send SMS
> > button, and the second surface shouldn't update Sync but it should log.
> > 
> 
> How about refactoring the SMS code out so that this class only contains the
> parts that are needed by both?

Done.

[sky, 2017-05-24 21:48:11]

LGTM

[mrefaat1, 2017-05-24 22:09:45]

The CQ bit was checked by mrefaat@google.com

[commit-bot: I haz the power, 2017-05-24 22:11:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 22:24:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 22:24:31]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mrefaat, 2017-05-24 22:32:53]

The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 22:33:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 23:19:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 23:19:14]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mrefaat, 2017-05-25 01:46:02]

The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 01:47:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 03:40:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 03:40:14]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mrefaat1, 2017-05-25 05:39:15]

Patchset #7 (id:240001) has been deleted

[mrefaat1, 2017-05-25 05:39:27]

The CQ bit was checked by mrefaat@google.com

[mrefaat1, 2017-05-25 05:39:27]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2803293002/#ps280001
(title: "win only")

[commit-bot: I haz the power, 2017-05-25 05:39:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 07:03:21]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1495690767029470,
"parent_rev": "a03358b4752b6ca9ee2c92c9dc20aa658bb1a250", "commit_rev":
"862a261b9f735eabd3b17b08fe4e7e3873c8c14e"}

[commit-bot: I haz the power, 2017-05-25 07:03:35]

Description was changed from

==========
Create Bookmark Footnote desktop iOS promotion.

Create a footnote that promote for chrome on iOS on the bookmark bubble,
when the footnote link is clicked the desktop iOS promotion Bubble will appear.

Introduce new entry point: FootnoteFollowupBubble this is used to distinguish in
logging between bubble used on the original bookmark promo and the bubble that
appear when link on footnote is clicked.

Update BubbleFrameView to allow removing the footnote in the transition between
clicking the footnote link and showing the bookmark promotion bubble.

example:
https://drive.google.com/a/google.com/file/d/0B9T1TKmsVG94YVZRUkJzdDltS2M/view
BUG=676655
==========

to

==========
Create Bookmark Footnote desktop iOS promotion.

Create a footnote that promote for chrome on iOS on the bookmark bubble,
when the footnote link is clicked the desktop iOS promotion Bubble will appear.

Introduce new entry point: FootnoteFollowupBubble this is used to distinguish in
logging between bubble used on the original bookmark promo and the bubble that
appear when link on footnote is clicked.

Update BubbleFrameView to allow removing the footnote in the transition between
clicking the footnote link and showing the bookmark promotion bubble.

example:
https://drive.google.com/a/google.com/file/d/0B9T1TKmsVG94YVZRUkJzdDltS2M/view
Review-Url: https://codereview.chromium.org/2803293002
Cr-Commit-Position: refs/heads/master@{#474593}
Committed:
https://chromium.googlesource.com/chromium/src/+/862a261b9f735eabd3b17b08fe4e...
==========

[commit-bot: I haz the power, 2017-05-25 07:03:36]

Committed patchset #8 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/862a261b9f735eabd3b17b08fe4e...

[mrefaat1, 2017-05-25 17:35:55]

Patchset #7 (id:260001) has been deleted