Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1767363002:
[md] Give each infobar its own layer. (Closed)
|
Created:
4 years, 9 months ago by Evan Stade Modified:
4 years, 9 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[md] Give each infobar its own layer.
This is necessary so latter bars can draw their arrow above the previous bar.
This is similar to crbug.com/589771 , the fix for which only worked for the first infobar.
This also fixes hit testing which was broken when there was a floating bookmark bar below the infobar.
BUG=592727, 593640
Committed: https://crrev.com/966e15221d7c60c7922d94af047a6663cb175c1e
Cr-Commit-Position: refs/heads/master@{#381267}
Patch Set 1 #
Total comments: 12
Patch Set 2 : nits #Patch Set 3 : make DoesIntersectRect do what the name says #
Total comments: 3
Patch Set 4 : improve comment #Patch Set 5 : true intersection #
Total comments: 1
Messages
Total messages: 37 (13 generated)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Description was changed from ========== [md] Give each infobar its own layer. This is necessary so latter bars can draw their arrow above the previous bar. This is similar to crbug.com/589771 which only worked for the first infobar. BUG=592727 ========== to ========== [md] Give each infobar its own layer. This is necessary so latter bars can draw their arrow above the previous bar. This is similar to crbug.com/589771 , the fix for which only worked for the first infobar. BUG=592727 ==========
estade@chromium.org changed reviewers: + pkasting@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767363002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767363002/1
https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_container_view.cc (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container_view.cc:69: bool InfoBarContainerView::DoesIntersectRect(const View* target, it seems it's necessary to keep this as well as adding DoesIntersectRect to the individual bars
https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_container_view.cc (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container_view.cc:73: // vertical space occupied by the arrow). Add to this comment to describe why we want to reject these. https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container_view.cc:74: return rect.CenterPoint().y() >= GetVerticalOverlap(nullptr); Why did this have to be changed? I'm getting increasingly uncomfortable with this inequality -- can we just do a true intersection of the non-top-arrow part of the container view with the supplied rect? https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_view.cc:440: return rect.CenterPoint().y() >= arrow_height(); This doesn't seem right... if the goal is to allow any rect that overlaps the below-arrow portion, this doesn't do that, and if the goal is to block any rect that overlaps the arrow portion, this doesn't do that either. Much like with the container view, this inequality makes me extremely uncomfortable, and I'd much rather have a true rect intersection check with the relevant portion of our bounds. https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_view.h (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_view.h:134: // ViewTargeterDelegate: Nit: views::
https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_container_view.cc (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container_view.cc:73: // vertical space occupied by the arrow). On 2016/03/08 01:08:17, Peter Kasting wrote: > Add to this comment to describe why we want to reject these. Done. https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container_view.cc:74: return rect.CenterPoint().y() >= GetVerticalOverlap(nullptr); On 2016/03/08 01:08:17, Peter Kasting wrote: > Why did this have to be changed? > > I'm getting increasingly uncomfortable with this inequality -- can we just do a > true intersection of the non-top-arrow part of the container view with the > supplied rect? It did not have to be part of this CL. It's somewhat tangential. When you're using a mouse, it won't make a difference, which is why it seemed right to me before. Now that I think more about it, it seems wrong because if you are considering touch events, we probably want to check the middle of your finger, not the bottom edge. https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_view.cc:440: return rect.CenterPoint().y() >= arrow_height(); On 2016/03/08 01:08:17, Peter Kasting wrote: > This doesn't seem right... if the goal is to allow any rect that overlaps the > below-arrow portion, this doesn't do that, and if the goal is to block any rect > that overlaps the arrow portion, this doesn't do that either. > > Much like with the container view, this inequality makes me extremely > uncomfortable, and I'd much rather have a true rect intersection check with the > relevant portion of our bounds. ditto to my other comment. The two behaviors described above are not goals. The goal is to accept any rectangle that is more inside than out and reject any rectangle that is more outside than in. https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_view.h (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_view.h:134: // ViewTargeterDelegate: On 2016/03/08 01:08:17, Peter Kasting wrote: > Nit: views:: Done.
https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_view.cc:440: return rect.CenterPoint().y() >= arrow_height(); On 2016/03/08 01:43:29, Evan Stade wrote: > On 2016/03/08 01:08:17, Peter Kasting wrote: > > This doesn't seem right... if the goal is to allow any rect that overlaps the > > below-arrow portion, this doesn't do that, and if the goal is to block any > rect > > that overlaps the arrow portion, this doesn't do that either. > > > > Much like with the container view, this inequality makes me extremely > > uncomfortable, and I'd much rather have a true rect intersection check with > the > > relevant portion of our bounds. > > ditto to my other comment. The two behaviors described above are not goals. The > goal is to accept any rectangle that is more inside than out and reject any > rectangle that is more outside than in. After reading your other comments, it seems like you're implying that if |rect| is bigger than a point, that it represents the bounds of some kind of touch event? Is that documented somewhere? (Because it sure isn't on ViewTargeter.) Is checking the center point of such a rect the canonical right way to handle such a case? It seems like there needs to be more comments here and possibly on ViewTargeter and possible also elsewhere (I dunno) about all this as I have very little idea of what's going on or why you would want to "accept any rectangle that is more inside than out and reject any rectangle that is more outside than in".
estade@chromium.org changed reviewers: + tdanderson@chromium.org
+tdanderson: you have experience with touch event handling, right? Can you respond to Peter's concerns?
On 2016/03/08 21:46:08, Evan Stade wrote: > +tdanderson: you have experience with touch event handling, right? Can you > respond to Peter's concerns? I the source of the confusion here and I am definitely the right person to be addressing it. But before I reply I'd like to check some assumptions by trying this out on a touchscreen (which I cannot do today since I am OOO). I will take a look and reply when I'm back into work tomorrow.
I'm finally able to build and took a look at this today. Please see below: https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_container_view.cc (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container_view.cc:74: return rect.CenterPoint().y() >= GetVerticalOverlap(nullptr); On 2016/03/08 01:43:29, Evan Stade (ooo Thurs-Fri) wrote: > On 2016/03/08 01:08:17, Peter Kasting wrote: > > Why did this have to be changed? > > > > I'm getting increasingly uncomfortable with this inequality -- can we just do > a > > true intersection of the non-top-arrow part of the container view with the > > supplied rect? > > It did not have to be part of this CL. It's somewhat tangential. > > When you're using a mouse, it won't make a difference, which is why it seemed > right to me before. Now that I think more about it, it seems wrong because if > you are considering touch events, we probably want to check the middle of your > finger, not the bottom edge. You should leave this as rect.bottom() (explained in my other comment). https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1767363002/diff/1/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_view.cc:440: return rect.CenterPoint().y() >= arrow_height(); On 2016/03/08 03:11:04, Peter Kasting wrote: > On 2016/03/08 01:43:29, Evan Stade wrote: > > On 2016/03/08 01:08:17, Peter Kasting wrote: > > > This doesn't seem right... if the goal is to allow any rect that overlaps > the > > > below-arrow portion, this doesn't do that, and if the goal is to block any > > rect > > > that overlaps the arrow portion, this doesn't do that either. > > > > > > Much like with the container view, this inequality makes me extremely > > > uncomfortable, and I'd much rather have a true rect intersection check with > > the > > > relevant portion of our bounds. > > > > ditto to my other comment. The two behaviors described above are not goals. > The > > goal is to accept any rectangle that is more inside than out and reject any > > rectangle that is more outside than in. > > After reading your other comments, it seems like you're implying that if |rect| > is bigger than a point, that it represents the bounds of some kind of touch > event? Is that documented somewhere? (Because it sure isn't on ViewTargeter.) > Is checking the center point of such a rect the canonical right way to handle > such a case? It seems like there needs to be more comments here and possibly on > ViewTargeter and possible also elsewhere (I dunno) about all this as I have very > little idea of what's going on or why you would want to "accept any rectangle > that is more inside than out and reject any rectangle that is more outside than > in". This should also use rect.bottom() instead of rect.CenterPoint().y(). |rect| does indeed represent the bounding box of a touch region. The purpose of DoesIntersectRect() is just to return the result of a hit test of |rect| against |this|. For the case of an infobar, you want to allow events in the rectangular portion of the bar, but not allow events in the arrow part. So the hit test should succeed if the rect intersects with any non-arrow part of the infobar. Now let's say the infobar arrow appears over top of a bookmark button, and you try to tap this button. Even if some of your finger overlaps the rectangular part of the bookmark bar (meaning DoesIntersectRect() returns true for the infobar), that's fine; it's the job of ViewTargeterDelegate::TargetForRect() to find the most probable target of the tap among all of the potential targets (i.e., the targets underneath your finger). The documentation for this is in the header of ViewTargeterDelegate. But after reading this over with fresh eyes I think clarity could be improved by re-naming DoesIntersectRect() to something like IsCandidateForEventTargeting(), DoesIntersectHitTestRegion(), etc. WDYT?
thanks Terry. PTAL latest patch.
LGTM
LGTM https://codereview.chromium.org/1767363002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_container_view.cc (right): https://codereview.chromium.org/1767363002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_container_view.cc:73: // vertical space occupied by the arrow). It would be nice to include some of Terry's explanation in this (and/or on the base class declaration of this function) and use the same comment here and in InfoBarView.
https://codereview.chromium.org/1767363002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_container_view.cc (right): https://codereview.chromium.org/1767363002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_container_view.cc:73: // vertical space occupied by the arrow). On 2016/03/14 22:37:36, Peter Kasting wrote: > It would be nice to include some of Terry's explanation in this (and/or on the > base class declaration of this function) and use the same comment here and in > InfoBarView. I've tried to improve the comment (and match between the two impls), but it seems like any more in-depth explanations or clarifications belong in ViewTargeterDelegate::DoesIntersectRect
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1767363002/#ps60001 (title: "improve comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767363002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767363002/60001
LGTM (again) https://codereview.chromium.org/1767363002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_container_view.cc (right): https://codereview.chromium.org/1767363002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_container_view.cc:73: // vertical space occupied by the arrow). On 2016/03/14 23:12:46, Evan Stade wrote: > On 2016/03/14 22:37:36, Peter Kasting wrote: > > It would be nice to include some of Terry's explanation in this (and/or on the > > base class declaration of this function) and use the same comment here and in > > InfoBarView. > > I've tried to improve the comment (and match between the two impls), but it > seems like any more in-depth explanations or clarifications belong in > ViewTargeterDelegate::DoesIntersectRect Right, when I said "base class declaration" that's what I meant. It would be nice to make the comments on that more clear about what it's used for. That's optional.
The CQ bit was unchecked by estade@chromium.org
Description was changed from ========== [md] Give each infobar its own layer. This is necessary so latter bars can draw their arrow above the previous bar. This is similar to crbug.com/589771 , the fix for which only worked for the first infobar. BUG=592727 ========== to ========== [md] Give each infobar its own layer. This is necessary so latter bars can draw their arrow above the previous bar. This is similar to crbug.com/589771 , the fix for which only worked for the first infobar. This also fixes hit testing which was broken when there was a floating bookmark bar below the infobar. BUG=592727, 593640 ==========
https://codereview.chromium.org/1767363002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1767363002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_view.cc:439: return rect.Intersects(non_arrow_bounds); turns out Intersects is required to fix bug 593640
On 2016/03/14 23:14:29, Peter Kasting wrote: > LGTM (again) > > https://codereview.chromium.org/1767363002/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/infobars/infobar_container_view.cc (right): > > https://codereview.chromium.org/1767363002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/infobars/infobar_container_view.cc:73: // vertical space > occupied by the arrow). > On 2016/03/14 23:12:46, Evan Stade wrote: > > On 2016/03/14 22:37:36, Peter Kasting wrote: > > > It would be nice to include some of Terry's explanation in this (and/or on > the > > > base class declaration of this function) and use the same comment here and > in > > > InfoBarView. > > > > I've tried to improve the comment (and match between the two impls), but it > > seems like any more in-depth explanations or clarifications belong in > > ViewTargeterDelegate::DoesIntersectRect > > Right, when I said "base class declaration" that's what I meant. It would be > nice to make the comments on that more clear about what it's used for. > > That's optional. I think it's a good idea. Terry, would you like to write that comment? Perhaps // Returns true if the bounds of |target| intersects |rect|, where |rect| // is in the local coodinate space of |target|. Overrides of this method by // a View subclass should enforce DCHECK_EQ(this, target). ==> // Returns true if the bounds of |target| intersects |rect|, where |rect| // is in the local coodinate space of |target|. If true, |target| will be // considered as one possible candidate for handling an event in |rect|. // The actual event handler will be chosen among candidate views by XXX.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767363002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767363002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/14 23:23:47, Evan Stade wrote: > https://codereview.chromium.org/1767363002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/infobars/infobar_view.cc (right): > > https://codereview.chromium.org/1767363002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/infobars/infobar_view.cc:439: return > rect.Intersects(non_arrow_bounds); > turns out Intersects is required to fix bug 593640 (slight re-review please?)
On 2016/03/15 01:46:41, Evan Stade wrote: > On 2016/03/14 23:23:47, Evan Stade wrote: > > > https://codereview.chromium.org/1767363002/diff/80001/chrome/browser/ui/views... > > File chrome/browser/ui/views/infobars/infobar_view.cc (right): > > > > > https://codereview.chromium.org/1767363002/diff/80001/chrome/browser/ui/views... > > chrome/browser/ui/views/infobars/infobar_view.cc:439: return > > rect.Intersects(non_arrow_bounds); > > turns out Intersects is required to fix bug 593640 > > (slight re-review please?) LGTM on Intersects(). That makes me happier anyway as it is more true to the name of the function.
On 2016/03/15 07:11:57, Peter Kasting wrote: > On 2016/03/15 01:46:41, Evan Stade wrote: > > On 2016/03/14 23:23:47, Evan Stade wrote: > > > > > > https://codereview.chromium.org/1767363002/diff/80001/chrome/browser/ui/views... > > > File chrome/browser/ui/views/infobars/infobar_view.cc (right): > > > > > > > > > https://codereview.chromium.org/1767363002/diff/80001/chrome/browser/ui/views... > > > chrome/browser/ui/views/infobars/infobar_view.cc:439: return > > > rect.Intersects(non_arrow_bounds); > > > turns out Intersects is required to fix bug 593640 > > > > (slight re-review please?) > > LGTM on Intersects(). That makes me happier anyway as it is more true to the > name of the function. Still LGTM. Thanks for the suggestion in changing the comment, I'll take care of that after this CL lands.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767363002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767363002/80001
Message was sent while issue was closed.
Description was changed from ========== [md] Give each infobar its own layer. This is necessary so latter bars can draw their arrow above the previous bar. This is similar to crbug.com/589771 , the fix for which only worked for the first infobar. This also fixes hit testing which was broken when there was a floating bookmark bar below the infobar. BUG=592727, 593640 ========== to ========== [md] Give each infobar its own layer. This is necessary so latter bars can draw their arrow above the previous bar. This is similar to crbug.com/589771 , the fix for which only worked for the first infobar. This also fixes hit testing which was broken when there was a floating bookmark bar below the infobar. BUG=592727, 593640 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [md] Give each infobar its own layer. This is necessary so latter bars can draw their arrow above the previous bar. This is similar to crbug.com/589771 , the fix for which only worked for the first infobar. This also fixes hit testing which was broken when there was a floating bookmark bar below the infobar. BUG=592727, 593640 ========== to ========== [md] Give each infobar its own layer. This is necessary so latter bars can draw their arrow above the previous bar. This is similar to crbug.com/589771 , the fix for which only worked for the first infobar. This also fixes hit testing which was broken when there was a floating bookmark bar below the infobar. BUG=592727, 593640 Committed: https://crrev.com/966e15221d7c60c7922d94af047a6663cb175c1e Cr-Commit-Position: refs/heads/master@{#381267} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/966e15221d7c60c7922d94af047a6663cb175c1e Cr-Commit-Position: refs/heads/master@{#381267} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
