Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2134293002:
Adding a ScrollView for IntentPickerBubbleView (Closed)
DescriptionAdding a ScrollView for IntentPickerBubbleView
Adding a ScrollView so we can handle cases when ARC has more that 3 app
candidates to handle a given URL. We are also marking the app selected
by the user by changing the background color.
Notice that we are displaying 3.5 rows when the app candidates exceed 3,
this is the intended behavior.
Removing EventMonitor and algorithm as these are not used anymore.
BUG=620129
Committed: https://crrev.com/8c141a8b213ec8e0ba570820c0a7abf7ba38d28b
Cr-Commit-Position: refs/heads/master@{#407663}
Patch Set 1 #Patch Set 2 : Adding unit tests, adapting the IntentPickerBubbleView constructor so we can avoid using the thrott… #Patch Set 3 : Fixing test cases #Patch Set 4 : Initializing the app's icon variable. #Patch Set 5 : Using a gfx::ImageSkia() as default for the LabelButton. #Patch Set 6 : Adding a test for NonNullIcons #Patch Set 7 : Ensuring bubble_ is destroyed first, by reseting. #Patch Set 8 : Making use of the browser() and profile() included in the parent class. #Patch Set 9 : Fix the last test failing, moving the order of excecution and getting rid of the override SetUp(). #
Total comments: 34
Patch Set 10 : Moving from friend to ForTesting() methods #
Total comments: 13
Patch Set 11 : Changing the way we access the LabelButtons* within the Bubble, misc fixes #Patch Set 12 : Style fixes *shame cube* #Patch Set 13 : Reutilize code for recovering a label ForTesting(). #Patch Set 14 : Adding const for GetAppInfoSizeForTesting() #
Total comments: 43
Patch Set 15 : Making ScrollView a memeber of the picker's class so his methods can access his LabelButtons* easie… #
Total comments: 4
Patch Set 16 : Removing ForTesting() methods and making friendship great again! #Patch Set 17 : Code style misc fixes #Patch Set 18 : Exposing a static method to create an intent picker bubble (Init'd but not displayed thru a widget). #Patch Set 19 : Using base::Unretained with base::Bind() #
Total comments: 8
Patch Set 20 : Printing failing test's index. #Patch Set 21 : Removing index checks #Patch Set 22 : Adding a DCHECK to make code easier to follow. #
Messages
Total messages: 80 (56 generated)
Description was changed from ========== Adding a ScrollView for IntentPickerBubbleView Adding a ScrollView so we can handle cases when ARC has more that 3 app candidates to handle a given URL. We are also marking the app selected by the user by changing the background color. Notice that we are displaying 3.5 rows when the app candidates exceed 3, this is the intended behavior. Removing EventMonitor and algorithm as these are not used anymore. BUG=620129 ========== to ========== Adding a ScrollView for IntentPickerBubbleView Adding a ScrollView so we can handle cases when ARC has more that 3 app candidates to handle a given URL. We are also marking the app selected by the user by changing the background color. Notice that we are displaying 3.5 rows when the app candidates exceed 3, this is the intended behavior. Removing EventMonitor and algorithm as these are not used anymore. BUG=620129 ==========
djacobo@google.com changed reviewers: + yusukes@chromium.org
https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:185: scroll_view->ClipHeightTo(kMaxWidth, (kMaxAppResults + 0.5) * kRowHeight); Can you add a code comment to document why we need '+ 0.5'? https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:218: app_label_ptr_.clear(); std::vector is initially empty. please remove. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:244: if (!was_callback_run_) { What's wrong with running a cb for the unit tests? Can't the test define an empty callback function? https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:72: friend class IntentPickerBubbleViewTest; Another way to allow the test to access internals of this class is to add a public function with _for_testing() or ForTesting() suffix. public: const std::vector<views::LabelButton*>& app_label_ptr_for_testing() const; // or size_t GetNumAppLablesForTesting() const; This allows you to control what test files can see. If you call such a method in non-test .cc file, trybot fails. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:108: std::vector<views::LabelButton*> app_label_ptr_; Is this really necessary? I'm not familiar with Views at all but ui/views/view.h has set_id() and GetViewById(). Holding non-weak and non-owning pointers looks a bit scary to me. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:9: #include "build/build_config.h" nit: what is this for? https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:12: #include "content/public/browser/navigation_handle.h" Is this necessary? https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:18: #include "ui/views/resources/grit/views_resources.h" same https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:28: IntentPickerBubbleViewTest() {} = default; https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:38: void CreateBubbleView(bool use_custom_icons) { use_icons might be better? I wasn't sure what 'custom' meant here. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:47: WebContents* web_contents = browser()->OpenURL(OpenURLParams( Why is it necessary to open a page here? Can you add a code comment? https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:55: app_info_, dummy_arc_navigation_throttle_cb, web_contents, true)); true /* was_callback_run */ https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:77: for (auto app : bubble_->app_label_ptr_) { s/app/label/ probably? https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:87: for (auto app : bubble_->app_label_ptr_) { same https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:98: EXPECT_TRUE(app_info_.size() == bubble_->app_label_ptr_.size()); EXPECT_EQ(app_info_.size(), bubble_->app_label_ptr_.size()); This way, when the test failed, you can see both values.
https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:30: constexpr size_t kMaxAppResults = 3; Please rebase this CL and use the throttle's. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:73: FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, NullIcons); Or, if you go with friend, can you remove line 73-75? Adding one FRIEND_TEST_ALL_PREFIXES per TEST_F seems too noisy to me.
The CQ bit was checked by djacobo@google.com 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/2134293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:30: constexpr size_t kMaxAppResults = 3; On 2016/07/20 08:05:54, Yusuke Sato (ooo Jul 21-31) wrote: > Please rebase this CL and use the throttle's. Done. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:185: scroll_view->ClipHeightTo(kMaxWidth, (kMaxAppResults + 0.5) * kRowHeight); On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > Can you add a code comment to document why we need '+ 0.5'? Done. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:218: app_label_ptr_.clear(); On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > std::vector is initially empty. please remove. Done. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:244: if (!was_callback_run_) { On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > What's wrong with running a cb for the unit tests? Can't the test define an > empty callback function? Yes, I will have a dummy method for receiving this call. Done https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:72: friend class IntentPickerBubbleViewTest; On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > Another way to allow the test to access internals of this class is to add a > public function with _for_testing() or ForTesting() suffix. > > public: > const std::vector<views::LabelButton*>& app_label_ptr_for_testing() const; > > // or > size_t GetNumAppLablesForTesting() const; > > This allows you to control what test files can see. If you call such a method in > non-test .cc file, trybot fails. > Done. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:73: FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, NullIcons); On 2016/07/20 08:05:54, Yusuke Sato (ooo Jul 21-31) wrote: > Or, if you go with friend, can you remove line 73-75? Adding one > FRIEND_TEST_ALL_PREFIXES per TEST_F seems too noisy to me. Done. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:108: std::vector<views::LabelButton*> app_label_ptr_; On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > Is this really necessary? I'm not familiar with Views at all but ui/views/view.h > has set_id() and GetViewById(). > > Holding non-weak and non-owning pointers looks a bit scary to me. Removed, opting for the GetViewByID() method you suggested, thanks! https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:9: #include "build/build_config.h" On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > nit: what is this for? My bad, forgot to delete it from previous versions. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:12: #include "content/public/browser/navigation_handle.h" On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > Is this necessary? A previous version was using it, removed. Done https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:18: #include "ui/views/resources/grit/views_resources.h" On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > same My understanding is that this is for the IDR_CLOSE image used for the dummy icons. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:28: IntentPickerBubbleViewTest() {} On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > = default; I will need a different constructor since I am using weak_ptr now for Bind'ing. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:38: void CreateBubbleView(bool use_custom_icons) { On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > use_icons might be better? I wasn't sure what 'custom' meant here. Done. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:47: WebContents* web_contents = browser()->OpenURL(OpenURLParams( On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > Why is it necessary to open a page here? Can you add a code comment? The bubble class need it since the bubble's lifetime is tied to the WebContents' lifetime. At least that's my understanding on the WebContentsObserver. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:55: app_info_, dummy_arc_navigation_throttle_cb, web_contents, true)); On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > true /* was_callback_run */ Done. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:77: for (auto app : bubble_->app_label_ptr_) { On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > s/app/label/ probably? Changing this part, now we retrieve View* with GetViewByID() https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:87: for (auto app : bubble_->app_label_ptr_) { On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > same Done. https://codereview.chromium.org/2134293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:98: EXPECT_TRUE(app_info_.size() == bubble_->app_label_ptr_.size()); On 2016/07/19 10:23:58, Yusuke Sato (ooo Jul 21-31) wrote: > EXPECT_EQ(app_info_.size(), bubble_->app_label_ptr_.size()); > > This way, when the test failed, you can see both values. Done.
djacobo@google.com changed reviewers: + lhchavez@chromium.org
lgtm with comments: https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:103: new IntentPickerBubbleView(app_info, throttle_cb, web_contents)); auto bubble = base::MakeUnique<...>(...); ? It's in base/memory/ptr_util.h. https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:186: tmp_label->set_id(i + 1); These IDs conflict with the ones in chrome/browser/ui/view_ids.h? I'm not sure if this is okay. Please check what other code does. https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:69: size_t GetNumberOfAppLabelsForTesting(); * optional: feel free to rename this to a shorter one like GetAppInfoSizeForTesting(). * () const; ? https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:70: ~IntentPickerBubbleView() override; move to line 61? https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:68: arc::ArcNavigationThrottle::CloseReason close_reason) {} optional: Is it possible to test whether or not this method is called?
drive-by https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:46: constexpr size_t kAppTagNoneSelected = -1; size_t is unsigned. Maybe ssize_t?
The CQ bit was checked by djacobo@google.com 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 checked by djacobo@google.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:46: constexpr size_t kAppTagNoneSelected = -1; On 2016/07/20 22:16:38, Luis Héctor Chávez wrote: > size_t is unsigned. Maybe ssize_t? This variable is used together with the enum CloseReason; CloseReason let the caller know about the status when the UI finish his execution, so this value is rather a dummy, let me know what you think. https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:103: new IntentPickerBubbleView(app_info, throttle_cb, web_contents)); On 2016/07/20 21:58:57, Yusuke Sato (ooo Jul 21-31) wrote: > auto bubble = base::MakeUnique<...>(...); ? > > It's in base/memory/ptr_util.h. by using base::MakeUnique<...>(...); que compiler ask me to make the constructor public, I would rather not to, what do you think? https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:186: tmp_label->set_id(i + 1); On 2016/07/20 21:58:57, Yusuke Sato (ooo Jul 21-31) wrote: > These IDs conflict with the ones in chrome/browser/ui/view_ids.h? I'm not sure > if this is okay. Please check what other code does. > On the most parts of the codebase that I watched the use is only for well known IDs like the ones you mention (on views_ids.h), I made some modifications to use child_at() instead, it's a bit messy but I separated that on a pair of functions (take a look at ButtonPressed(). Done https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:69: size_t GetNumberOfAppLabelsForTesting(); On 2016/07/20 21:58:57, Yusuke Sato (ooo Jul 21-31) wrote: > * optional: feel free to rename this to a shorter one like > GetAppInfoSizeForTesting(). > * () const; ? Done. https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:70: ~IntentPickerBubbleView() override; On 2016/07/20 21:58:57, Yusuke Sato (ooo Jul 21-31) wrote: > move to line 61? Done. https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:68: arc::ArcNavigationThrottle::CloseReason close_reason) {} On 2016/07/20 21:58:57, Yusuke Sato (ooo Jul 21-31) wrote: > optional: Is it possible to test whether or not this method is called? I believe so, let me defer it while I give it a good guess.
The CQ bit was checked by djacobo@google.com 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: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
djacobo@google.com changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:103: new IntentPickerBubbleView(app_info, throttle_cb, web_contents)); On 2016/07/21 16:04:11, djacobo wrote: > On 2016/07/20 21:58:57, Yusuke Sato (ooo Jul 21-31) wrote: > > auto bubble = base::MakeUnique<...>(...); ? > > > > It's in base/memory/ptr_util.h. > > by using base::MakeUnique<...>(...); que compiler ask me to make the constructor > public, I would rather not to, what do you think? Another option is to make base::MakeUnique<IntentPickerBubbleView> a friend, but you're probably right in that this is maybe not worth it. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:46: constexpr size_t kAppTagNoneSelected = -1; ok, just do an explicit cast: static_cast<size_t>(-1); https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:323: int scroll_index = static_cast<int>(ViewsId::SCROLL_VIEW); Having the |selected_app_tag_| be a huge value is kind of scary. Can you add bounds-checks in all the places it might be used (either directly or indirectly)? If you end up returning nullptr, then you also need nullchecks in all the places where you call this.
https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_navigation_throttle.h:44: enum { kMaxAppResults = 3 }; enum style is ALL_CAPS (like CloseReason above). That said, why are using an enum here at all? https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:147: // Insets in the format top, left, bottom, right. This comment isn't particularly useful when all the values are the same. You can also use CreateEmptytBorder(gfx::Insets(16)) https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:166: // Adding the |open_with| label to the picker. Is this comment relevant here? https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:168: AddChildViewAt(header, static_cast<int>(ViewsId::HEADER)); Based on the name I would expect ViewId to correspond to the View::id(), but that isn't the case. It's the index of the view in the parent. That's confusing. AFAICT the reason you're doing that is because you want to get a specific child on line 323, which even with the enums is error prone. Instead make scroll_view a member and get rid of ViewId. Much simpler that way. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:170: // Setting a view and a layout for a ScrollView for the apps' list. This comment is confusing. I think you mean something like: // Creates a view to hold the views for each app. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:194: scroll_view->SetVerticalScrollBar(new views::OverlayScrollBar(false)); Is it really necessary to create an overlayscrollbar? Assuming yes, add a comment. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:197: // The added 0.5 on the else block allow us to let the user know there are I don't understand the *.5. Why not just add 1 as you want 1 extra pixel, right? https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:200: scroll_view->ClipHeightTo(kMaxWidth, app_info_.size() * kRowHeight); The first argument is the min height, not a width. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:267: if (!was_callback_run_) { Why do you need was_callback_run_? https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:53: enum class ViewsId : int { Move to private section, and add a comment. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:71: static std::unique_ptr<IntentPickerBubbleView> CreateBubbleForTesting( You're exposing 3 methods purely for testing. How about friending the test instead? https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:100: views::LabelButton* GetLabelButtonAt(size_t index); Add description. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:101: void PaintLabelButton(size_t index, SkColor color); Based on the name I thought this was called during painting to paint the label. That isn't the case. It resets the border. Please rename to make it more obvious, perhaps SetLabelButtonBackgroundColor. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:75: base::WeakPtrFactory<IntentPickerBubbleViewTest> weak_ptr_factory_; Do you really need the ptr factory here?
https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_navigation_throttle.h:44: enum { kMaxAppResults = 3 }; On 2016/07/21 22:57:06, sky wrote: > enum style is ALL_CAPS (like CloseReason above). That said, why are using an > enum here at all? https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/hDqJ4KBVqog...
Ugh. Ok. On Thu, Jul 21, 2016 at 3:59 PM, <lhchavez@chromium.org> wrote: > > https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right): > > https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_navigation_throttle.h:44: enum { > kMaxAppResults = 3 }; > On 2016/07/21 22:57:06, sky wrote: >> enum style is ALL_CAPS (like CloseReason above). That said, why are > using an >> enum here at all? > > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/hDqJ4KBVqog... > > https://codereview.chromium.org/2134293002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by djacobo@google.com 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/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:46: constexpr size_t kAppTagNoneSelected = -1; On 2016/07/21 21:55:34, Luis Héctor Chávez wrote: > ok, just do an explicit cast: static_cast<size_t>(-1); Done. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:147: // Insets in the format top, left, bottom, right. On 2016/07/21 22:57:06, sky wrote: > This comment isn't particularly useful when all the values are the same. You can > also use CreateEmptytBorder(gfx::Insets(16)) Nice shorthand, applied the same to |just_once_button_|. Done https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:166: // Adding the |open_with| label to the picker. On 2016/07/21 22:57:06, sky wrote: > Is this comment relevant here? I believe is easy to see what's going on by the code. Removing. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:168: AddChildViewAt(header, static_cast<int>(ViewsId::HEADER)); On 2016/07/21 22:57:06, sky wrote: > Based on the name I would expect ViewId to correspond to the View::id(), but > that isn't the case. It's the index of the view in the parent. That's confusing. > AFAICT the reason you're doing that is because you want to get a specific child > on line 323, which even with the enums is error prone. Instead make scroll_view > a member and get rid of ViewId. Much simpler that way. You are right and the idea sounds better, modifications in place, thanks! https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:170: // Setting a view and a layout for a ScrollView for the apps' list. On 2016/07/21 22:57:06, sky wrote: > This comment is confusing. I think you mean something like: > // Creates a view to hold the views for each app. Oh wow I messed up pretty badly indeed, Done. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:194: scroll_view->SetVerticalScrollBar(new views::OverlayScrollBar(false)); On 2016/07/21 22:57:06, sky wrote: > Is it really necessary to create an overlayscrollbar? Assuming yes, add a > comment. Yes, I want the ScrollBar for this bubble to be shown only when the user set his pointer inside of the ScrollView boundaries. Adding a comment. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:197: // The added 0.5 on the else block allow us to let the user know there are On 2016/07/21 22:57:06, sky wrote: > I don't understand the *.5. Why not just add 1 as you want 1 extra pixel, right? What I was trying to say is, we are required to show *half a row* extra, that's why we have, height = (kMaxAppResults + 0.5) * kRowHeight; Thus is not simply 1 extra pixel, but half a row will be displayed, which is intended to let the user know more apps are listed by scrolling. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:200: scroll_view->ClipHeightTo(kMaxWidth, app_info_.size() * kRowHeight); On 2016/07/21 22:57:06, sky wrote: > The first argument is the min height, not a width. Totally misread the interface, moving to 1 Row at min. Done. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:267: if (!was_callback_run_) { On 2016/07/21 22:57:06, sky wrote: > Why do you need was_callback_run_? I don't want the callback being run twice since that could end in a bad state. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:323: int scroll_index = static_cast<int>(ViewsId::SCROLL_VIEW); On 2016/07/21 21:55:34, Luis Héctor Chávez wrote: > Having the |selected_app_tag_| be a huge value is kind of scary. Can you add > bounds-checks in all the places it might be used (either directly or > indirectly)? > > If you end up returning nullptr, then you also need nullchecks in all the places > where you call this. Done. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:53: enum class ViewsId : int { On 2016/07/21 22:57:06, sky wrote: > Move to private section, and add a comment. Done. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:71: static std::unique_ptr<IntentPickerBubbleView> CreateBubbleForTesting( On 2016/07/21 22:57:06, sky wrote: > You're exposing 3 methods purely for testing. How about friending the test > instead? We already did that on a previous version but opted for this way (with ForTesting() methods) in order to keep a better separation of the class to test and the tester, let me know what you think. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:100: views::LabelButton* GetLabelButtonAt(size_t index); On 2016/07/21 22:57:06, sky wrote: > Add description. Done. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:101: void PaintLabelButton(size_t index, SkColor color); On 2016/07/21 22:57:06, sky wrote: > Based on the name I thought this was called during painting to paint the label. > That isn't the case. It resets the border. Please rename to make it more > obvious, perhaps SetLabelButtonBackgroundColor. Much better, thanks!. Done https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:75: base::WeakPtrFactory<IntentPickerBubbleViewTest> weak_ptr_factory_; On 2016/07/21 22:57:06, sky wrote: > Do you really need the ptr factory here? I think so. The idea was to use it with the Bind in line 53.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:197: // The added 0.5 on the else block allow us to let the user know there are On 2016/07/22 01:22:07, djacobo wrote: > On 2016/07/21 22:57:06, sky wrote: > > I don't understand the *.5. Why not just add 1 as you want 1 extra pixel, > right? > > What I was trying to say is, we are required to show *half a row* extra, that's > why we have, height = (kMaxAppResults + 0.5) * kRowHeight; > > Thus is not simply 1 extra pixel, but half a row will be displayed, which is > intended to let the user know more apps are listed by scrolling. My mistake, got it. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:267: if (!was_callback_run_) { On 2016/07/22 01:22:07, djacobo wrote: > On 2016/07/21 22:57:06, sky wrote: > > Why do you need was_callback_run_? > > I don't want the callback being run twice since that could end in a bad state. How can that happen? AFAICT the widget is closed on the next line? https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:71: static std::unique_ptr<IntentPickerBubbleView> CreateBubbleForTesting( On 2016/07/22 01:22:07, djacobo wrote: > On 2016/07/21 22:57:06, sky wrote: > > You're exposing 3 methods purely for testing. How about friending the test > > instead? > > We already did that on a previous version but opted for this way (with > ForTesting() methods) in order to keep a better separation of the class to test > and the tester, let me know what you think. I prefer the friend and moving to test, otherwise you're adding a bunch of code that may or may not get optimized out that is only for testing. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:75: base::WeakPtrFactory<IntentPickerBubbleViewTest> weak_ptr_factory_; On 2016/07/22 01:22:07, djacobo wrote: > On 2016/07/21 22:57:06, sky wrote: > > Do you really need the ptr factory here? > > I think so. The idea was to use it with the Bind in line 53. You only need a weak_ptr if the bind is going to outlive the object you're supplying. AFAICT that isn't the case. https://codereview.chromium.org/2134293002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:27: namespace content { Why do you need the forward declaration here? If you have it anywhere, I would expect the header (which is the place that has contents::WebContents).
The CQ bit was checked by djacobo@google.com 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 checked by djacobo@google.com 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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by djacobo@google.com 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/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:267: if (!was_callback_run_) { On 2016/07/22 16:32:54, sky wrote: > On 2016/07/22 01:22:07, djacobo wrote: > > On 2016/07/21 22:57:06, sky wrote: > > > Why do you need was_callback_run_? > > > > I don't want the callback being run twice since that could end in a bad state. > > How can that happen? AFAICT the widget is closed on the next line? We can get rid of the if (!was_callback_run_) checkers here, but I still need the flag so the bubble don't use the callback again on exit (or if the WebContents gets destroyed calling WebContentsDestroyed(). https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:71: static std::unique_ptr<IntentPickerBubbleView> CreateBubbleForTesting( On 2016/07/22 16:32:54, sky wrote: > On 2016/07/22 01:22:07, djacobo wrote: > > On 2016/07/21 22:57:06, sky wrote: > > > You're exposing 3 methods purely for testing. How about friending the test > > > instead? > > > > We already did that on a previous version but opted for this way (with > > ForTesting() methods) in order to keep a better separation of the class to > test > > and the tester, let me know what you think. > > I prefer the friend and moving to test, otherwise you're adding a bunch of code > that may or may not get optimized out that is only for testing. Ok, got rid of the ForTesting() methods, I want to keep this method as static. Take a look at the new version. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:75: base::WeakPtrFactory<IntentPickerBubbleViewTest> weak_ptr_factory_; On 2016/07/22 16:32:54, sky wrote: > On 2016/07/22 01:22:07, djacobo wrote: > > On 2016/07/21 22:57:06, sky wrote: > > > Do you really need the ptr factory here? > > > > I think so. The idea was to use it with the Bind in line 53. > > You only need a weak_ptr if the bind is going to outlive the object you're > supplying. AFAICT that isn't the case. That is true, however if I don't Bind my method in this fashion I receive compiling errors. I would prefer to stick with this implementation if possible so we don't have to modify the general structure of the code (and the constructor mainly). What do you think? https://codereview.chromium.org/2134293002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:27: namespace content { On 2016/07/22 16:32:54, sky wrote: > Why do you need the forward declaration here? If you have it anywhere, I would > expect the header (which is the place that has contents::WebContents). Moving to the header, I think we needed the forward declaration since we are only passing around pointers to WebContents.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:267: if (!was_callback_run_) { On 2016/07/22 22:37:24, djacobo wrote: > On 2016/07/22 16:32:54, sky wrote: > > On 2016/07/22 01:22:07, djacobo wrote: > > > On 2016/07/21 22:57:06, sky wrote: > > > > Why do you need was_callback_run_? > > > > > > I don't want the callback being run twice since that could end in a bad > state. > > > > How can that happen? AFAICT the widget is closed on the next line? > > We can get rid of the if (!was_callback_run_) checkers here, but I still need > the flag so the bubble don't use the callback again on exit (or if the > WebContents gets destroyed calling WebContentsDestroyed(). That I understand. My question is solely about the need to add the checks in this function. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:75: base::WeakPtrFactory<IntentPickerBubbleViewTest> weak_ptr_factory_; On 2016/07/22 22:37:24, djacobo wrote: > On 2016/07/22 16:32:54, sky wrote: > > On 2016/07/22 01:22:07, djacobo wrote: > > > On 2016/07/21 22:57:06, sky wrote: > > > > Do you really need the ptr factory here? > > > > > > I think so. The idea was to use it with the Bind in line 53. > > > > You only need a weak_ptr if the bind is going to outlive the object you're > > supplying. AFAICT that isn't the case. > > That is true, however if I don't Bind my method in this fashion I receive > compiling errors. I would prefer to stick with this implementation if possible > so we don't have to modify the general structure of the code (and the > constructor mainly). What do you think? > Sorry, I should have been clear what I'm asking for. Can't you use base::Unretained here? https://codereview.chromium.org/2134293002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:27: namespace content { On 2016/07/22 22:37:25, djacobo wrote: > On 2016/07/22 16:32:54, sky wrote: > > Why do you need the forward declaration here? If you have it anywhere, I would > > expect the header (which is the place that has contents::WebContents). > > Moving to the header, I think we needed the forward declaration since we are > only passing around pointers to WebContents. You must be inheriting it from someone else your previous patch would have failed. Moving to the header is the right thing though.
The CQ bit was checked by djacobo@google.com 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/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:267: if (!was_callback_run_) { On 2016/07/22 23:45:56, sky wrote: > On 2016/07/22 22:37:24, djacobo wrote: > > On 2016/07/22 16:32:54, sky wrote: > > > On 2016/07/22 01:22:07, djacobo wrote: > > > > On 2016/07/21 22:57:06, sky wrote: > > > > > Why do you need was_callback_run_? > > > > > > > > I don't want the callback being run twice since that could end in a bad > > state. > > > > > > How can that happen? AFAICT the widget is closed on the next line? > > > > We can get rid of the if (!was_callback_run_) checkers here, but I still need > > the flag so the bubble don't use the callback again on exit (or if the > > WebContents gets destroyed calling WebContentsDestroyed(). > > That I understand. My question is solely about the need to add the checks in > this function. Oh ok, we actually don't need it anymore. The thing is, in a previous version I was writing testing methods which relied in the fact that a callback should never be triggered . However now we have a dummy function on the testing class, which makes the was_callback_run_ flag unnecessary for this part. https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:75: base::WeakPtrFactory<IntentPickerBubbleViewTest> weak_ptr_factory_; On 2016/07/22 23:45:56, sky wrote: > On 2016/07/22 22:37:24, djacobo wrote: > > On 2016/07/22 16:32:54, sky wrote: > > > On 2016/07/22 01:22:07, djacobo wrote: > > > > On 2016/07/21 22:57:06, sky wrote: > > > > > Do you really need the ptr factory here? > > > > > > > > I think so. The idea was to use it with the Bind in line 53. > > > > > > You only need a weak_ptr if the bind is going to outlive the object you're > > > supplying. AFAICT that isn't the case. > > > > That is true, however if I don't Bind my method in this fashion I receive > > compiling errors. I would prefer to stick with this implementation if possible > > so we don't have to modify the general structure of the code (and the > > constructor mainly). What do you think? > > > > Sorry, I should have been clear what I'm asking for. Can't you use > base::Unretained here? Done. https://codereview.chromium.org/2134293002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:27: namespace content { On 2016/07/22 23:45:56, sky wrote: > On 2016/07/22 22:37:25, djacobo wrote: > > On 2016/07/22 16:32:54, sky wrote: > > > Why do you need the forward declaration here? If you have it anywhere, I > would > > > expect the header (which is the place that has contents::WebContents). > > > > Moving to the header, I think we needed the forward declaration since we are > > only passing around pointers to WebContents. > > You must be inheriting it from someone else your previous patch would have > failed. Moving to the header is the right thing though. Yeah I agree, at least a subset of the trybots should have died if totally required, I will try to stick to IWYU if possible :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:269: if (static_cast<size_t>(sender->tag()) >= app_info_.size()) Under what conditions will this, line 311 and 320 be true? From my read here they should never happen, and so should be DCHECKs. See discussion of DCHECKs here for details: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:62: for (auto& app : app_info_) { nit: no {} https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:84: EXPECT_TRUE(app != nullptr); General practice is to redirect the loop index so that if an assertion hits you know what triggered it, eg: EXECT_TRUE(app) << i
The CQ bit was checked by djacobo@google.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:269: if (static_cast<size_t>(sender->tag()) >= app_info_.size()) On 2016/07/25 15:12:19, sky wrote: > Under what conditions will this, line 311 and 320 be true? From my read here > they should never happen, and so should be DCHECKs. See discussion of DCHECKs > here for details: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Well the basic flow of this UI ensures the |selected_app_tag_| has a value in the range [0, app_info_.size() - 1] every time the user presses a LabelButton. I put these index checkers by request, since the |selected_app_tag_| is initialized (in the UI constructor) to a very big value. So the reason is basically safety, but I don't see how an invalid value could sneak in this section. https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:62: for (auto& app : app_info_) { On 2016/07/25 15:12:19, sky wrote: > nit: no {} Done. https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:84: EXPECT_TRUE(app != nullptr); On 2016/07/25 15:12:19, sky wrote: > General practice is to redirect the loop index so that if an assertion hits you > know what triggered it, eg: > EXECT_TRUE(app) << i Changed here and next test. Done
https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:269: if (static_cast<size_t>(sender->tag()) >= app_info_.size()) On 2016/07/25 19:42:21, djacobo wrote: > On 2016/07/25 15:12:19, sky wrote: > > Under what conditions will this, line 311 and 320 be true? From my read here > > they should never happen, and so should be DCHECKs. See discussion of DCHECKs > > here for details: > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > Well the basic flow of this UI ensures the |selected_app_tag_| has a value in > the range [0, app_info_.size() - 1] every time the user presses a LabelButton. I > put these index checkers by request, since the |selected_app_tag_| is > initialized (in the UI constructor) to a very big value. So the reason is > basically safety, but I don't see how an invalid value could sneak in this > section. Again, see the style guide discussion. From what I can tell this should be a DCHECK as should 311 and 320.
The CQ bit was checked by djacobo@google.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:269: if (static_cast<size_t>(sender->tag()) >= app_info_.size()) On 2016/07/25 20:20:26, sky wrote: > On 2016/07/25 19:42:21, djacobo wrote: > > On 2016/07/25 15:12:19, sky wrote: > > > Under what conditions will this, line 311 and 320 be true? From my read here > > > they should never happen, and so should be DCHECKs. See discussion of > DCHECKs > > > here for details: > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > > > Well the basic flow of this UI ensures the |selected_app_tag_| has a value in > > the range [0, app_info_.size() - 1] every time the user presses a LabelButton. > I > > put these index checkers by request, since the |selected_app_tag_| is > > initialized (in the UI constructor) to a very big value. So the reason is > > basically safety, but I don't see how an invalid value could sneak in this > > section. > > Again, see the style guide discussion. From what I can tell this should be a > DCHECK as should 311 and 320. Ok, notice that if I use DCHECKs in here it will be in the same fashion as views.h does in the child_at() method, so, wouldn't be better if I remove this index check AND rely on the child_at() DCHECKs? As a result I would remove a couple of checks on the unit tests and attach the index print to the other checks. what do you think?
DCHECKs also serve as a way to document expectations, so code like: // Only way to get here is if app_info_ is valid. DCHECK(static_cast<size_t>(sender->tag()) < app_info_.size()); Is much easier to follow than a DCHECK in other code. -Scott On Mon, Jul 25, 2016 at 2:29 PM, <djacobo@google.com> wrote: > > https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... > File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): > > https://codereview.chromium.org/2134293002/diff/360001/chrome/browser/ui/view... > chrome/browser/ui/views/intent_picker_bubble_view.cc:269: if > (static_cast<size_t>(sender->tag()) >= app_info_.size()) > On 2016/07/25 20:20:26, sky wrote: >> On 2016/07/25 19:42:21, djacobo wrote: >> > On 2016/07/25 15:12:19, sky wrote: >> > > Under what conditions will this, line 311 and 320 be true? From my > read here >> > > they should never happen, and so should be DCHECKs. See discussion > of >> DCHECKs >> > > here for details: >> > > >> > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md >> > >> > Well the basic flow of this UI ensures the |selected_app_tag_| has a > value in >> > the range [0, app_info_.size() - 1] every time the user presses a > LabelButton. >> I >> > put these index checkers by request, since the |selected_app_tag_| > is >> > initialized (in the UI constructor) to a very big value. So the > reason is >> > basically safety, but I don't see how an invalid value could sneak > in this >> > section. >> >> Again, see the style guide discussion. From what I can tell this > should be a >> DCHECK as should 311 and 320. > > Ok, notice that if I use DCHECKs in here it will be in the same fashion > as views.h does in the child_at() method, so, wouldn't be better if I > remove this index check AND rely on the child_at() DCHECKs? As a result > I would remove a couple of checks on the unit tests and attach the index > print to the other checks. what do you think? > > https://codereview.chromium.org/2134293002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM with at least a comment.
The CQ bit was checked by djacobo@google.com 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: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2134293002/#ps420001 (title: "Adding a DCHECK to make code easier to follow.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adding a ScrollView for IntentPickerBubbleView Adding a ScrollView so we can handle cases when ARC has more that 3 app candidates to handle a given URL. We are also marking the app selected by the user by changing the background color. Notice that we are displaying 3.5 rows when the app candidates exceed 3, this is the intended behavior. Removing EventMonitor and algorithm as these are not used anymore. BUG=620129 ========== to ========== Adding a ScrollView for IntentPickerBubbleView Adding a ScrollView so we can handle cases when ARC has more that 3 app candidates to handle a given URL. We are also marking the app selected by the user by changing the background color. Notice that we are displaying 3.5 rows when the app candidates exceed 3, this is the intended behavior. Removing EventMonitor and algorithm as these are not used anymore. BUG=620129 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Adding a ScrollView for IntentPickerBubbleView Adding a ScrollView so we can handle cases when ARC has more that 3 app candidates to handle a given URL. We are also marking the app selected by the user by changing the background color. Notice that we are displaying 3.5 rows when the app candidates exceed 3, this is the intended behavior. Removing EventMonitor and algorithm as these are not used anymore. BUG=620129 ========== to ========== Adding a ScrollView for IntentPickerBubbleView Adding a ScrollView so we can handle cases when ARC has more that 3 app candidates to handle a given URL. We are also marking the app selected by the user by changing the background color. Notice that we are displaying 3.5 rows when the app candidates exceed 3, this is the intended behavior. Removing EventMonitor and algorithm as these are not used anymore. BUG=620129 Committed: https://crrev.com/8c141a8b213ec8e0ba570820c0a7abf7ba38d28b Cr-Commit-Position: refs/heads/master@{#407663} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/8c141a8b213ec8e0ba570820c0a7abf7ba38d28b Cr-Commit-Position: refs/heads/master@{#407663} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
