Move the hiding of Views location bar bubbles into LocationBarView.

Move the hiding of Views location bar bubbles into LocationBarView.

[Justin Donnelly, 2016-03-17 21:42:42]

jdonnelly@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2016-03-17 21:52:07]

I like the previous plan better. This is consistent, in the sense that
copy-paste is consistent, but it is not elegant. Adding more and more to the
already-monolothic BrowserView is unappealing. As much as possible, I think
individual UI elements should take responsibility for their own behavior, i.e.,
all relevant behavior should be concentrated in fewer places.

[Justin Donnelly, 2016-03-17 22:48:28]

On 2016/03/17 21:52:07, Evan Stade wrote:
> I like the previous plan better. This is consistent, in the sense that
> copy-paste is consistent, but it is not elegant. Adding more and more to the
> already-monolothic BrowserView is unappealing. As much as possible, I think
> individual UI elements should take responsibility for their own behavior,
i.e.,
> all relevant behavior should be concentrated in fewer places.

Ok, so how about going forward with the original idea of having LocationBarView
implement TabStripModelObserver then? I could move all the bubble closing code
out of BrowserView::OnActiveTabChanged.

[Evan Stade, 2016-03-18 01:23:42]

sounds excellent.


-- Evan Stade

On Thu, Mar 17, 2016 at 3:48 PM, <jdonnelly@chromium.org> wrote:

> On 2016/03/17 21:52:07, Evan Stade wrote:
> > I like the previous plan better. This is consistent, in the sense that
> > copy-paste is consistent, but it is not elegant. Adding more and more to
> the
> > already-monolothic BrowserView is unappealing. As much as possible, I
> think
> > individual UI elements should take responsibility for their own behavior,
> i.e.,
> > all relevant behavior should be concentrated in fewer places.
>
> Ok, so how about going forward with the original idea of having
> LocationBarView
> implement TabStripModelObserver then? I could move all the bubble closing
> code
> out of BrowserView::OnActiveTabChanged.
>
> https://codereview.chromium.org/1813043002/
>

-- 
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.

[Justin Donnelly, 2016-03-18 19:56:34]

Description was changed from

==========
Hide the save card bubble on tab switch similarly to the other bubbles.

Back in https://codereview.chromium.org/1568983002/ I introduced a tab strip
model observer to the save credit card icon to hide the bubble on tab switch. At
the time, we discussed how it might make sense to generalize that approach to
the other location bar bubbles. But on further review, the other bubbles handle
this in a consistent way already in BrowserView::OnActiveTabChanged.This change
does the same for the save card bubble.

BUG=535784
==========

to

==========
Move the hiding of Views location bar bubbles into LocationBarView.
==========

[Justin Donnelly, 2016-03-18 19:58:02]

PTAL

[Evan Stade, 2016-03-18 21:09:30]

On 2016/03/18 19:58:02, Justin Donnelly wrote:
> PTAL

sorry, I guess I didn't read your last question closely enough. I thought you
were suggesting moving the code to LocationBarBubbleDelegateView or
BubbleIconView. The permissions bubble is enough of an outlier that it can be
handled separately. Perhaps since PermissionBubbleManager is a
WebContentsObserver it can override WasHidden()/WasShown() a la
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[Justin Donnelly, 2016-03-23 20:39:17]

On 2016/03/18 21:09:30, Evan Stade wrote:
> On 2016/03/18 19:58:02, Justin Donnelly wrote:
> > PTAL
> 
> sorry, I guess I didn't read your last question closely enough. I thought you
> were suggesting moving the code to LocationBarBubbleDelegateView or
> BubbleIconView. The permissions bubble is enough of an outlier that it can be
> handled separately. Perhaps since PermissionBubbleManager is a
> WebContentsObserver it can override WasHidden()/WasShown() a la
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

My first reaction was that this sounded great but would require enough reworking
of interfaces that I probably couldn't get to it for a while. But I was looking
at it again this morning and it may be a lot more straightforward than I
thought. I'll send out a new version tomorrow.

[Justin Donnelly, 2016-03-29 16:24:16]

Ok, I take it back. I thought I had seen a way to do this without completely
overhauling the way the lifecycle of the bubbles work. But as soon as you start
pulling on that thread it all unravels given how each bubble works almost
totally differently. It would be great to have each of the bubbles and icons
implement the same lifecycle pattern, managed by a controller to match
SaveCardBubbleController. But that's out of scope for what I can do now.

I could have each BubbleIconView or LocationBarBubbleDelegateView subclass
independently implement TabStripModelObserver and close the bubble in its own
special way. But that seems to me to add even more complexity, especially given
that each view would also have to keep a reference to the browser instance in
order to remove itself as a tab observer.

The change I have here is at least an improvement over the status quo. It moves
the bubble closing on tab switch out of a very bloated class into a less bloated
one, one which is more closely related to the bubble UI. But if you don't think
it's an improvement or enough of one, I'll abandon it.

[Evan Stade, 2016-03-29 19:46:05]

On 2016/03/29 16:24:16, Justin Donnelly wrote:
> Ok, I take it back. I thought I had seen a way to do this without completely
> overhauling the way the lifecycle of the bubbles work. But as soon as you
start
> pulling on that thread it all unravels given how each bubble works almost
> totally differently. It would be great to have each of the bubbles and icons
> implement the same lifecycle pattern, managed by a controller to match
> SaveCardBubbleController. But that's out of scope for what I can do now.
> 
> I could have each BubbleIconView or LocationBarBubbleDelegateView subclass
> independently implement TabStripModelObserver and close the bubble in its own
> special way. But that seems to me to add even more complexity, especially
given
> that each view would also have to keep a reference to the browser instance in
> order to remove itself as a tab observer.
> 
> The change I have here is at least an improvement over the status quo. It
moves
> the bubble closing on tab switch out of a very bloated class into a less
bloated
> one, one which is more closely related to the bubble UI. But if you don't
think
> it's an improvement or enough of one, I'll abandon it.

I would say it's a mild win, but I'd be wary of potential regressions (even from
seemingly simple/bulletproof refactors). So in light of that, I'd just as soon
leave it as it is. If you could file a bug against me to look at this as part of
my bubble work that would be great.

[Justin Donnelly, 2016-03-30 21:12:02]

On 2016/03/29 19:46:05, Evan Stade wrote:
> On 2016/03/29 16:24:16, Justin Donnelly wrote:
> > Ok, I take it back. I thought I had seen a way to do this without completely
> > overhauling the way the lifecycle of the bubbles work. But as soon as you
> start
> > pulling on that thread it all unravels given how each bubble works almost
> > totally differently. It would be great to have each of the bubbles and icons
> > implement the same lifecycle pattern, managed by a controller to match
> > SaveCardBubbleController. But that's out of scope for what I can do now.
> > 
> > I could have each BubbleIconView or LocationBarBubbleDelegateView subclass
> > independently implement TabStripModelObserver and close the bubble in its
own
> > special way. But that seems to me to add even more complexity, especially
> given
> > that each view would also have to keep a reference to the browser instance
in
> > order to remove itself as a tab observer.
> > 
> > The change I have here is at least an improvement over the status quo. It
> moves
> > the bubble closing on tab switch out of a very bloated class into a less
> bloated
> > one, one which is more closely related to the bubble UI. But if you don't
> think
> > it's an improvement or enough of one, I'll abandon it.
> 
> I would say it's a mild win, but I'd be wary of potential regressions (even
from
> seemingly simple/bulletproof refactors). So in light of that, I'd just as soon
> leave it as it is. If you could file a bug against me to look at this as part
of
> my bubble work that would be great.

Sounds good. Filed http://crbug.com/599240.

[Justin Donnelly, 2016-03-30 21:12:08]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2016-03-30 21:12:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813043002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813043002/20001

[commit-bot: I haz the power, 2016-03-30 21:12:28]

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

[commit-bot: I haz the power, 2016-03-30 21:12:29]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Justin Donnelly, 2016-03-30 21:12:55]

On 2016/03/30 21:12:29, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet. 
> CQ run can only be started by full committers or once the patch has
> received an L-G-T-M from a full committer.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Oops. Clicked the wrong button.