Fixing crash Report - Magic Signature: views::View::ConvertPointToScreen

Fixing crash Report - Magic Signature: views::View::ConvertPointToScreen

The problem:
- The BubbleDelegateView tracks the destruction of the widget which owns the anchor view.
- The anchor view gets queried dynamically for its location upon any position change of the anchor widget.
- When a view gets deleted, the widget does not have to be deleted.
Taking that all together and looking at the shelf it is possible to get to the following scenario:

The user puts the shelf into auto hiding mode (so it changes its location). The bubble gets shown. The button - which was used for the anchor - gets deleted, which triggers the destruction of the bubble. The bubble however gets faded out slowly (while the shelf shifts out of view). In this case the location would get queried, but the view is already gone.

BUG=297676
TEST=unittest, but none visually since there isn't a simple reproducible case known

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226158

[Mr4D (OOO till 08-26), 2013-09-25 22:59:15]

As discussed. Please have a look!

[msw, 2013-09-26 22:17:18]

Sorry I didn't fully understand the problem until I saw this CL. Overall this
might be a worthwhile fix for the cited crashes, so I'd accept this change
alone, but it's not the best solution, as you mentioned yesterday.

Clearing the anchor on fade-out and close-on-deactivate won't handle other
crashes when the anchor view is destroyed but the anchor widget is not and then
the bubble tries to reposition with the Widget. Changing the anchor view raw
pointer to a WeakPtr or adding a View destruction observer mechanism would solve
that completely. I'd suggest using WeakPtrFactory, but that'd require changing
all Views used as bubble anchors...

A few alternate approaches would be to (1) call set_move_with_anchor(false) for
these problematic bubbles, (2) give the problematic bubbles an initial
anchor_rect instead of an anchor_view, (3) call something like DetachFromAnchor
in those anchor views' destructors.

All that said, this patch *may* help avoid the cited crashes, so it's a viable
option. I'd like Scott's opinion if he has time to think about this.

https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
ui/views/bubble/bubble_delegate.cc:447: return
This statement needs a semicolon. Have you tested this patch?

https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
ui/views/bubble/bubble_delegate.cc:448: anchor_widget_->RemoveObserver(this);
Do this in an if (anchor_widget) block, and set |anchor_view| and
|anchor_widget| to NULL regardless instead of early returning if |anchor_widget|
is NULL.

[Mr4D (OOO till 08-26), 2013-09-26 22:52:31]

Somehow I expected this - which is the reason why I told you at the end of our
discussion to send you the CL and then you would understand :)

Just to underline what you said: Yes, that is exactly what I said.

Unless the destruction of the view does not trigger the release of the anchor
there is always the possibility for a crash.

Looking at the use case I think that this solution will address the problem at
hand, but it is not a guarantee against accidents / mis-use.

[sky, 2013-09-26 23:18:13]

How about making Bubble use ViewStorage to track the view? ViewStorage
associates an id with a view and allows looking up views by id.
ViewStorage is notified when all views are destroyed so that it can
cleanup internal maps.

  -Scott

On Thu, Sep 26, 2013 at 3:17 PM,  <msw@chromium.org> wrote:
> Sorry I didn't fully understand the problem until I saw this CL. Overall
> this
> might be a worthwhile fix for the cited crashes, so I'd accept this change
> alone, but it's not the best solution, as you mentioned yesterday.
>
> Clearing the anchor on fade-out and close-on-deactivate won't handle other
> crashes when the anchor view is destroyed but the anchor widget is not and
> then
> the bubble tries to reposition with the Widget. Changing the anchor view raw
> pointer to a WeakPtr or adding a View destruction observer mechanism would
> solve
> that completely. I'd suggest using WeakPtrFactory, but that'd require
> changing
> all Views used as bubble anchors...
>
> A few alternate approaches would be to (1) call set_move_with_anchor(false)
> for
> these problematic bubbles, (2) give the problematic bubbles an initial
> anchor_rect instead of an anchor_view, (3) call something like
> DetachFromAnchor
> in those anchor views' destructors.
>
> All that said, this patch *may* help avoid the cited crashes, so it's a
> viable
> option. I'd like Scott's opinion if he has time to think about this.
>
>
>
https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
> File ui/views/bubble/bubble_delegate.cc (right):
>
>
https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
> ui/views/bubble/bubble_delegate.cc:447: return
> This statement needs a semicolon. Have you tested this patch?
>
>
https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
> ui/views/bubble/bubble_delegate.cc:448:
> anchor_widget_->RemoveObserver(this);
> Do this in an if (anchor_widget) block, and set |anchor_view| and
> |anchor_widget| to NULL regardless instead of early returning if
> |anchor_widget| is NULL.
>
> https://codereview.chromium.org/24469006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mr4D (OOO till 08-26), 2013-09-27 02:47:14]

StorageView - interesting! I looked quickly at it and that looks good!

Will update the CL.


On Thu, Sep 26, 2013 at 4:17 PM, Scott Violet <sky@chromium.org> wrote:

> How about making Bubble use ViewStorage to track the view? ViewStorage
> associates an id with a view and allows looking up views by id.
> ViewStorage is notified when all views are destroyed so that it can
> cleanup internal maps.
>
>   -Scott
>
> On Thu, Sep 26, 2013 at 3:17 PM,  <msw@chromium.org> wrote:
> > Sorry I didn't fully understand the problem until I saw this CL. Overall
> > this
> > might be a worthwhile fix for the cited crashes, so I'd accept this
> change
> > alone, but it's not the best solution, as you mentioned yesterday.
> >
> > Clearing the anchor on fade-out and close-on-deactivate won't handle
> other
> > crashes when the anchor view is destroyed but the anchor widget is not
> and
> > then
> > the bubble tries to reposition with the Widget. Changing the anchor view
> raw
> > pointer to a WeakPtr or adding a View destruction observer mechanism
> would
> > solve
> > that completely. I'd suggest using WeakPtrFactory, but that'd require
> > changing
> > all Views used as bubble anchors...
> >
> > A few alternate approaches would be to (1) call
> set_move_with_anchor(false)
> > for
> > these problematic bubbles, (2) give the problematic bubbles an initial
> > anchor_rect instead of an anchor_view, (3) call something like
> > DetachFromAnchor
> > in those anchor views' destructors.
> >
> > All that said, this patch *may* help avoid the cited crashes, so it's a
> > viable
> > option. I'd like Scott's opinion if he has time to think about this.
> >
> >
> >
>
https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
> > File ui/views/bubble/bubble_delegate.cc (right):
> >
> >
>
https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
> > ui/views/bubble/bubble_delegate.cc:447: return
> > This statement needs a semicolon. Have you tested this patch?
> >
> >
>
https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
> > ui/views/bubble/bubble_delegate.cc:448:
> > anchor_widget_->RemoveObserver(this);
> > Do this in an if (anchor_widget) block, and set |anchor_view| and
> > |anchor_widget| to NULL regardless instead of early returning if
> > |anchor_widget| is NULL.
> >
> > https://codereview.chromium.org/24469006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mr4D (OOO till 08-26), 2013-09-27 17:22:50]

Please have another look!

Note: Since several commands were not "unix hacker style" anymore I had to do
some more changes - sorry for that!

https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
ui/views/bubble/bubble_delegate.cc:447: return
I told you to send a patch within 10 minutes so that you have a better
understanding what I am talking about (as you have pointed out that was a good
idea ;) ).

https://codereview.chromium.org/24469006/diff/1/ui/views/bubble/bubble_delega...
ui/views/bubble/bubble_delegate.cc:448: anchor_widget_->RemoveObserver(this);
On 2013/09/26 22:17:19, msw wrote:
> Do this in an if (anchor_widget) block, and set |anchor_view| and
> |anchor_widget| to NULL regardless instead of early returning if
|anchor_widget|
> is NULL.

Done.

[msw, 2013-09-27 18:20:31]

I think this will be a great change once it's simplified.

https://codereview.chromium.org/24469006/diff/27001/ash/shelf/overflow_bubble.cc
File ash/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:84: DCHECK(AnchorView());
nit: Remove this. DCHECKing just before a potential null deref is not necessary.

https://codereview.chromium.org/24469006/diff/27001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/24469006/diff/27001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:844: // Note that
even thogh the view might already be gone, the owning
nit: "though". I don't quite understand the point of this comment.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:133:
last_known_anchor_view_rect_(gfx::Rect()),
Remove this and others, the default gfx::Rect ctor is implicitly invoked.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:291: anchor_view_storage_id_ =
ViewStorage::GetInstance()->CreateStorageID();
This int needs ctor initialization; either initialize it to -1 and replace
|has_anchor_view_| with (anchor_view_storage_id_ != -1), or initialize it with a
new storage ID like this and drop |has_anchor_view_| in favor of simply checking
for GetAnchorView.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:298: void
BubbleDelegateView::SetAnchorRect(const gfx::Rect& rect) {
Reverted this to set_anchor_rect with my suggestion in GetAnchorRect.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:304: if (!AnchorView()) {
I think this function can remain largely the same; remove
|last_known_anchor_view_rect_| and simply update and return |anchor_rect_| when
this is called and AnchorView() exists.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:62: View* AnchorView() const;
nit: GetAnchorView.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:64: // Even though AnchorView might return
NULL, the bubble might have an
Why is this important? I think it can be removed.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:69: const gfx::Rect& AnchorRect() const;
nit: GetAnchorRect.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:189: // To avoid the bubble to jump upon early
anchor destruction, we remember the
nit: I think the complex behavior and the rest of this comment "To avoid ...
cleared." is unnecessary; please remove that part of this comment and see my
other comments about simplifying the change.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:121: // Create an anchor widget
which has multiple views, one of them will be used
I don't see multiple views, revise this comment.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:124: scoped_ptr<View>
anchor_view(new View());
Do not use a scoped_ptr here, the View is owned by the widget.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:128: // Preventing close on
deactivate should not prevent closing with the anchor.
Why are you testing this and calling set_close_on_deactivate(false); here?

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:131: EXPECT_EQ(bubble_delegate,
bubble_widget->widget_delegate());
Remove the expectations checked by other tests, they're not needed here.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:134: test::TestWidgetObserver
bubble_observer(bubble_widget);
Remove this observer, it's not needed for this test.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:136: // Check that the anchor view
is correct and set up an anchor view rect.
It's probably good to test that the anchor_view bounds override an anchor rect,
(and also that updated anchor_view positions are reflected in subsequent calls
to GetAnchorRect) but that behavior should be checked in a separate test.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:141: gfx::Rect set_anchor_rect =
gfx::Rect(10, 10, 100, 100);
nit: const.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:143: gfx::Rect view_rect =
bubble_delegate->GetAnchorRect();
nit: const.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:159: // Inform now the bubble that
the anchor view is gone and expect it to
I think this behavior is unnecessary; is it ever used in practice?
Couldn't any user simply set a new anchor rect instead?

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:166: // Ensure that closing the
anchor widget also closes the bubble itself.
This test does not need to check this.

https://codereview.chromium.org/24469006/diff/27001/ui/views/touchui/touch_se...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/touchui/touch_se...
ui/views/touchui/touch_selection_controller_impl.cc:425: gfx::Point
anchor(context_menu_->AnchorRect().CenterPoint().x(),
nit: cache the anchor rect in a local beforehand.

[Mr4D (OOO till 08-26), 2013-09-27 20:29:35]

Addressed.

Please note: You asked to not overwrite the bounds explicitly in our chat
earlier since I suggested to do it this way. As such I added your constraint as
well as the unit test to make sure it does not happen. Since this seems suddenly
not of concern anymore I have removed it again.

Please have another look

https://codereview.chromium.org/24469006/diff/27001/ash/shelf/overflow_bubble.cc
File ash/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:84: DCHECK(AnchorView());
On 2013/09/27 18:20:31, msw wrote:
> nit: Remove this. DCHECKing just before a potential null deref is not
necessary.

Done.

https://codereview.chromium.org/24469006/diff/27001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/24469006/diff/27001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:844: // Note that
even thogh the view might already be gone, the owning
On 2013/09/27 18:20:31, msw wrote:
> nit: "though". I don't quite understand the point of this comment.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:133:
last_known_anchor_view_rect_(gfx::Rect()),
On 2013/09/27 18:20:31, msw wrote:
> Remove this and others, the default gfx::Rect ctor is implicitly invoked.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:291: anchor_view_storage_id_ =
ViewStorage::GetInstance()->CreateStorageID();
According to the header file -1 is not an invalid state. As there are no invalid
storage ID's it seems to be much more secure to have the boolean. Note that due
to this, there is also no ctor initialization required (since there is no
invalid value).

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:298: void
BubbleDelegateView::SetAnchorRect(const gfx::Rect& rect) {
On 2013/09/27 18:20:31, msw wrote:
> Reverted this to set_anchor_rect with my suggestion in GetAnchorRect.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:304: if (!AnchorView()) {
I did that this way since you *explicitly* told me yesterday multiple times that
you do *not* want to get the location overwritten. Which is also the reason why
I added that to the unit test to make sure that your requirement was taken care
of.

Okay, since you do not care about it anymore I can of course remove this again.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:62: View* AnchorView() const;
On 2013/09/27 18:20:31, msw wrote:
> nit: GetAnchorView.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:64: // Even though AnchorView might return
NULL, the bubble might have an
Because the bubble might have been created with an anchor_view, has still an
anchor_widget and the anchor_rect from its last use but the view itself is gone.

(See immersive_mode_controller as an example)

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:69: const gfx::Rect& AnchorRect() const;
On 2013/09/27 18:20:31, msw wrote:
> nit: GetAnchorRect.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:189: // To avoid the bubble to jump upon early
anchor destruction, we remember the
On 2013/09/27 18:20:31, msw wrote:
> nit: I think the complex behavior and the rest of this comment "To avoid ...
> cleared." is unnecessary; please remove that part of this comment and see my
> other comments about simplifying the change.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:121: // Create an anchor widget
which has multiple views, one of them will be used
Well there is already the content view. Therefore there are multiple, but
changed.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:124: scoped_ptr<View>
anchor_view(new View());
That is not correct. The view will get removed from the hierarchy and
"RemoveChildView" is NOT deleting the view. So it still belongs to us and we
need to get rid of it. Sure I could remove the scoped_ptr and simply delete it -
is that what you are asking for?

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:128: // Preventing close on
deactivate should not prevent closing with the anchor.
On 2013/09/27 18:20:31, msw wrote:
> Why are you testing this and calling set_close_on_deactivate(false); here?

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:131: EXPECT_EQ(bubble_delegate,
bubble_widget->widget_delegate());
On 2013/09/27 18:20:31, msw wrote:
> Remove the expectations checked by other tests, they're not needed here.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:134: test::TestWidgetObserver
bubble_observer(bubble_widget);
On 2013/09/27 18:20:31, msw wrote:
> Remove this observer, it's not needed for this test.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:136: // Check that the anchor view
is correct and set up an anchor view rect.
Not quite sure I get what you are asking for here. I assume that you want that
location changes of the view will lead to different rectangles. Will add that.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:141: gfx::Rect set_anchor_rect =
gfx::Rect(10, 10, 100, 100);
On 2013/09/27 18:20:31, msw wrote:
> nit: const.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:143: gfx::Rect view_rect =
bubble_delegate->GetAnchorRect();
On 2013/09/27 18:20:31, msw wrote:
> nit: const.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:159: // Inform now the bubble that
the anchor view is gone and expect it to
You asked for this yesterday. Done. removed.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:166: // Ensure that closing the
anchor widget also closes the bubble itself.
On 2013/09/27 18:20:31, msw wrote:
> This test does not need to check this.

Done.

https://codereview.chromium.org/24469006/diff/27001/ui/views/touchui/touch_se...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/touchui/touch_se...
ui/views/touchui/touch_selection_controller_impl.cc:425: gfx::Point
anchor(context_menu_->AnchorRect().CenterPoint().x(),
On 2013/09/27 18:20:31, msw wrote:
> nit: cache the anchor rect in a local beforehand.

Done.

[msw, 2013-09-27 21:21:52]

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:291: anchor_view_storage_id_ =
ViewStorage::GetInstance()->CreateStorageID();
On 2013/09/27 20:29:35, Mr4D wrote:
> According to the header file -1 is not an invalid state. As there are no
invalid
> storage ID's it seems to be much more secure to have the boolean. Note that
due
> to this, there is also no ctor initialization required (since there is no
> invalid value).

CreateStorageID always uses positive numbers, at least until it overflows, but I
guess the API sadly doesn't make any such guarantee. I suggest initializing
|anchor_view_storage_id_| to a valid storage id in the ctor, as most other
ViewStorage users do. In the worst case ViewStorage::view_storage_next_id_ will
overflow when the user shows 32k bubbles (or browser windows, etc.), and
probably continue to be valid until 64k such IDs are taken, and then it will
began to repeat and clash IDs... This seems like a low-pri bug that should be
addressed separately.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:304: if (!AnchorView()) {
On 2013/09/27 20:29:35, Mr4D wrote:
> I did that this way since you *explicitly* told me yesterday multiple times
that
> you do *not* want to get the location overwritten. Which is also the reason
why
> I added that to the unit test to make sure that your requirement was taken
care
> of.
> 
> Okay, since you do not care about it anymore I can of course remove this
again.

Sorry for not being clear enough yesterday, but that's not what I asked for. I
asked you to not cache the anchor view's last known bounds at all, merely to
avoid complexity. But now that I see the code, it's quite easy to just overwrite
|anchor_rect_| on each GetAnchorRect call, as your latest patch set does, so
thanks for cleaning up the extra Rect class member.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:64: // Even though AnchorView might return
NULL, the bubble might have an
On 2013/09/27 20:29:35, Mr4D wrote:
> Because the bubble might have been created with an anchor_view, has still an
> anchor_widget and the anchor_rect from its last use but the view itself is
gone.
> 
> (See immersive_mode_controller as an example)

I understand how that works, but why would anyone care if there was an anchor
view but there is not one any more? If that code should care about the existence
of a parent window or an anchor widget, it should use those instead.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:124: scoped_ptr<View>
anchor_view(new View());
On 2013/09/27 20:29:35, Mr4D wrote:
> That is not correct. The view will get removed from the hierarchy and
> "RemoveChildView" is NOT deleting the view. So it still belongs to us and we
> need to get rid of it. Sure I could remove the scoped_ptr and simply delete it
-
> is that what you are asking for?

Nope, I incorrectly assumed that RemoveChildView would delete the view. Thank
you for correcting me, you can keep the scoped_ptr.

https://codereview.chromium.org/24469006/diff/27001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:136: // Check that the anchor view
is correct and set up an anchor view rect.
On 2013/09/27 20:29:35, Mr4D wrote:
> Not quite sure I get what you are asking for here. I assume that you want that
> location changes of the view will lead to different rectangles. Will add that.

I'm asking you to simplify this test. The interaction between SetAnchorRect and
SetAnchorView should be done in another test. This test should simply check that
GetAnchorRect returns the same value (|view_rect|), and more importantly doesn't
crash, after the View is deleted.

https://codereview.chromium.org/24469006/diff/63001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/24469006/diff/63001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:840: if
(bubble_delegate && bubble_delegate->has_anchor_view()) {
It seems like this code is avoiding a lock here for "simplicity" by making
BubbleManager obtain its own lock. Wouldn't it be far simpler to remove the
condition here and always grab a lock; then either allow this redundant lock, or
remove the special cased BubbleManager code to obtain a lock?

Either way, I don't like that this code needs to know if the bubble was
previously attached to a view, regardless of it's current condition. Perhaps
those bubbles should set an anchor_widget and this code should check for an
anchor_widget if that's what really matters.

TL;DR: Remove this check and has_anchor_view(); consider other cleanup.

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:291: // If the anchor view was going away, we
have to return the last known good
nit: remove this comment and the curly braces.

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:295: gfx::Rect anchor_bounds =
GetAnchorView()->GetBoundsInScreen();
nit: remove |anchor_bounds|, just use |anchor_rect_|.

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:121: // Create an anchor widget and
add a view to be used as anchor view.
nit: "an anchor view."

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:165: 
nit: remove this blank line.

https://codereview.chromium.org/24469006/diff/63001/ui/views/touchui/touch_se...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/24469006/diff/63001/ui/views/touchui/touch_se...
ui/views/touchui/touch_selection_controller_impl.cc:425: const gfx::Rect& rect =
context_menu_->GetAnchorRect();
I don't think using a ref is correct here, since the function isn't return a
const ref to a class member (though I guess it could do that...).

https://codereview.chromium.org/24469006/diff/63001/ui/views/touchui/touch_se...
ui/views/touchui/touch_selection_controller_impl.cc:426: gfx::Point
anchor(rect.CenterPoint().x(), rect.y());
nit: const

[Mr4D (OOO till 08-26), 2013-09-27 22:52:20]

Okay. Please see comments. Have another look!

https://codereview.chromium.org/24469006/diff/63001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/24469006/diff/63001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:840: if
(bubble_delegate && bubble_delegate->has_anchor_view()) {
Okay - I see you really really really don't want to have a new function in the
class. The luck is with us and with setting the anchor view the anchor widget
will be set as well. So I can only check for the widget here. Done.

BUT WAIT! There was actually a bug since a long time! The function SetAnchorView
was never setting the widget and thus not the observer. As such it is
conceivable that this might have caused problems. Fixed.

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:291: // If the anchor view was going away, we
have to return the last known good
On 2013/09/27 21:21:53, msw wrote:
> nit: remove this comment and the curly braces.

Done.

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:295: gfx::Rect anchor_bounds =
GetAnchorView()->GetBoundsInScreen();
On 2013/09/27 21:21:53, msw wrote:
> nit: remove |anchor_bounds|, just use |anchor_rect_|.

Done.

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:121: // Create an anchor widget and
add a view to be used as anchor view.
On 2013/09/27 21:21:53, msw wrote:
> nit: "an anchor view."

Done.

https://codereview.chromium.org/24469006/diff/63001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:165: 
On 2013/09/27 21:21:53, msw wrote:
> nit: remove this blank line.

Done.

https://codereview.chromium.org/24469006/diff/63001/ui/views/touchui/touch_se...
File ui/views/touchui/touch_selection_controller_impl.cc (right):

https://codereview.chromium.org/24469006/diff/63001/ui/views/touchui/touch_se...
ui/views/touchui/touch_selection_controller_impl.cc:425: const gfx::Rect& rect =
context_menu_->GetAnchorRect();
On 2013/09/27 21:21:53, msw wrote:
> I don't think using a ref is correct here, since the function isn't return a
> const ref to a class member (though I guess it could do that...).

Done.

https://codereview.chromium.org/24469006/diff/63001/ui/views/touchui/touch_se...
ui/views/touchui/touch_selection_controller_impl.cc:426: gfx::Point
anchor(rect.CenterPoint().x(), rect.y());
On 2013/09/27 21:21:53, msw wrote:
> nit: const

Done.

[msw, 2013-09-27 23:45:55]

LGTM with a couple minor comments.

https://codereview.chromium.org/24469006/diff/42001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/42001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:169: // We set the view into the storage
here. CreateBubble will then do the rest
Just call SetAnchorView here. Even if the anchor's Widget has not been
constructed, it should work okay, and the second SetAnchorView call in
CreateBubble would attempt to locate/observe the anchor widget again, right?
Also, adjust the comment in CreateBubble that you removed to something like "Get
the latest anchor widget from the anchor view at bubble creation time."

https://codereview.chromium.org/24469006/diff/42001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:287: if (!anchor_view)
nit: invert this check and call StoreView in its scope.

[Mr4D (OOO till 08-26), 2013-09-28 00:01:17]

Done thanks!

Sky please can you have another look?

https://codereview.chromium.org/24469006/diff/42001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/42001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:169: // We set the view into the storage
here. CreateBubble will then do the rest
On 2013/09/27 23:45:56, msw wrote:
> Just call SetAnchorView here. Even if the anchor's Widget has not been
> constructed, it should work okay, and the second SetAnchorView call in
> CreateBubble would attempt to locate/observe the anchor widget again, right?
> Also, adjust the comment in CreateBubble that you removed to something like
"Get
> the latest anchor widget from the anchor view at bubble creation time."

Done.

https://codereview.chromium.org/24469006/diff/42001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:287: if (!anchor_view)
On 2013/09/27 23:45:56, msw wrote:
> nit: invert this check and call StoreView in its scope.

Done.

[sky, 2013-09-30 14:58:44]

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:262: View*
BubbleDelegateView::GetAnchorView() const {
Make order match header.

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:62: View* GetAnchorView() const;
This should return const View*. If you need a non-const View*, then make it:
View* GetAnchorView();

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:183: int anchor_view_storage_id_;
const

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:120: TEST_F(BubbleDelegateTest,
CloseAnchorViewTest) {
If the bubble close on focus/activation loss, then do these need to be in
interactive ui tests?

[Mr4D (OOO till 08-26), 2013-09-30 16:22:32]

Addressed (mostly - see comments). Please have another look!

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.cc:262: View*
BubbleDelegateView::GetAnchorView() const {
On 2013/09/30 14:58:45, sky wrote:
> Make order match header.

Done.

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:62: View* GetAnchorView() const;
I do not quite follow here. Beside the point that it was before const - the
function still does not change the internal state of this class.

To answer your question: No - the view cannot be const since the view is being
used with non const functions.

I tried to remove the const (even though it left me puzzled why that would be
wise) and it led to a flood of other functions which required it to be const
(which would therefore be non const either). So if you really want to remove the
const - be prepared for a giant review. Still want it?

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:183: int anchor_view_storage_id_;
On 2013/09/30 14:58:45, sky wrote:
> const

Done.

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:120: TEST_F(BubbleDelegateTest,
CloseAnchorViewTest) {
This unit test checks the problematic case where the view gets deleted before
the bubble is gone (which can happen due to fade out animations) and as such it
might query the location again to move with its anchor parent widget (which
still exists).

This test is exactly testing this and somehow I don't see what an interactive
test would gain us here.

That said - somehow it showed me that I didn't add comments for the test itself
(which I usually do) and added them now. So I hope this is "done".

[sky, 2013-09-30 16:44:51]

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/24469006/diff/83001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:120: TEST_F(BubbleDelegateTest,
CloseAnchorViewTest) {
On 2013/09/30 16:22:32, Mr4D wrote:
> This unit test checks the problematic case where the view gets deleted before
> the bubble is gone (which can happen due to fade out animations) and as such
it
> might query the location again to move with its anchor parent widget (which
> still exists).
> 
> This test is exactly testing this and somehow I don't see what an interactive
> test would gain us here.
> 
> That said - somehow it showed me that I didn't add comments for the test
itself
> (which I usually do) and added them now. So I hope this is "done".

The problem is unit tests run in parallel. That means if you have some test that
relies on focus/activation (like I believe this does), then you're not
guaranteed of consistent results and may get flake. If you make the bubble not
close on focus/activation changes, then I think it's fine to leave it here.

[sky, 2013-09-30 16:45:05]

And I'm fine with pushing const changes to a separate patch.

[Mr4D (OOO till 08-26), 2013-09-30 17:06:33]

Added. Please have another look!

[sky, 2013-09-30 20:22:48]

LGTM

[commit-bot: I haz the power, 2013-09-30 20:31:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/24469006/114001

[commit-bot: I haz the power, 2013-10-01 06:00:17]

Change committed as 226158