Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781553003:
Integrate Desktop iOS promotion with bookmarks. (Closed)
DescriptionIntegrate Desktop iOS promotion with bookmarks.
1- Show the iOS promotion to eligible users when they create a bookmark and then
click done on the bookmark bubble.
2- Refactor Bubble Text strings arrays.
Bug=676655
Test= (1)Launch Chrome with --force-desktop-ios-promotion switch, (2)bookmark
any website, (3)click Done, (4)The promotion should appear.
Review-Url: https://codereview.chromium.org/2781553003
Cr-Commit-Position: refs/heads/master@{#460943}
Committed: https://chromium.googlesource.com/chromium/src/+/633b3659cb47272c5d71731cb66b3583df3bc947
Patch Set 1 : integrate to bookmarks #
Total comments: 16
Patch Set 2 : Add Browser test and address comments #
Total comments: 15
Patch Set 3 : address comments #Patch Set 4 : merge #
Total comments: 8
Patch Set 5 : address comments 3 #
Messages
Total messages: 38 (28 generated)
Description was changed from ========== Integrate Desktop iOS promotion with bookmarks. Show the iOS promotion to eligible users when they create a bookmark and then click done on the bookmark bubble. Bug=676655 Test= (1)Launch Chrome with --force-desktop-ios-promotion switch, (2)bookmark any website, (3)click Done, (4)The promotion should appear. ========== to ========== Integrate Desktop iOS promotion with bookmarks. 1- Show the iOS promotion to eligible users when they create a bookmark and then click done on the bookmark bubble. 2- Refactor Bubble Text strings arrays. Bug=676655 Test= (1)Launch Chrome with --force-desktop-ios-promotion switch, (2)bookmark any website, (3)click Done, (4)The promotion should appear. ==========
Patchset #1 (id:1) has been deleted
mrefaat@chromium.org changed reviewers: + sky@chromium.org
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please add test coverage using https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/testin... . https://codereview.chromium.org/2781553003/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2781553003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:6060: + <!-- Desktop to iOS promotion --> Did you place this in a windows only section? https://codereview.chromium.org/2781553003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:6070: + <message name="IDS_BOOKMARK_BUBBLE_DESKTOP_TO_IOS_PROMO_TITLE" desc="Title for Chrome iOS promotion appearing in the bookmark bubble after a bookmark is saved."> These should be grouped with the other bookmark bubble strings. https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:150: kBubbleTitleTextId[static_cast<int>(entry_point) - 1] Don't hardcode the -1, use a constant. And by that I mean defined near the enum so it's obvious where it comes from. https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:353: apply_edits_ = false; Why are you resetting apply_edits_? https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:364: void BookmarkBubbleView::ShowIOSPromotion() { Make order match header. https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:365: RemoveAllChildViews(true); Explicitly removing all the views is error prone (see the conditional you're adding to the destructor). I would prefer you add the existing controls to a View that is hidden and make the ios_promo_view_ visible as necessary. https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right): https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:153: DesktopIOSPromotionBubbleView* ios_promo_view_; This is never initialized. I recommend initializing right here, same with ios_promotion_views_. Less error prone. https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:162: bool ios_promotion_viewed_; I think you want is_showing_ios_promotion_?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2781553003/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2781553003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:6060: + <!-- Desktop to iOS promotion --> On 2017/03/27 20:03:55, sky wrote: > Did you place this in a windows only section? Done. https://codereview.chromium.org/2781553003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:6070: + <message name="IDS_BOOKMARK_BUBBLE_DESKTOP_TO_IOS_PROMO_TITLE" desc="Title for Chrome iOS promotion appearing in the bookmark bubble after a bookmark is saved."> On 2017/03/27 20:03:55, sky wrote: > These should be grouped with the other bookmark bubble strings. Done. https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:150: kBubbleTitleTextId[static_cast<int>(entry_point) - 1] On 2017/03/27 20:03:55, sky wrote: > Don't hardcode the -1, use a constant. And by that I mean defined near the enum > so it's obvious where it comes from. used padding instead to keep the consistency https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:353: apply_edits_ = false; On 2017/03/27 20:03:55, sky wrote: > Why are you resetting apply_edits_? changed that https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:364: void BookmarkBubbleView::ShowIOSPromotion() { On 2017/03/27 20:03:55, sky wrote: > Make order match header. Done. https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:365: RemoveAllChildViews(true); On 2017/03/27 20:03:55, sky wrote: > Explicitly removing all the views is error prone (see the conditional you're > adding to the destructor). I would prefer you add the existing controls to a > View that is hidden and make the ios_promo_view_ visible as necessary. Done. https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right): https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:153: DesktopIOSPromotionBubbleView* ios_promo_view_; On 2017/03/27 20:03:55, sky wrote: > This is never initialized. I recommend initializing right here, same with > ios_promotion_views_. Less error prone. i added initialization on the constructor for consistency. https://codereview.chromium.org/2781553003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:162: bool ios_promotion_viewed_; On 2017/03/27 20:03:55, sky wrote: > I think you want is_showing_ios_promotion_? Done.
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc (right): https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) (see style guide) https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. Name this file bookmark_bubble_view_browsertest to match the class under test. https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:25: class BookmarkBubbleBrowserTest : public DialogBrowserTest { And name this to match bookmarkbubbleview too. https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:36: BookmarkModelFactory::GetForBrowserContext(profile()); You use profile_ above, and profile() here. Please use profile_ everywhere and remove profile() as there isn't a real need for it. https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:47: BrowserView* browser_view = GetBrowserViewForBrowser(browser()). https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:76: std::unique_ptr<TestingProfile> profile_; Move to private_ ? Tests allow for protected members, but it doesn't seem that you need it to be protected. https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right): https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:157: View* bookmark_details_view_; I believe you leak this.
Patchset #2 (id:80001) has been deleted
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc (right): https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/29 21:54:36, sky wrote: > no (c) (see style guide) Ack, But we need to change the presubmit rule, i got presubmit warning about that i should match .*? Copyright (\(c\) )?(2017|2016|2015|2014|2013|2012|2011|2010|2009|2008|2007|2006|2006-2008|2006-2009|2006-2010) The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LIC ENSE file\.(?: \*/)?\n https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/29 21:54:34, sky wrote: > Name this file bookmark_bubble_view_browsertest to match the class under test. Done. https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:25: class BookmarkBubbleBrowserTest : public DialogBrowserTest { On 2017/03/29 21:54:35, sky wrote: > And name this to match bookmarkbubbleview too. Done. https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:36: BookmarkModelFactory::GetForBrowserContext(profile()); On 2017/03/29 21:54:37, sky wrote: > You use profile_ above, and profile() here. Please use profile_ everywhere and > remove profile() as there isn't a real need for it. i thought its better than using profile_.get() everywhere - removed it. https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:47: BrowserView* browser_view = On 2017/03/29 21:54:34, sky wrote: > GetBrowserViewForBrowser(browser()). Done. https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:76: std::unique_ptr<TestingProfile> profile_; On 2017/03/29 21:54:34, sky wrote: > Move to private_ ? Tests allow for protected members, but it doesn't seem that > you need it to be protected. Done. https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right): https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:157: View* bookmark_details_view_; On 2017/03/29 21:54:37, sky wrote: > I believe you leak this. Done.
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc (right): https://codereview.chromium.org/2781553003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_browsertest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/30 17:37:15, mrefaat wrote: > On 2017/03/29 21:54:36, sky wrote: > > no (c) (see style guide) > > Ack, But we need to change the presubmit rule, i got presubmit warning about > that i should match > .*? Copyright (\(c\) > )?(2017|2016|2015|2014|2013|2012|2011|2010|2009|2008|2007|2006|2006-2008|2006-2009|2006-2010) > The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is > governed by a BSD-style license that can be\n.*? found in the LIC > ENSE file\.(?: \*/)?\n You must be doing something else wrong if you need the (c). See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md , which says "There is no (c) after Copyright.". https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:181: bookmark_details_view_.reset(new View); Use base::MakeUnique (search for threads on chromium-dev if curious). https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:302: bookmark_details_view_(nullptr), Remove this as bookmark_details_view_ is now a unique_ptr. https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:406: RemoveChildView(bookmark_details_view_.get()); DCHECK(!is_showing_ios_promotion_) https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:411: GetWidget()->non_client_view()->ResetWindowControls(); Why do you need the ResetWindowControls()?
https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:181: bookmark_details_view_.reset(new View); On 2017/03/30 20:30:17, sky wrote: > Use base::MakeUnique (search for threads on chromium-dev if curious). Done. https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:302: bookmark_details_view_(nullptr), On 2017/03/30 20:30:17, sky wrote: > Remove this as bookmark_details_view_ is now a unique_ptr. Done. https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:406: RemoveChildView(bookmark_details_view_.get()); On 2017/03/30 20:30:17, sky wrote: > DCHECK(!is_showing_ios_promotion_) Done. https://codereview.chromium.org/2781553003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:411: GetWidget()->non_client_view()->ResetWindowControls(); On 2017/03/30 20:30:17, sky wrote: > Why do you need the ResetWindowControls()? i thought because is use the control (Done) and the bubble is resized i may need that but checked now and i probably don't
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mrefaat@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490917328550800,
"parent_rev": "589d05ed666fd48defd3cd338e8e23a2bcc21937", "commit_rev":
"633b3659cb47272c5d71731cb66b3583df3bc947"}
Message was sent while issue was closed.
Description was changed from ========== Integrate Desktop iOS promotion with bookmarks. 1- Show the iOS promotion to eligible users when they create a bookmark and then click done on the bookmark bubble. 2- Refactor Bubble Text strings arrays. Bug=676655 Test= (1)Launch Chrome with --force-desktop-ios-promotion switch, (2)bookmark any website, (3)click Done, (4)The promotion should appear. ========== to ========== Integrate Desktop iOS promotion with bookmarks. 1- Show the iOS promotion to eligible users when they create a bookmark and then click done on the bookmark bubble. 2- Refactor Bubble Text strings arrays. Bug=676655 Test= (1)Launch Chrome with --force-desktop-ios-promotion switch, (2)bookmark any website, (3)click Done, (4)The promotion should appear. Review-Url: https://codereview.chromium.org/2781553003 Cr-Commit-Position: refs/heads/master@{#460943} Committed: https://chromium.googlesource.com/chromium/src/+/633b3659cb47272c5d71731cb66b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/633b3659cb47272c5d71731cb66b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
