Issue 1251633002: Add BubbleManager to manage bubbles and ChromeBubbleManager for events.

please use gerrit instead

rouslan@chromium.org changed reviewers: + rouslan@chromium.org

5 years, 5 months ago (2015-07-22 00:09:36 UTC) #1

please use gerrit instead

A few initial comments. More to come + offline discussions will be useful, too. https://codereview.chromium.org/1251633002/diff/1/chrome/browser/ui/browser.cc ...

5 years, 5 months ago (2015-07-22 00:09:37 UTC) #2

please use gerrit instead

Just the large refactoring things. https://codereview.chromium.org/1251633002/diff/20001/chrome/browser/ui/DEPS File chrome/browser/ui/DEPS (right): https://codereview.chromium.org/1251633002/diff/20001/chrome/browser/ui/DEPS#newcode3 chrome/browser/ui/DEPS:3: "+components/bubble", May not be ...

5 years, 5 months ago (2015-07-22 23:56:09 UTC) #4

hcarmona

Sending this out for discussion. Tests need to be updated. Things I've noticed now that ...

5 years, 4 months ago (2015-08-05 01:57:37 UTC) #6

please use gerrit instead

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/browser.cc#newcode1760 chrome/browser/ui/browser.cc:1760: PermissionBubbleManager* bubble_manager = Shouldn't PermissionBubbleManager go away entirely? https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/chrome_bubble_manager.cc ...

5 years, 4 months ago (2015-08-05 18:05:25 UTC) #7

hcarmona

On deeper inspection: PermissionBubbleDelegate is necessary because the BubbleManager will take ownership. BubbleController still seems ...

5 years, 4 months ago (2015-08-05 18:43:47 UTC) #8

On deeper inspection: PermissionBubbleDelegate is necessary because the
BubbleManager will take ownership.

BubbleController still seems unnecessary.

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1760: PermissionBubbleManager* bubble_manager =
On 2015/08/05 18:05:25, Rouslan wrote:
> Shouldn't PermissionBubbleManager go away entirely?

Permission bubbles need a browser to get their window. The bubble
manager needs to be able to get to it. Is there a way to get the browser
to the PermissionBubbleManager so that it doesn't need to be here?

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_bubble_manager.cc:31: // ShowMatchingBubbles(context);
On 2015/08/05 18:05:25, Rouslan wrote:
> Why does ShowMatchingBubbles(context); not work?

Showing bubbles is easy. Figuring out which bubbles need to be re-shown
is harder.

What if we added a |CloseOnHide| method to the delegate. Then, we can
add an additional |Close| callback to dismiss the bubbles when we won't
be showing them again.

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_bubble_manager.h:15: class ChromeBubbleManager : public
BubbleManager {
On 2015/08/05 18:05:25, Rouslan wrote:
> Can ChromeBubbleManager implement content::WebContentsObserver itself instead
of
> introducing ChromeWebContentsObserver?

Is it possible to observe more than one web contents at the same time?

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right):

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_delegate.h:28: void
Closing();
On 2015/08/05 18:05:25, Rouslan wrote:
> These methods (at least Accept(), Deny(), and Closing()) should be defined in
> BubbleDelegate.

These are all callbacks from the UI. What's the advantage of adding them
to the base class if no part of the bubble API will call them?

https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_delegate.h:33: const
std::vector<bool>& accept_states();
On 2015/08/05 18:05:25, Rouslan wrote:
> These methods are used only in BuildBubbleUI(), so they should not be public.
> Send this information to PermissionBubbleView::Create() somehow.

Only this class knows about it, so it shouldn't be a problem.

hcarmona

Forgot to publish the comments from yesterday. https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1251633002/diff/60001/chrome/browser/ui/browser.cc#newcode1760 chrome/browser/ui/browser.cc:1760: PermissionBubbleManager* bubble_manager ...

5 years, 4 months ago (2015-08-06 17:43:44 UTC) #9

hcarmona

2 quick things I want to change, but please provide feedback on remainder. Thanks! https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/website_settings/permission_bubble_manager.h ...

5 years, 4 months ago (2015-08-06 20:34:10 UTC) #11

groby-ooo-7-16

groby@chromium.org changed reviewers: + groby@chromium.org

5 years, 4 months ago (2015-08-06 21:32:55 UTC) #12

groby-ooo-7-16

A few drive-by notes https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/chrome_bubble_manager.cc File chrome/browser/ui/chrome_bubble_manager.cc (right): https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/chrome_bubble_manager.cc#newcode29 chrome/browser/ui/chrome_bubble_manager.cc:29: friend class content::WebContentsUserData<ChromeWebContentsObserver>; We public ...

5 years, 4 months ago (2015-08-06 21:32:56 UTC) #13

hcarmona

Applied some feedback. I wanted to send this out today to publish comments. I've added ...

5 years, 4 months ago (2015-08-07 02:12:59 UTC) #14

Applied some feedback. I wanted to send this out today to publish
comments. I've added some TODOs in the code for feedback that I haven't
had a chance to apply. I'll upload a new patch tomorrow with the
remaining changes.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_bubble_manager.cc:29: friend class
content::WebContentsUserData<ChromeWebContentsObserver>;
On 2015/08/06 21:32:55, groby wrote:
> We public inherit from that - why do we need to friend it? (This seems to
> indicate a broken API on WCU, or a mistake)

This is done because the constructor is private.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_bubble_manager.cc:33: // Weak.
On 2015/08/06 21:32:55, groby wrote:
> Just add comment at the end. Save a line :)

Line saved.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_bubble_manager.cc:66: void
ChromeBubbleManager::ListenToWebContents(
On 2015/08/06 21:32:55, groby wrote:
> Who invokes that? Shouldn't bubbles at least on Chrome automatically call
this?

Currently used in the BrowserView.
How do we set this up to be called automatically?

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (left):

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:874:
PermissionBubbleManager::FromWebContents(old_contents)->HideBubble();
On 2015/08/06 21:32:56, groby wrote:
> Who's invoking the HideBubble behavior now?

This was redundant because TabDeactivated is always called before this,
so the bubble's already hidden.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:879:
manager->SetBrowser(browser_.get());
On 2015/08/06 21:32:56, groby wrote:
> Why do we need to update the browser here, just for a tab change?

Yes, changing the tab can make the PBM have a different browser.

> Also, why notify the PBM? Why can't we just tell the bubble manager that we've
> changed active tabs, and to please look up the bubble associated with the
> current contents?

The bubble manager doesn't know about the browser or about the PBM.
Should it know about the browser? The ChromeBubbleManager could know
about it, but then we would also need to have a delegate (or bubble ui)
that the ChromeBubbleManager knows about that also knows about the
browser. This would be similar to the existing PermissionBubbleView.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1568:
ChromeBubbleManagerFactory::GetForBrowserContext(browser_->profile())
On 2015/08/06 21:32:56, groby wrote:
> I'm wondering if ChromeBubbleManager shouldn't just be a TabStripModelObserver
-
> that would allow us to decouple entirely from BrowserView?

Sounds good. It would let us remove all the code from here.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:425:
views::View* PermissionBubbleViewViews::GetAnchorView() {
On 2015/08/06 21:32:56, groby wrote:
> Once we have a Cocoa version, too, let's see how much of this code can be
> shared. Getting the anchor view should be a fairly platform-neutral exercise,
> ideally.

Sounds good

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:472: void
PermissionBubbleViewViews::Hide() {
On 2015/08/06 21:32:56, groby wrote:
> Ideally, nobody calls Hide here any more - that should move onto the
> BubbleController. That's the ultimate goal of all this, most code just has a
> BubbleController and doesn't care about the specific type any more. Same goes
> for IsVisible, UpdateAnchorPosition, etc. 
> 
> We'll probably have to wait till the Cocoa version is ready, but
> PermissionBubbleView should really shrink quite a bit, containing only things
> highly specific to PermissionBubbles
> 
> [Edit: It looks like PermissionBubbleView doesn't have the Hide() API any
more,
> so isn't this code superfluous?)

Show, Hide and UpdateAnchorPosition are part of BubbleUI, so the
BubbleController calls them directly. I haven't looked at the exact
differences between show/hide/update in views and cocoa.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_delegate.cc (right):

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_delegate.cc:49: Browser*
PermissionBubbleDelegate::browser() {
On 2015/08/06 21:32:56, groby wrote:
> GetBrowser, please
> 
> If it's not inlined, it's not trivially cheap, and we shouldn't use lower case
> any more.

Ok, sounds good.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right):

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_delegate.h:20: //
PermissionBubbleDelegate:
On 2015/08/06 21:32:56, groby wrote:
> You mean BubbleDelegate?

Yes. Updated.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_delegate.h:32: // Pass
along to the BubbleUI.
On 2015/08/06 21:32:56, groby wrote:
> That comment doesn't make sense to me. Pass along what? 

Removed browser and updated comment.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_delegate.h:39:
PermissionBubbleView::Factory view_factory_;
On 2015/08/06 21:32:56, groby wrote:
> Really? Each delegate needs its own personal factory?

It's a callback to the constructor to allow injecting a test object. In
production code the value never changes and is defined in views or cocoa
to get the right bubble UI.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (left):

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:342:
content::BrowserThread::PostTask(
On 2015/08/06 21:32:56, groby wrote:
> We needed a thread transition here - are we now guaranteed that Finalize will
> always be called on the UI thread?

The bubble manager will always defer show to the UI thread.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:181:
UpdateBubble(); // TODO(hcarmona): HOW?
On 2015/08/06 21:32:56, groby wrote:
> Why not keep the old approach of calling Hide()?

Anyone who lets the bubble manager manage their bubble shouldn't be able
to tell the manager when to hide/show a bubble because that's stepping
on the bubble manager's responsibilities. I'm not sure what the best
approach for this case is.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:203:
ChromeBubbleManagerFactory::GetForBrowserContext(browser_->profile())
On 2015/08/06 21:32:56, groby wrote:
> Can we just call active_bubble_->Close() here?

Closing bubbles should go through the bubble manager because bubbles
don't know about the bubble manager.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:246: if
(!details.is_in_page ||
On 2015/08/06 21:32:56, groby wrote:
> That's a separate fix, no? I suppose we're just waiting to rebase when that CL
> landed?

Yes, reverted.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:309: for
(requests_iter = requests_.begin();
On 2015/08/06 21:32:56, groby wrote:
> C++11 please
> 
> for(PermissionBubbleRequest* request : requests_)
>   request->RequestFinished();

Awesome, I'll update the iterators.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:351:
ChromeBubbleManagerFactory::GetForBrowserContext(browser_->profile())
On 2015/08/06 21:32:56, groby wrote:
> Doesn't ShowBubble return an active_bubble_?

Yes... this code's broken. Fixed.

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:355:
NOTREACHED();
On 2015/08/06 21:32:56, groby wrote:
> Please don't handle NOTREACHED separately. If active_bubble_ truly never can
be
> set, just DCHECK before calling ShowBubble

Yes, this was debug code and it shouldn't exist.

https://codereview.chromium.org/1251633002/diff/80001/components/bubble/bubbl...
File components/bubble/bubble_controller.cc (right):

https://codereview.chromium.org/1251633002/diff/80001/components/bubble/bubbl...
components/bubble/bubble_controller.cc:30: if (!bubble_ui_)
On 2015/08/06 21:32:56, groby wrote:
> Wait, we don't seem to handle CloseOnHide? 

It's handled in the manager.

https://codereview.chromium.org/1251633002/diff/80001/components/bubble/bubbl...
File components/bubble/bubble_controller.h (right):

https://codereview.chromium.org/1251633002/diff/80001/components/bubble/bubbl...
components/bubble/bubble_controller.h:37: class BubbleController : public
base::SupportsWeakPtr<BubbleController> {
On 2015/08/06 20:34:10, Hector Carmona wrote:
> This class should be moved to an anonymous namespace in the
> bubble_manager.cc file because no other class should use it.

ChromeBubbleManager needs to know about it, can't move to
anonymous namespace

https://codereview.chromium.org/1251633002/diff/80001/components/bubble/bubbl...
File components/bubble/bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/80001/components/bubble/bubbl...
components/bubble/bubble_manager.cc:59: for (auto iter = controllers_.begin();
iter != controllers_.end(); ++iter) {
On 2015/08/06 21:32:56, groby wrote:
> I'd rather have a single ForMatchingBubbles that takes a callback...
> 
> i.e.
> 
> typedef Callback<void(BubbleController*)>
> void ForMatchingBubbles(void* context, BubbleCallback callback) {
> 
> I see the problem for CloseMatching, but if we simply 
> a) always created a list of matching bubbles first and then called the
callback
> on all of them
> b) Removed bubble controllers whenever the bubble closes
> 
> we'd have that addressed, too.

Sounds good, I'll update the code.

please use gerrit instead

https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/chrome_bubble_manager.cc File chrome/browser/ui/chrome_bubble_manager.cc (right): https://codereview.chromium.org/1251633002/diff/80001/chrome/browser/ui/chrome_bubble_manager.cc#newcode29 chrome/browser/ui/chrome_bubble_manager.cc:29: friend class content::WebContentsUserData<ChromeWebContentsObserver>; On 2015/08/07 02:12:58, Hector Carmona wrote: ...

5 years, 4 months ago (2015-08-07 20:34:28 UTC) #15

please use gerrit instead

https://codereview.chromium.org/1251633002/diff/100001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1251633002/diff/100001/chrome/browser/ui/browser.cc#newcode1763 chrome/browser/ui/browser.cc:1763: bubble_manager->SetBrowser(this); Hmmm. https://codereview.chromium.org/1251633002/diff/100001/chrome/browser/ui/chrome_bubble_manager.cc File chrome/browser/ui/chrome_bubble_manager.cc (right): https://codereview.chromium.org/1251633002/diff/100001/chrome/browser/ui/chrome_bubble_manager.cc#newcode14 chrome/browser/ui/chrome_bubble_manager.cc:14: namespace ...

5 years, 4 months ago (2015-08-07 23:02:28 UTC) #16

hcarmona

https://codereview.chromium.org/1251633002/diff/100001/chrome/browser/ui/website_settings/permission_bubble_manager.h File chrome/browser/ui/website_settings/permission_bubble_manager.h (right): https://codereview.chromium.org/1251633002/diff/100001/chrome/browser/ui/website_settings/permission_bubble_manager.h#newcode58 chrome/browser/ui/website_settings/permission_bubble_manager.h:58: void SetBrowser(Browser* browser) { browser_ = browser; } Remove ...

5 years, 4 months ago (2015-08-07 23:02:38 UTC) #17

hcarmona

Feedback! Still need to update design doc. https://codereview.chromium.org/1251633002/diff/100001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1251633002/diff/100001/chrome/browser/ui/browser.cc#newcode1763 chrome/browser/ui/browser.cc:1763: bubble_manager->SetBrowser(this); On ...

5 years, 4 months ago (2015-08-11 02:35:47 UTC) #18

hcarmona

hcarmona@chromium.org changed reviewers: + msw@chromium.org

5 years, 4 months ago (2015-08-11 19:03:46 UTC) #19

hcarmona

msw, PTAL, this is the bubble API from go/bubble-api. I'll be updating the design doc ...

5 years, 4 months ago (2015-08-11 19:03:47 UTC) #20

msw

On 2015/08/11 19:03:47, Hector Carmona wrote: > msw, > > PTAL, this is the bubble ...

5 years, 4 months ago (2015-08-12 17:37:21 UTC) #21

msw

I think tackling a permission bubble refactoring is too much for this CL, and it ...

5 years, 4 months ago (2015-08-13 03:37:24 UTC) #22

groby-ooo-7-16

Just replying to the main comment block: On 2015/08/13 03:37:24, msw wrote: > I think ...

5 years, 4 months ago (2015-08-13 05:58:45 UTC) #23

Just replying to the main comment block:

On 2015/08/13 03:37:24, msw wrote:
> I think tackling a permission bubble refactoring is too much for this CL, 

I suggested to Hector to do the implementation together with a bubble refactor
so things can be seen in context. If you're willing to approve a CL without any
direct consumers, there shouldn't be any issue with splitting things up.

> and it
> seems like you missed the Cocoa implementations (besides the testing aspect).

That's being worked on in a separate CL. 

> Perhaps that refactoring is a useful exercise for fleshing out some use cases,
> but I wouldn't want a CL to land that implements the new bubble manager
> component *and* makes such substantial changes to the already complex
permission
> bubble code (it makes this CL hard to review and would complicate a
> revert/reland, etc.)

Suggestion: 

1) Land components part
2) Land browser part
3 ) Land Views/Cocoa refactor. (Possibly separate, but that might mean one
platform is temporarily broken, no?)

> 
> I find the split of controller, delegate, and UI for a bubble potentially
overly
> complex (especially given that these are all subject to the manager), but
that's
> not necessarily wrong.

That's happening because each has actually a separate function that we'll need
as we refactor further bubbles.

* UI is the (platform-specific) UI implementation. It's deliberately separated
out so we can share logic across platforms.
* Controller is controlling the default behavior for bubbles - what to do on
focus loss, fullscreen switch, etc.
* Delegate is a set of customization points, should bubbles want to deviate from
that behavior. I'd prefer that approach over full implementation inheritance.
(Especially since many bubbles should work without having to implement a
delegate at all)

> I think that permission bubbles are an awful example of a bubble and I'm not
> sure they're a fair primary use case to target for a new bubble management
> component. 

Well, none of the bubbles are a "fair" case, because they're currently all
bespoke solutions :) They do exercise the delegate mechanic a little bit more,
so they make sense in that context.

Our next few targets are Foil, Passwords, and Translate. Maybe Translate makes a
good case for "normal" bubbles, but that should happen soon.

> Firstly, they have the rare (if not unique) property of faux
> tab-modality, which *really* ought to be replaced with simply closing on loss
of
> focus/activation (like the website settings bubble, bookmark bubble, and
> extension page actions).

IIRC, that's done. We don't actually do the "let's share a bubble across all
tabs" stuff any more.  We still _do_ preserve state for each tab, and
re-activate content on tab switch. 

> Faux tab modality isn't worth its code complexity; I've
> brought that up before, and I think it's time to press the matter, rather than
> design a bubble manager around that concept (using web_contents as context).

That's IIRC a UI demand for settings - maybe felt@ can chime in? Essentially, we
decided that we don't want to lose a permission request just because the user
switched tabs. We can take that up separately, but that's really a question for
UI/security folks. The goal of the refactor was to preserve existing behavior.

Also, there will be other contexts - I think it's foil that is essentially
browser-wide. I too would love to see bubbles only live attached to a web
content, and they go away if you switch to a different content. It would
simplify things a lot. Alas, we've made a lot of UI decision in a different
direction in the past. And I don't think we can undo all of them.

> Further, I didn't even understand what 'updating' a bubble meant until I saw
> that the permission bubbles close and reopen with new UI for updated
permission
> requests; that really shouldn't be a first-class bubble [manager] operation.
The
> permission bubble UI implementations themselves should support changes in
> layout/content/sizing rather than tearing down the bubble and bringing up a
new
> one at the manager level, or the permission bubble manager (to be renamed)
ought
> to close the bubble and bring up a new one through the usual functions.
> 
> I apologize for the amount of heavy comments here, perhaps I should have tried
> to engage more earlier in the design of the bubble api, or perhaps I'm not a
> fitting reviewer for this work.

You're probably one of the people most familiar with this entire thing - maybe
we could've had a simpler process if we had reached agreement earlier, but
switching reviewers seems to throw out the baby with the bathwater :) (Unless
you'd _like_ somebody else to review this, that is)

groby-ooo-7-16

A few more replies to individual comments. https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chrome_bubble_manager.cc File chrome/browser/ui/chrome_bubble_manager.cc (right): https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chrome_bubble_manager.cc#newcode68 chrome/browser/ui/chrome_bubble_manager.cc:68: this->AsWeakPtr())); Why ...

5 years, 4 months ago (2015-08-13 18:44:38 UTC) #24

A few more replies to individual comments.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.cc:68: this->AsWeakPtr()));
Why does this require WeakPtr? Can the bubble manager actually go away during
the updates?

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.h:16: class ChromeBubbleManager : public
BubbleManager,
On 2015/08/13 03:37:21, msw wrote:
> Should this also handle browser window activation/size/position/etc. changes?

None of these affect bubbles. (And hopefully never will). Or am I missing an
edge case here?

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager_factory.h (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager_factory.h:13: class
ChromeBubbleManagerFactory : public BrowserContextKeyedServiceFactory {
On 2015/08/13 03:37:21, msw wrote:
> I'm somewhat confused as to why we need a factory and a
> ChromeBubbleManagerInstance for each browser context if each bubble itself has
> some associated context. In the long term, do we plan to manage all bubbles
from
> a single manager? (maybe the manager would need some notion of context
hierarchy
> between tabs and browsers, etc. but that doesn't seem unreasonably hard)

The BrowserContext is the wider context across which bubbles are managed. Each
browser needs to do that separately, because the bubbles don't interact across
browsers. 

However, bubbles can interact across WebContents - the BubbleManager coordinates
that. The individual context for a bubble is whatever content it is associated
with - either the WebContents, or the BrowserWindow that the bubble is
associated with. 

So we'll never have a single manager for all BrowserContexts, because they never
interact.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:459: return
!(bubble_delegate_ && bubble_delegate_->IsMouseHovered());
On 2015/08/13 03:37:21, msw wrote:
> It seems odd that the mouse hovering might prevent a permission from being
> added/removed, or the bubble being automatically closed or something... Will
the
> UI attempt to update later (after the mouse moves away) or does this leave the
> UI perpetually stale?

That was always the case for permission bubbles, the reason being that we want
to avoid permissions being added just when the user is ready to press OK. 

And yes, they'll be shown later - but in the next bubble. (That's probably a
flaw w/ permissions, but it's in the existing bubbles as well)

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:274: for
(PermissionBubbleRequest* request : requests_)
On 2015/08/13 03:37:21, msw wrote:
> I wonder... if the bubble UI wasn't updated for a change in requests, and the
> user clicked allow/deny or just dismissed the bubble, does that decision apply
> to permissions that they never even saw? (ie. website asks for camera, I hover
> bubble, website asks for mic but UI doesn't update, I click deny for the
camera
> ui being shown, both camera *and mic* are denied... perhaps worse if I
accepted
> permissions I never saw!)

IIUC, these requests are stored in |queued_requests_| and so will not be marked
as denied. That's existing behavior, and is related to the postponing discussed
above.

FinalizeBubble used to pop up the next bubble with those requests. It's now
called DidClose, and invoked via CloseBubble.

@hcarmona: Maybe renaming FinalizeBubble is a refactor that should be landed
separately, since it doesn't affect functionality and will it make easier to
follow what's happening here.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:287: for
(requests_iter = requests_.begin();
On 2015/08/13 03:37:21, msw wrote:
> nit: update loop to foreach like above.

Ugh. I'd rather not do that here. We've already got enough changes :)

 Can we do a separate refactor CL to clean up the for loops, and do this before
this CL?

https://codereview.chromium.org/1251633002/diff/120001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3268: '../components/bubble.gypi:bubble',
On 2015/08/13 03:37:21, msw wrote:
> This doesn't appear to be necessary (at least not yet).

Don't we need this for the ChromeBubbleManager? It's based on the bubble
component, so we need to pull it in?

https://codereview.chromium.org/1251633002/diff/120001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/1251633002/diff/120001/components/OWNERS#newc...
components/OWNERS:29: per-file bubble.gypi=rouslan@chromium.org
Please keep Mike as owner. (Unless he'd rather not :)

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/OWNERS
File components/bubble/OWNERS (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/OWNE...
components/bubble/OWNERS:2: hcarmona@chromium.org
+msw, please?

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_controller.h (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_controller.h:20: // User did not interact with the
bubble, but changed tab.
On 2015/08/13 03:37:22, msw wrote:
> What's the value of distinguishing this from *_IGNORE? If there is appreciable
> value here, maybe we should do the same for switching browser windows, etc.?

There's potentially a different meaning to this - _IGNORE means you kept
interacting with the content, but ignored the request, TABSWITCH means you
switched content. Erring on the side of caution and spelling out separate
dismissal reasons for now, since we'll have metrics based on it.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_controller.h:26: // User selected the "Allow" option in
a bubble.
On 2015/08/13 03:37:22, msw wrote:
> Most bubbles won't have "Allow" and "Deny" options, those sound a little
overly
> specific for general responses; many bubbles may not even have any clear
> response options at all (and I assume that's fine). Still, consider using more
> generic terminology here (like views::DialogDelegate's Accept/Cancel, or
follow
> another more generic patterns like OK/Cancel, Accept/Deny)... Regardless,
maybe
> revise the comments to indicate that the bubble reports some sort of generic
> positive or negative response, not just the literals "Allow" and "Deny".

This definitely refers to the positive/negative reply, not the literal concepts.
Would removing the quotes in the comments be enough to make that clear?

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.cc:86: // Don't call close here to avoid
bubbles trying to show the next in a
On 2015/08/13 03:37:23, msw wrote:
> Ugh, horrifying... This is why BubbleManager should evolve to explicitly
handle
> requests to show multiple bubbles simultaneously or in succession (ie.
> ShowBubble queues the bubble if another is being shown, or potentially
overrides
> a less important bubble). You're attacking permission bubbles in this same
cl...
> is adding some queueing functionality now reasonable? (perhaps it would be...
> you'd need to handle changes of context, like tab switching, that should
> invalidate or delay bubbles queued for that context, etc.)

The problem is that bubbles will be shown _in response_ to a Close() call. Which
means queueing alone is not enough. (I suppose we could make all calls to Show()
into a PostTask, which would fix this immediate issue - but I'm not sure it
won't be asking for more pain later.)

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.h:27: void CloseBubble(BubbleReference bubble);
On 2015/08/13 03:37:23, msw wrote:
> q: should this class offer a HideBubble function that avoids closing it?

I don't think it should. It's bad enough that PermissionBubbles abuse the hiding
pattern to achieve some sort of tab-modality, but I really don't want to
encourage that pattern.

A bubble should either get a user interaction, or be dismissed. There really
isn't a good case to be made for "temporarily suspended"

msw

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chrome_bubble_manager.h File chrome/browser/ui/chrome_bubble_manager.h (right): https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chrome_bubble_manager.h#newcode16 chrome/browser/ui/chrome_bubble_manager.h:16: class ChromeBubbleManager : public BubbleManager, On 2015/08/13 18:44:38, groby ...

5 years, 4 months ago (2015-08-13 19:18:58 UTC) #25

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.h:16: class ChromeBubbleManager : public
BubbleManager,
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:21, msw wrote:
> > Should this also handle browser window activation/size/position/etc.
changes?
> 
> None of these affect bubbles. (And hopefully never will). Or am I missing an
> edge case here?

If the bubbles don't close on deactivate, they potentially need to update on
anchor-window size/position changes. Views bubbles should already do this; I
don't know about Cocoa. There's also a possible edge case of the anchor window
changing while the bubble is activated (perhaps by 3rd party software?). The
more common cases here are possibly, but not necessarily, something the manager
should handle.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager_factory.h (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager_factory.h:13: class
ChromeBubbleManagerFactory : public BrowserContextKeyedServiceFactory {
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:21, msw wrote:
> > I'm somewhat confused as to why we need a factory and a
> > ChromeBubbleManagerInstance for each browser context if each bubble itself
has
> > some associated context. In the long term, do we plan to manage all bubbles
> from
> > a single manager? (maybe the manager would need some notion of context
> hierarchy
> > between tabs and browsers, etc. but that doesn't seem unreasonably hard)
> 
> The BrowserContext is the wider context across which bubbles are managed. Each
> browser needs to do that separately, because the bubbles don't interact across
> browsers. 
> 
> However, bubbles can interact across WebContents - the BubbleManager
coordinates
> that. The individual context for a bubble is whatever content it is associated
> with - either the WebContents, or the BrowserWindow that the bubble is
> associated with. 
> 
> So we'll never have a single manager for all BrowserContexts, because they
never
> interact. 

I'm not sure I understand what sort of interaction bubbles should actually have
across web-contents. Having a manager for each browser window might be
reasonable, but it might also be unnecessary.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:459: return
!(bubble_delegate_ && bubble_delegate_->IsMouseHovered());
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:21, msw wrote:
> > It seems odd that the mouse hovering might prevent a permission from being
> > added/removed, or the bubble being automatically closed or something... Will
> the
> > UI attempt to update later (after the mouse moves away) or does this leave
the
> > UI perpetually stale?
> 
> That was always the case for permission bubbles, the reason being that we want
> to avoid permissions being added just when the user is ready to press OK. 
> 
> And yes, they'll be shown later - but in the next bubble. (That's probably a
> flaw w/ permissions, but it's in the existing bubbles as well)

Okay, I hope we have tests that adding a permission while bubble is hovered
doesn't let that 'stale' bubble accept/deny the delayed permission request.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:287: for
(requests_iter = requests_.begin();
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:21, msw wrote:
> > nit: update loop to foreach like above.
> 
> Ugh. I'd rather not do that here. We've already got enough changes :)
> 
>  Can we do a separate refactor CL to clean up the for loops, and do this
before
> this CL?

It's a nit; definitely not required, and welcome to be addressed separately.

https://codereview.chromium.org/1251633002/diff/120001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3268: '../components/bubble.gypi:bubble',
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:21, msw wrote:
> > This doesn't appear to be necessary (at least not yet).
> 
> Don't we need this for the ChromeBubbleManager? It's based on the bubble
> component, so we need to pull it in?

That should be covered by chrome_browser_ui.gypi (not here)

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_controller.h (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_controller.h:20: // User did not interact with the
bubble, but changed tab.
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:22, msw wrote:
> > What's the value of distinguishing this from *_IGNORE? If there is
appreciable
> > value here, maybe we should do the same for switching browser windows, etc.?
> 
> There's potentially a different meaning to this - _IGNORE means you kept
> interacting with the content, but ignored the request, TABSWITCH means you
> switched content. Erring on the side of caution and spelling out separate
> dismissal reasons for now, since we'll have metrics based on it.

Okay, reasonable, but perhaps better generalized as a context switch (there's no
notion of a tab in this component). Also aren't clicking away from the browser
window, navigating the tab, closing the tab, etc. also effectively context
switches? Tab switching while a permission bubble is shown might be more
appropriate for the permission code to track. I'm not too adamant about this
point, if tab switching really is an important metric to measure and relevant to
all bubbles (but I doubt it).

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_controller.h:26: // User selected the "Allow" option in
a bubble.
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:22, msw wrote:
> > Most bubbles won't have "Allow" and "Deny" options, those sound a little
> overly
> > specific for general responses; many bubbles may not even have any clear
> > response options at all (and I assume that's fine). Still, consider using
more
> > generic terminology here (like views::DialogDelegate's Accept/Cancel, or
> follow
> > another more generic patterns like OK/Cancel, Accept/Deny)... Regardless,
> maybe
> > revise the comments to indicate that the bubble reports some sort of generic
> > positive or negative response, not just the literals "Allow" and "Deny".
> 
> This definitely refers to the positive/negative reply, not the literal
concepts.
> Would removing the quotes in the comments be enough to make that clear?

That might be okay, but allow/deny is much more permission inspired than the
options I outlined above; I'd vote for generic Accept/Cancel dialog terminology.
(I hope someday bubbles and dialogs will share much more code)

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_controller.h:33: // BubbleController is responisble for
the lifetime of the delegate and it's UI.
nit: responsible spelling.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.cc:86: // Don't call close here to avoid
bubbles trying to show the next in a
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:23, msw wrote:
> > Ugh, horrifying... This is why BubbleManager should evolve to explicitly
> handle
> > requests to show multiple bubbles simultaneously or in succession (ie.
> > ShowBubble queues the bubble if another is being shown, or potentially
> overrides
> > a less important bubble). You're attacking permission bubbles in this same
> cl...
> > is adding some queueing functionality now reasonable? (perhaps it would
be...
> > you'd need to handle changes of context, like tab switching, that should
> > invalidate or delay bubbles queued for that context, etc.)
> 
> The problem is that bubbles will be shown _in response_ to a Close() call.
Which
> means queueing alone is not enough. (I suppose we could make all calls to
Show()
> into a PostTask, which would fix this immediate issue - but I'm not sure it
> won't be asking for more pain later.)

Okay, this pattern (weak_erase first then Close later) isn't such a big deal,
but permission bubbles really are awful.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.h:27: void CloseBubble(BubbleReference bubble);
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:23, msw wrote:
> > q: should this class offer a HideBubble function that avoids closing it?
> 
> I don't think it should. It's bad enough that PermissionBubbles abuse the
hiding
> pattern to achieve some sort of tab-modality, but I really don't want to
> encourage that pattern.
> 
> A bubble should either get a user interaction, or be dismissed. There really
> isn't a good case to be made for "temporarily suspended"

I agree, I hope we can eliminate hiding altogether, but wondered about API
consistency (Users can Hide bubbles via the controller anyway).

hcarmona

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chrome_bubble_manager.cc File chrome/browser/ui/chrome_bubble_manager.cc (right): https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chrome_bubble_manager.cc#newcode93 chrome/browser/ui/chrome_bubble_manager.cc:93: details.type == content::NAVIGATION_TYPE_SAME_PAGE || On 2015/08/13 03:37:21, msw wrote: ...

5 years, 4 months ago (2015-08-13 20:50:36 UTC) #26

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.cc:93: details.type ==
content::NAVIGATION_TYPE_SAME_PAGE ||
On 2015/08/13 03:37:21, msw wrote:
> q: did you mean != here?

I will revert any changes to this logic from what was on the permission
bubble manager. If we change it, it should be a separate CL.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:174:
(bubble_manager && bubble_manager->ShouldUpdateBubble(active_bubble_));
On 2015/08/13 03:37:21, msw wrote:
> q: if the bubble_manager is null, that seems to indicate that there's no
browser
> hosting the web_contents, does that mean that the tab has been closed or could
> it just be a background/extension page or something? I'm wondering if
> |can_erase| should be true if there is no bubble manager. I guess I'm also
> wondering when the bubble manager might not exist or be found... what happens
to
> those bubbles below that aren't updated or closed?

This happens when a tab is closed. The bubble manager is watching the
web contents and it will close the bubbles when the web contents are
destroyed.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:205: if
(BubbleManager* bubble_manager = GetBubbleManager())
On 2015/08/13 03:37:21, msw wrote:
> Again, when could there be no bubble manager and what happens to these
abandoned
> unclosed bubbles? This shouldn't be possible with good bookkeeping.

Yes, this seems like a weird state that we should try to eliminate.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:274: for
(PermissionBubbleRequest* request : requests_)
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:21, msw wrote:
> > I wonder... if the bubble UI wasn't updated for a change in requests, and
the
> > user clicked allow/deny or just dismissed the bubble, does that decision
apply
> > to permissions that they never even saw? (ie. website asks for camera, I
hover
> > bubble, website asks for mic but UI doesn't update, I click deny for the
> camera
> > ui being shown, both camera *and mic* are denied... perhaps worse if I
> accepted
> > permissions I never saw!)
> 
> IIUC, these requests are stored in |queued_requests_| and so will not be
marked
> as denied. That's existing behavior, and is related to the postponing
discussed
> above.
> 
> FinalizeBubble used to pop up the next bubble with those requests. It's now
> called DidClose, and invoked via CloseBubble.
> 
> @hcarmona: Maybe renaming FinalizeBubble is a refactor that should be landed
> separately, since it doesn't affect functionality and will it make easier to
> follow what's happening here.

Sounds good, we can have other CLs to refactor first.

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:287: for
(requests_iter = requests_.begin();
On 2015/08/13 18:44:38, groby wrote:
> On 2015/08/13 03:37:21, msw wrote:
> > nit: update loop to foreach like above.
> 
> Ugh. I'd rather not do that here. We've already got enough changes :)
> 
>  Can we do a separate refactor CL to clean up the for loops, and do this
before
> this CL?

We can have a separate refactor CL. This particular loop cannot be
changed because |RequestFinished| will clean up requests.

https://codereview.chromium.org/1251633002/diff/120001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3268: '../components/bubble.gypi:bubble',
On 2015/08/13 19:18:58, msw wrote:
> On 2015/08/13 18:44:38, groby wrote:
> > On 2015/08/13 03:37:21, msw wrote:
> > > This doesn't appear to be necessary (at least not yet).
> > 
> > Don't we need this for the ChromeBubbleManager? It's based on the bubble
> > component, so we need to pull it in?
> 
> That should be covered by chrome_browser_ui.gypi (not here)

Is there a tool to verify these?

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_controller.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_controller.cc:52:
weak_ptr_factory_.InvalidateWeakPtrs();
On 2015/08/13 03:37:22, msw wrote:
> Huh? Is this instance going to be torn down now or something?

Permission bubbles want to show another bubble when this one closes. We
invalidate the weak pointers so that the permission bubble queue knows
that it can now show another bubble.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.cc:29: return bubble_ref;
On 2015/08/13 03:37:23, msw wrote:
> By exposing a BubbleReference, don't you allow users to directly call public
> BubbleController functions, like Show/Hide/Close? Wouldn't that potentially
> interfere with your attempt to manage these bubbles? (eg.
> BubbleManager::CloseBubble assumes that the bubble wasn't yet hidden or
closed)

True, do we have an existing pattern that would allow us to give a
reference that's just a token so we can find the bubble with the ability
to invalidate later?

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.cc:41: // Dancing around this because some
bubbles will chain another on close.
On 2015/08/13 03:37:23, msw wrote:
> I'm not quite sure what you're saying here. Does closing one bubble cause
> another bubble to close sometimes? (when? maybe give an example?) Can you
revise
> this comment to explain more clearly what you mean by dancing and chaining? 

Permission bubbles will sometimes show another bubble on close which
interferes with iterating.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.cc:94: ++iter;
On 2015/08/13 03:37:23, msw wrote:
> Shouldn't this also be called if a bubble matches the context, but fails the
> inner check (ie. returns false for ShouldClose and close_option==ALLOW_HIDE),
> otherwise this will loop on that iter infinitely...

Yes, it's outside both checks.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.h:46: CloseOption close_option,
On 2015/08/13 03:37:23, msw wrote:
> nit: a simple |allow_hide| or |force_close| bool (plus an optional function
> comment) might suffice here, insead of adding the CloseOption enum.

Using the enum makes it more clear what will happen when this function
is called without having to refer back to documentation.

msw

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/website_settings/permission_bubble_manager.cc File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right): https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/website_settings/permission_bubble_manager.cc#newcode174 chrome/browser/ui/website_settings/permission_bubble_manager.cc:174: (bubble_manager && bubble_manager->ShouldUpdateBubble(active_bubble_)); On 2015/08/13 20:50:35, Hector Carmona wrote: ...

5 years, 4 months ago (2015-08-13 21:41:37 UTC) #27

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:174:
(bubble_manager && bubble_manager->ShouldUpdateBubble(active_bubble_));
On 2015/08/13 20:50:35, Hector Carmona wrote:
> On 2015/08/13 03:37:21, msw wrote:
> > q: if the bubble_manager is null, that seems to indicate that there's no
> browser
> > hosting the web_contents, does that mean that the tab has been closed or
could
> > it just be a background/extension page or something? I'm wondering if
> > |can_erase| should be true if there is no bubble manager. I guess I'm also
> > wondering when the bubble manager might not exist or be found... what
happens
> to
> > those bubbles below that aren't updated or closed?
> 
> This happens when a tab is closed. The bubble manager is watching the
> web contents and it will close the bubbles when the web contents are
> destroyed.

So if |bubble_manager| is null, should |can_erase| be true or false?

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_controller.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_controller.cc:52:
weak_ptr_factory_.InvalidateWeakPtrs();
On 2015/08/13 20:50:35, Hector Carmona wrote:
> On 2015/08/13 03:37:22, msw wrote:
> > Huh? Is this instance going to be torn down now or something?
> 
> Permission bubbles want to show another bubble when this one closes. We
> invalidate the weak pointers so that the permission bubble queue knows
> that it can now show another bubble.

The permission code should attempt to show another bubble when the response is
received, not when the bubble object is closed. That might be effectively the
same here, so this workaround might still be necessary, but maybe not.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.cc:29: return bubble_ref;
On 2015/08/13 20:50:35, Hector Carmona wrote:
> On 2015/08/13 03:37:23, msw wrote:
> > By exposing a BubbleReference, don't you allow users to directly call public
> > BubbleController functions, like Show/Hide/Close? Wouldn't that potentially
> > interfere with your attempt to manage these bubbles? (eg.
> > BubbleManager::CloseBubble assumes that the bubble wasn't yet hidden or
> closed)
> 
> True, do we have an existing pattern that would allow us to give a
> reference that's just a token so we can find the bubble with the ability
> to invalidate later?

Maybe remove the controller functions or make them private, only let users call
BubbleManager::CloseBubble, etc.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.cc:41: // Dancing around this because some
bubbles will chain another on close.
On 2015/08/13 20:50:35, Hector Carmona wrote:
> On 2015/08/13 03:37:23, msw wrote:
> > I'm not quite sure what you're saying here. Does closing one bubble cause
> > another bubble to close sometimes? (when? maybe give an example?) Can you
> revise
> > this comment to explain more clearly what you mean by dancing and chaining? 
> 
> Permission bubbles will sometimes show another bubble on close which
> interferes with iterating.

Acknowledged; maybe rephrase the comment to make that clearer.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.cc:94: ++iter;
On 2015/08/13 20:50:35, Hector Carmona wrote:
> On 2015/08/13 03:37:23, msw wrote:
> > Shouldn't this also be called if a bubble matches the context, but fails the
> > inner check (ie. returns false for ShouldClose and
close_option==ALLOW_HIDE),
> > otherwise this will loop on that iter infinitely...
> 
> Yes, it's outside both checks.

Oh duh, you're right, sorry.

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
File components/bubble/bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/120001/components/bubble/bubb...
components/bubble/bubble_manager.h:46: CloseOption close_option,
On 2015/08/13 20:50:35, Hector Carmona wrote:
> On 2015/08/13 03:37:23, msw wrote:
> > nit: a simple |allow_hide| or |force_close| bool (plus an optional function
> > comment) might suffice here, insead of adding the CloseOption enum.
> 
> Using the enum makes it more clear what will happen when this function
> is called without having to refer back to documentation.

Acknowledged.

hcarmona

Applied feedback from Thursday's meeting and addressed comments. The permission bubble's changes have been removed ...

5 years, 4 months ago (2015-08-15 02:03:21 UTC) #28

hcarmona

PTAL. This has both Views and Cocoa changes. Running trybots to see if anything is ...

5 years, 4 months ago (2015-08-18 02:25:02 UTC) #29

msw

https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/browser.cc#newcode813 chrome/browser/ui/browser.cc:813: bubble_manager_.FullscreenToggle(); nit: name this WindowFullscreenStateChanged for consistency. https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/browser.cc#newcode813 chrome/browser/ui/browser.cc:813: ...

5 years, 4 months ago (2015-08-18 17:26:20 UTC) #30

hcarmona

Applied feedback. https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/browser.cc#newcode813 chrome/browser/ui/browser.cc:813: bubble_manager_.FullscreenToggle(); On 2015/08/18 17:26:18, msw wrote: > ...

5 years, 4 months ago (2015-08-18 23:08:49 UTC) #31

msw

https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/browser.cc#newcode996 chrome/browser/ui/browser.cc:996: bubble_manager_.TabDetached(); On 2015/08/18 23:08:47, Hector Carmona wrote: > On ...

5 years, 4 months ago (2015-08-19 00:18:11 UTC) #32

https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:996: bubble_manager_.TabDetached();
On 2015/08/18 23:08:47, Hector Carmona wrote:
> On 2015/08/18 17:26:18, msw wrote:
> > Instead of adding logic here, can the ChromeBubbleManager also be a
> > TabStripModelObserver, like Browser?
> 
> Making it into a TabStipModelObserver seems like overkill just for
> TabDetached and TabDeactivated since we still need to forward the
> fullscreen event to the bubble manager.

I don't think it's overkill, especially when it reduces the code you're adding
to browser.cc. Ideally, we wouldn't need any bubble manager logic here, and I'm
sure others would agree that making ChromeBubbleManager a TabStripModelObserver
is a step in the right direction.

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
File components/bubble/bubble_controller.h (right):

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_controller.h:10: #include
"components/bubble/bubble_delegate.h"
On 2015/08/18 23:08:48, Hector Carmona wrote:
> On 2015/08/18 17:26:19, msw wrote:
> > nit: remove this, forward-declare BubbleDelegate, like BubbleUI.
> 
> Need BubbleCloseReason.

Acknowledged.

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_controller.h:15: class BubbleController {
On 2015/08/18 23:08:48, Hector Carmona wrote:
> On 2015/08/18 17:26:19, msw wrote:
> > Make this a SupportsWeakPtr subclass, remove the factory and AsWeakPtr.
> 
> I didn't use SupportsWeakPtr because it's necessary to invalidate the
> weak references before cleaning up the delegate. This is for cases where
> a bubble will be chained, the delegate chaining needs to know that its
> bubble reference is gone before showing another bubble.

So the problem is that SupportsWeakPtr's "internal::WeakReferenceOwner
weak_reference_owner_;" member is only invalidated when the object is destroyed?
If so, I guess this is okay (for now)... But better handling of chaining might
make more sense for this and other reasons.

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
File components/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_delegate.h:56: // IMPORTANT: DO NOT SHOW OR HIDE OTHER
BUBBLES IN THIS FUNCTION.
On 2015/08/18 23:08:48, Hector Carmona wrote:
> On 2015/08/18 17:26:19, msw wrote:
> > I wonder how tough it would be to support this, and if doing so would make
the
> > API better/simpler for users. We may want Show to actually enqueue bubbles,
> and
> > to only show queued bubbles after the close loops have finished... Maybe add
a
> > CHECK in BubbleManager for now, ensuring that ShowBubble/CloseBubble/etc.
> isn't
> > called during CloseBubble/CloseAllBubbles/ShouldClose?
> 
> It doesn't sound very hard to fix this. I like your idea of adding a
> queue for the requests that come in while closing bubbles. Separate CL
> or add the queuing mechanism as part of this CL?

I think it would be good to tackle in this CL, unless it really proves to be too
challenging. It would remove some of our assumptions about ordering and should
allow minor nice fixes like the SupportsWeakPtr usage.

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
File components/bubble/bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_manager.cc:59: // Remove the controllers before
attempting to delete because some
On 2015/08/18 23:08:48, Hector Carmona wrote:
> On 2015/08/18 17:26:19, msw wrote:
> > What happens on BubbleManager destruction (browser window closing) with a
> > chaining bubble open? I don't see any explicit handling. The scoped vector
of
> > controllers will be destroyed normally and BubbleManager::ShowBubble will
get
> > called during destruction... We should cover this in automated testing.
> 
> Yes, I'll create a test for this scenario.
> 
> Added TODO item in destructor. This should be fixed with queuing of
> pending show requests in ShouldHide. We would end up with a queue of
> pending bubbles that we can destroy without showing, so they won't add
> more bubbles.

sgtm, thanks.

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_manager.cc:74: void
BubbleManager::ShowBubbleUI(base::WeakPtr<BubbleController> controller) {
On 2015/08/18 23:08:48, Hector Carmona wrote:
> On 2015/08/18 17:26:20, msw wrote:
> > Why not just inline this?
> 
> Can't inline virtual functions. These functions are virtual because the
> ChromeBubbleManager wants to assert that they are run in the UI thread.

Sorry, what I meant is that we shouldn't define a BubbleManager::ShowBubbleUI
function at all, BubbleManager::ShowBubble should call |controller->Show()|
directly instead. Ditto for the functions below (remove them and call the
controller functions directly in the code above).

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
File components/bubble/bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_manager.h:11: #include
"components/bubble/bubble_delegate.h"
On 2015/08/18 23:08:48, Hector Carmona wrote:
> On 2015/08/18 17:26:20, msw wrote:
> > nit: forward declare BubbleDelegate instead.
> 
> Including b/c of enum BubbleCloseReason.

Acknowledged.

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_manager.h:36: void UpdateAllBubbleAnchors();
On 2015/08/18 23:08:48, Hector Carmona wrote:
> On 2015/08/18 17:26:20, msw wrote:
> > I think this should also be protected (that should be okay after making
> > ChromeBubbleManager a tab strip observer).
> 
> Still needs to be public because we need to notify the BubbleManager
> when the toolbar slides in and out on the mac.

Acknowledged.

https://codereview.chromium.org/1251633002/diff/180001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1251633002/diff/180001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2823: '../components/bubble.gypi:bubble',
On 2015/08/18 23:08:49, Hector Carmona wrote:
> On 2015/08/18 17:26:20, msw wrote:
> > Does this belong in the ultimate ('OS!="android" and OS!="ios"') dep block,
> > since this will be views+cocoa specific (excluded on android/ios).
> 
> This should be views/cocoa independent. ChromeBubbleManager is the part
> that's specific to views and cocoa.

Right, but it shouldn't be expressed as a dependency of the chrome browser ui
code on Android if it won't be used there. It shouldn't be compiled/linked into
Chrome on platforms where it's not used.

groby-ooo-7-16

A few tiny remarks https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/browser.cc#newcode996 chrome/browser/ui/browser.cc:996: bubble_manager_.TabDetached(); On 2015/08/19 00:18:11, msw ...

5 years, 4 months ago (2015-08-19 00:52:01 UTC) #33

A few tiny remarks

https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1251633002/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:996: bubble_manager_.TabDetached();
On 2015/08/19 00:18:11, msw wrote:
> On 2015/08/18 23:08:47, Hector Carmona wrote:
> > On 2015/08/18 17:26:18, msw wrote:
> > > Instead of adding logic here, can the ChromeBubbleManager also be a
> > > TabStripModelObserver, like Browser?
> > 
> > Making it into a TabStipModelObserver seems like overkill just for
> > TabDetached and TabDeactivated since we still need to forward the
> > fullscreen event to the bubble manager.
> 
> I don't think it's overkill, especially when it reduces the code you're adding
> to browser.cc. Ideally, we wouldn't need any bubble manager logic here, and
I'm
> sure others would agree that making ChromeBubbleManager a
TabStripModelObserver
> is a step in the right direction.

+1 on that. browser.cc is currently some catch-all repository. The more we can
remove there, the happier the world is.

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
File components/bubble/bubble_controller.h (right):

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_controller.h:15: class BubbleController {
On 2015/08/19 00:18:11, msw wrote:
> On 2015/08/18 23:08:48, Hector Carmona wrote:
> > On 2015/08/18 17:26:19, msw wrote:
> > > Make this a SupportsWeakPtr subclass, remove the factory and AsWeakPtr.
> > 
> > I didn't use SupportsWeakPtr because it's necessary to invalidate the
> > weak references before cleaning up the delegate. This is for cases where
> > a bubble will be chained, the delegate chaining needs to know that its
> > bubble reference is gone before showing another bubble.
> 
> So the problem is that SupportsWeakPtr's "internal::WeakReferenceOwner
> weak_reference_owner_;" member is only invalidated when the object is
destroyed?
> If so, I guess this is okay (for now)... But better handling of chaining might
> make more sense for this and other reasons.

There's also the point that WeakPtrFactory is _by far_ the more prevalent
pattern in Chrome. (5:1 or so)

But: If this stays a WeakPtrFactory, AsWeakPtr should go. WPF is for use of
WeakPtr _inside_ the class only. (That includes things like binding and passing
to delegates, but no external users should request WeakPtrs)

hcarmona

Applied feedback and added tests. I'll update the ChromeBubbleManager to be a TabStipModelObserver on Monday. ...

5 years, 4 months ago (2015-08-20 02:34:31 UTC) #34

Applied feedback and added tests.

I'll update the ChromeBubbleManager to be a TabStipModelObserver on
Monday. (OOO Thurs + Fri)

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
File components/bubble/bubble_controller.h (right):

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_controller.h:15: class BubbleController {
On 2015/08/19 00:52:01, groby wrote:
> On 2015/08/19 00:18:11, msw wrote:
> > On 2015/08/18 23:08:48, Hector Carmona wrote:
> > > On 2015/08/18 17:26:19, msw wrote:
> > > > Make this a SupportsWeakPtr subclass, remove the factory and AsWeakPtr.
> > > 
> > > I didn't use SupportsWeakPtr because it's necessary to invalidate the
> > > weak references before cleaning up the delegate. This is for cases where
> > > a bubble will be chained, the delegate chaining needs to know that its
> > > bubble reference is gone before showing another bubble.
> > 
> > So the problem is that SupportsWeakPtr's "internal::WeakReferenceOwner
> > weak_reference_owner_;" member is only invalidated when the object is
> destroyed?
> > If so, I guess this is okay (for now)... But better handling of chaining
might
> > make more sense for this and other reasons.
> 
> There's also the point that WeakPtrFactory is _by far_ the more prevalent
> pattern in Chrome. (5:1 or so)
> 
> But: If this stays a WeakPtrFactory, AsWeakPtr should go. WPF is for use of
> WeakPtr _inside_ the class only. (That includes things like binding and
passing
> to delegates, but no external users should request WeakPtrs)
> 

We now have better chaining, so we can use SupportsWeakPtr.

Now that this SupportsWeakPtr, is it ok to hand out the WeakPtrs as a
BubbleReference or should we avoid that?

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
File components/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_delegate.h:56: // IMPORTANT: DO NOT SHOW OR HIDE OTHER
BUBBLES IN THIS FUNCTION.
On 2015/08/19 00:18:11, msw wrote:
> On 2015/08/18 23:08:48, Hector Carmona wrote:
> > On 2015/08/18 17:26:19, msw wrote:
> > > I wonder how tough it would be to support this, and if doing so would make
> the
> > > API better/simpler for users. We may want Show to actually enqueue
bubbles,
> > and
> > > to only show queued bubbles after the close loops have finished... Maybe
add
> a
> > > CHECK in BubbleManager for now, ensuring that ShowBubble/CloseBubble/etc.
> > isn't
> > > called during CloseBubble/CloseAllBubbles/ShouldClose?
> > 
> > It doesn't sound very hard to fix this. I like your idea of adding a
> > queue for the requests that come in while closing bubbles. Separate CL
> > or add the queuing mechanism as part of this CL?
> 
> I think it would be good to tackle in this CL, unless it really proves to be
too
> challenging. It would remove some of our assumptions about ordering and should
> allow minor nice fixes like the SupportsWeakPtr usage.

Done.

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
File components/bubble/bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubb...
components/bubble/bubble_manager.cc:74: void
BubbleManager::ShowBubbleUI(base::WeakPtr<BubbleController> controller) {
On 2015/08/19 00:18:11, msw wrote:
> On 2015/08/18 23:08:48, Hector Carmona wrote:
> > On 2015/08/18 17:26:20, msw wrote:
> > > Why not just inline this?
> > 
> > Can't inline virtual functions. These functions are virtual because the
> > ChromeBubbleManager wants to assert that they are run in the UI thread.
> 
> Sorry, what I meant is that we shouldn't define a BubbleManager::ShowBubbleUI
> function at all, BubbleManager::ShowBubble should call |controller->Show()|
> directly instead. Ditto for the functions below (remove them and call the
> controller functions directly in the code above).

Done. Moved the UI thread check to here.

msw

This is looking much better; nice tests. https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubble_controller.h File components/bubble/bubble_controller.h (right): https://codereview.chromium.org/1251633002/diff/160001/components/bubble/bubble_controller.h#newcode15 components/bubble/bubble_controller.h:15: class BubbleController ...

5 years, 4 months ago (2015-08-21 01:48:31 UTC) #35

hcarmona

https://codereview.chromium.org/1251633002/diff/240001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/1251633002/diff/240001/components/BUILD.gn#newcode25 components/BUILD.gn:25: "//components/bookmarks/test", On 2015/08/21 01:48:30, msw wrote: > Add //components/bubble ...

5 years, 3 months ago (2015-08-25 02:13:37 UTC) #36

https://codereview.chromium.org/1251633002/diff/240001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1251633002/diff/240001/components/BUILD.gn#ne...
components/BUILD.gn:25: "//components/bookmarks/test",
On 2015/08/21 01:48:30, msw wrote:
> Add //components/bubble here

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble.gypi
File components/bubble.gypi (right):

https://codereview.chromium.org/1251633002/diff/240001/components/bubble.gypi...
components/bubble.gypi:6: 'targets': [
On 2015/08/21 01:48:30, msw wrote:
> Add a target for the unit tests here (in gyp) too.

Most other tests in components don't do this. Is there an advantage in
deviating from the other tests in components?

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
File components/bubble/bubble_controller.h (right):

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_controller.h:15: class BubbleController : public
base::SupportsWeakPtr<BubbleController> {
On 2015/08/21 01:48:30, msw wrote:
> If users hold WeakPtrs to BubbleControllers, they'll also need to keep (weak)
> pointers to the BubbleManager (or the Browser), otherwise they can't do
anything
> with the reference. Perhaps BubbleController should have a member pointing to
> the manager for convenience, or (for even more user convenience) perhaps the
> controller should have public versions of show/update/close that calls through
> to its bubble manager.
> 
> These are just optional ideas to make BubbleReferences more self-contained for
> users. It's not critical yet, but worth considering as existing bubbles start
> using the new component.

I added |CloseBubble| so it can be called on the bubble reference, but I
don't think we want to add |CloseAllBubbles| or |UpdateAllBubbleAnchors|
to a single bubble reference.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
File components/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_delegate.h:14: enum BubbleCloseReason {
On 2015/08/21 01:48:30, msw wrote:
> nit: maybe this belongs in its own file (that would also alleviate the need to
> include bubble_delegate.h in so many other headers)

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_delegate.h:30: // Window has entered fullscreen mode.
Will also be called for immersive
On 2015/08/21 01:48:30, msw wrote:
> nit: The parent window has entered or exited

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_delegate.h:32: BUBBLE_CLOSE_FULLSCREEN_TOGGLED,
On 2015/08/21 01:48:30, msw wrote:
> I wonder if this should be used for other anchor window bounds changes. If a
> bubble isn't closing for loss of focus, then the parent/anchor window bounds
can
> be manipulated (and the bubble anchor may need updating). Views bubbles should
> already handle those anchor window updates with a WidgetObserver, but we might
> want to add a notification for that here (or expand this one to cover broader
> anchor window bounds changes). This may not be immediately necessary if we
rely
> on per-platform behavior, but then that might be worth noting in a comment.

I'm a bit confused by what exactly this comment means... I don't think that
resizing a window should send a close bubble event. A bubble should
close itself because of a loss of focus at that point, rather than rely
on a window resize event.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_delegate.h:50: BubbleDelegate() {}
On 2015/08/21 01:48:30, msw wrote:
> nit: if you move the dtor definition, you may as well move this too.

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_delegate.h:51: virtual ~BubbleDelegate() {}
On 2015/08/21 01:48:30, msw wrote:
> nit: this virtual dtor should be defined in the cc file, i think.

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
File components/bubble/bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager.cc:24: if (!can_show_bubbles_) {
On 2015/08/21 01:48:30, msw wrote:
> As I eluded to with my suggestion in the header, consider: 
>   if (!can_show_bubbles_) {
>     show_queue_->push_back(controller.Pass());
>   } else {
>     controller->Show();
>     controllers_.push_back(controller.Pass());
>   }
> 
> The close functions can cache/restore the value of |can_show_bubbles_| to
handle
> the dtor case (or you can use a separate |in_dtor_| flag).

Acknowledged.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager.cc:53: ShowPendingBubbles();
On 2015/08/21 01:48:31, msw wrote:
> I don't think this call would be necessary... If the bubble didn't close, then
> we shouldn't get anything in the queue, maybe DCHECK nothing was added there.

A bubble could theoretically trigger another bubble and not close.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager.cc:77: if ((*iter)->ShouldClose(reason)) {
On 2015/08/21 01:48:30, msw wrote:
> optionally consider: iter = (*iter)->ShouldClose(reason) ?
> controllers_.erase(iter) : iter + 1;

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
File components/bubble/bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager.h:30: // Close a specific bubble with a given
reason.
On 2015/08/21 01:48:31, msw wrote:
> nit:  // Notify a specific bubble of an event that might trigger close.

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager.h:40: // Notify all bubbles of a potential
close event.
On 2015/08/21 01:48:31, msw wrote:
>   // Notify all bubbles of an event that might trigger close.

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager.h:44: // Show any bubbles that were added to
show_queue_.
On 2015/08/21 01:48:31, msw wrote:
> nit: |show_queue_|

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager.h:47: // This will be false once the
BubbleManager is being destroyed to prevent
On 2015/08/21 01:48:31, msw wrote:
> nit: consider something like "A flag used to delay new bubbles from showing
> while another bubble is closing, and prohibits bubbles from showing during
> manager teardown."

Renamed to |manager_state_| and updated the comment.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager.h:54: // If a closing bubble needs to show
another bubble, it will go here.
On 2015/08/21 01:48:31, msw wrote:
> nit: // The bubbles queued to be shown when possible.

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager.h:55:
scoped_ptr<ScopedVector<BubbleController>> show_queue_;
On 2015/08/21 01:48:31, msw wrote:
> Consider keeping this as a ScopedVector that's always valid, and use
> |can_show_bubbles_| as a toggle for adding controllers to |controllers_| or
> |show_queue_|.

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
File components/bubble/bubble_manager_unittest.cc (right):

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager_unittest.cc:65: class DelegateChainHelper {
On 2015/08/21 01:48:31, msw wrote:
> nit: comment

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager_unittest.cc:84: class BubbleManagerFixture :
public BubbleManager {
On 2015/08/21 01:48:31, msw wrote:
> Why not just use a BubbleManager instance below?
> (maybe make CloseAllBubbles public if that's the only blocker?)

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager_unittest.cc:105: // thread. This will allow the
UI DCHECK pass in these tests.
On 2015/08/21 01:48:31, msw wrote:
> nit: "DCHECK to pass"

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager_unittest.cc:123: // Manager will delete
bubble_ui.
On 2015/08/21 01:48:31, msw wrote:
> Can you EXPECT_CALL(*bubble_ui, ~MockBubbleUI());? (ditto below)

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager_unittest.cc:129: // Manager will delete
delegate.
On 2015/08/21 01:48:31, msw wrote:
> Can you EXPECT_CALL(*delegate, ~MockBubbleDelegate());? (ditto below)

Done.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager_unittest.cc:235:
manager_->CloseAllEvent(BUBBLE_CLOSE_ACCEPTED);
On 2015/08/21 01:48:31, msw wrote:
> Hmm, I wonder if we should DCHECK/guard against certain close reasons in
> CloseAllBubbles. No production codepath should ever
> CloseAll(BUBBLE_CLOSE_ACCEPTED), etc. All those bubbles will act as if they
were
> accepted/rejected, etc.

Added DCHECK for ACCEPTED and CANCELLED. I think the others are OK.

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_manager_unittest.cc:265: // Bubble should NOT get an
update event because it's already closed.
On 2015/08/21 01:48:31, msw wrote:
> nit: "a close event"?

Done.

msw

https://codereview.chromium.org/1251633002/diff/240001/components/bubble.gypi File components/bubble.gypi (right): https://codereview.chromium.org/1251633002/diff/240001/components/bubble.gypi#newcode6 components/bubble.gypi:6: 'targets': [ On 2015/08/25 02:13:36, Hector Carmona wrote: > ...

5 years, 3 months ago (2015-08-26 01:42:36 UTC) #37

hcarmona

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubble_delegate.h File components/bubble/bubble_delegate.h (right): https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubble_delegate.h#newcode32 components/bubble/bubble_delegate.h:32: BUBBLE_CLOSE_FULLSCREEN_TOGGLED, On 2015/08/26 01:42:35, msw wrote: > On 2015/08/25 ...

5 years, 3 months ago (2015-08-26 17:25:58 UTC) #38

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
File components/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/1251633002/diff/240001/components/bubble/bubb...
components/bubble/bubble_delegate.h:32: BUBBLE_CLOSE_FULLSCREEN_TOGGLED,
On 2015/08/26 01:42:35, msw wrote:
> On 2015/08/25 02:13:37, Hector Carmona wrote:
> > On 2015/08/21 01:48:30, msw wrote:
> > > I wonder if this should be used for other anchor window bounds changes. If
a
> > > bubble isn't closing for loss of focus, then the parent/anchor window
bounds
> > can
> > > be manipulated (and the bubble anchor may need updating). Views bubbles
> should
> > > already handle those anchor window updates with a WidgetObserver, but we
> might
> > > want to add a notification for that here (or expand this one to cover
> broader
> > > anchor window bounds changes). This may not be immediately necessary if we
> > rely
> > > on per-platform behavior, but then that might be worth noting in a
comment.
> > 
> > I'm a bit confused by what exactly this comment means... I don't think that
> > resizing a window should send a close bubble event. A bubble should
> > close itself because of a loss of focus at that point, rather than rely
> > on a window resize event.
> 
> My thought was also that bubbles might need to update their anchoring on
parent
> window size changes (or maybe close if their anchor has disappeared due to
> layout changes). But as I said, views already does the basics here, and it
might
> not be needed for now.

Acknowledged.

https://codereview.chromium.org/1251633002/diff/300001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1251633002/diff/300001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:475:
tab_strip_model_->RemoveObserver(bubble_manager_.get());
On 2015/08/26 01:42:35, msw wrote:
> Make the ChromeBubbleManager dtor remove itself as an observer and just
> unconditionally call |bubble_manager_.reset()| here (or soon; ie. before
> tab_strip_model_ is destroyed).

Done.

https://codereview.chromium.org/1251633002/diff/300001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:557:
tab_strip_model_->AddObserver(bubble_manager_.get());
On 2015/08/26 01:42:35, msw wrote:
> Do this in the ChromeBubbleManager ctor; pass it |tab_strip_model_| or |this|.

Done.

https://codereview.chromium.org/1251633002/diff/300001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:823:
GetBubbleManager()->WindowFullscreenStateChanged();
On 2015/08/26 01:42:35, msw wrote:
> Make this conditional on the existence of a manager, don't create one here.

Done.

https://codereview.chromium.org/1251633002/diff/300001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager.h (right):

https://codereview.chromium.org/1251633002/diff/300001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.h:13: public
base::SupportsWeakPtr<ChromeBubbleManager> {
On 2015/08/26 01:42:35, msw wrote:
> Remove this if it's unused; nothing calls AsWeakPtr on a manager, afaict.
> (otherwise, include the weak_ptr header if it's really needed)

Done.

https://codereview.chromium.org/1251633002/diff/300001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.h:22: void
WindowFullscreenStateChanged();
On 2015/08/26 01:42:36, msw wrote:
> Do this with a WebContentsObserver::DidToggleFullscreenModeForTab override and
> remove the last remaining observer hook in browser.cc (and the fullscreen call
> in Cocoa's moveViewsForImmersiveFullscreen).

Done.

https://codereview.chromium.org/1251633002/diff/300001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.h:23: void NavigationEntryCommitted();
On 2015/08/26 01:42:35, msw wrote:
> When is this ever called? Should this be a WebContentsObserver override?

Done.

https://codereview.chromium.org/1251633002/diff/300001/components/bubble/bubb...
File components/bubble/bubble_controller.cc (right):

https://codereview.chromium.org/1251633002/diff/300001/components/bubble/bubb...
components/bubble/bubble_controller.cc:43: bubble_ui_->Close();
On 2015/08/26 01:42:36, msw wrote:
> I wonder if BubbleUI::Close is needed or if its dtor would suffice.

I like having both |Close| and |Show| because it makes the code more
readable. It's clear what should happen in |Show| and |Close|.

https://codereview.chromium.org/1251633002/diff/300001/components/bubble/bubb...
File components/bubble/bubble_controller.h (right):

https://codereview.chromium.org/1251633002/diff/300001/components/bubble/bubb...
components/bubble/bubble_controller.h:23: // Will call CloseBubble on the
manager that this bubble reference is
On 2015/08/26 01:42:36, msw wrote:
> nit: consider one-liner: "Calls CloseBubble on the associated BubbleManager."

Done.

https://codereview.chromium.org/1251633002/diff/300001/components/bubble/bubb...
File components/bubble/bubble_manager.cc (right):

https://codereview.chromium.org/1251633002/diff/300001/components/bubble/bubb...
components/bubble/bubble_manager.cc:57: if (manager_state_ == QUEUE_BUBBLES)
On 2015/08/26 01:42:36, msw wrote:
> Hmm, I wonder if this function could somehow be called while the manager is
> already in queue state, and setting it to show now might not be correct...
Maybe
> DCHECK_NE(manager_state_, QUEUE_BUBBLES) for now at the top of the function?
> Ditto in CloseAllBubbles, and maybe move the state change into
> ShowPendingBubbles?

If a bubble closed another bubble through the manager, then that would
change this the state SHOW, which would be bad. I've added the DCHECKs.

https://codereview.chromium.org/1251633002/diff/300001/components/bubble/bubb...
File components/bubble/bubble_manager_unittest.cc (right):

https://codereview.chromium.org/1251633002/diff/300001/components/bubble/bubb...
components/bubble/bubble_manager_unittest.cc:19: ~MockBubbleUI() override {
Die(); }
On 2015/08/26 01:42:36, msw wrote:
> nit: s/Die/Destroy[ed]/

Done.

msw

lgtm with a nit; thanks for bearing with my heavy feedback. I'd like to see ...

5 years, 3 months ago (2015-08-26 20:29:09 UTC) #39

msw

Oh, you should also remove mentions of the permission bubbles from the CL title and ...

5 years, 3 months ago (2015-08-26 20:30:38 UTC) #40

hcarmona

On 2015/08/26 20:29:09, msw wrote: > lgtm with a nit; thanks for bearing with my ...

5 years, 3 months ago (2015-08-26 20:52:28 UTC) #41

hcarmona

hcarmona@chromium.org changed reviewers: + blundell@chromium.org

5 years, 3 months ago (2015-08-26 21:15:20 UTC) #42

hcarmona

Hi blundell@, Please take a quick look at the files we're adding in components. We've ...

5 years, 3 months ago (2015-08-26 21:15:26 UTC) #43

blundell

On 2015/08/26 21:15:26, Hector Carmona wrote: > Hi blundell@, > > Please take a quick ...

5 years, 3 months ago (2015-08-27 09:22:19 UTC) #44

hcarmona

On 2015/08/27 09:22:19, blundell wrote: > On 2015/08/26 21:15:26, Hector Carmona wrote: > > Hi ...

5 years, 3 months ago (2015-08-27 18:35:21 UTC) #45

blundell

Thanks! Please remove all the remaining //content dependencies and DEPS allowances at the //components level ...

5 years, 3 months ago (2015-08-27 18:38:47 UTC) #46

msw

https://codereview.chromium.org/1251633002/diff/360001/components/bubble.gypi File components/bubble.gypi (right): https://codereview.chromium.org/1251633002/diff/360001/components/bubble.gypi#newcode15 components/bubble.gypi:15: '../content/content.gyp:content_browser', You can also remove this now. https://codereview.chromium.org/1251633002/diff/360001/components/bubble/BUILD.gn File ...

5 years, 3 months ago (2015-08-27 18:41:13 UTC) #47

hcarmona

Renamed to |thread_checker_| and removed unneeded dependencies. Dependency on gfx was left over from an ...

5 years, 3 months ago (2015-08-27 20:25:42 UTC) #48

msw

https://codereview.chromium.org/1251633002/diff/400001/components/bubble/DEPS File components/bubble/DEPS (right): https://codereview.chromium.org/1251633002/diff/400001/components/bubble/DEPS#newcode1 components/bubble/DEPS:1: include_rules = [ Is this file still needed? The ...

5 years, 3 months ago (2015-08-27 21:56:10 UTC) #49

hcarmona

https://codereview.chromium.org/1251633002/diff/400001/components/bubble/DEPS File components/bubble/DEPS (right): https://codereview.chromium.org/1251633002/diff/400001/components/bubble/DEPS#newcode1 components/bubble/DEPS:1: include_rules = [ On 2015/08/27 21:56:09, msw wrote: > ...

5 years, 3 months ago (2015-08-27 22:19:51 UTC) #50

blundell

//components lgtm Thanks! https://codereview.chromium.org/1251633002/diff/420001/components/bubble/BUILD.gn File components/bubble/BUILD.gn (right): https://codereview.chromium.org/1251633002/diff/420001/components/bubble/BUILD.gn#newcode5 components/bubble/BUILD.gn:5: static_library("bubble") { nit: source_set is preferable ...

5 years, 3 months ago (2015-08-28 08:44:32 UTC) #52

hcarmona

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, blundell@chromium.org Link to ...

5 years, 3 months ago (2015-08-28 17:01:29 UTC) #55

commit-bot: I haz the power

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

5 years, 3 months ago (2015-08-28 17:01:39 UTC) #56

commit-bot: I haz the power

5 years, 3 months ago (2015-08-28 18:46:42 UTC) #58

Message was sent while issue was closed.

Patchset 23 (id:??) landed as
https://crrev.com/aa431c041f3bf551cddc077645bc2409bdeade01
Cr-Commit-Position: refs/heads/master@{#346195}

Issue 1251633002: Add BubbleManager to manage bubbles and ChromeBubbleManager for events. (Closed)

Description

Patch Set 1 : Work in Progress #

Patch Set 2 : Kill views_bubble_controller #

Patch Set 3 : Broken, Ignore #

Patch Set 4 : Update #

Patch Set 5 : Apply Feedback #

Patch Set 6 : Partial Feedback #

Patch Set 7 : Apply Feedback #

Patch Set 8 : Bubble Manager only #

Patch Set 9 : Mac Changes to test trybots #

Patch Set 10 : Fix Build #

Patch Set 11 : Apply Feedback #

Patch Set 12 : Fix mac compile #

Patch Set 13 : TESTS! #

Patch Set 14 : Feedback Applied #

Patch Set 15 : Fix trybots pt1 #

Patch Set 16 : Fix trybots pt2 #

Patch Set 17 : Feedback #

Patch Set 18 : Address nit #

Patch Set 19 : Feedback #

Patch Set 20 : No more //content dep in bubbles #

Patch Set 21 : Less deps #

Patch Set 22 : No DEPS in bubble #

Patch Set 23 : gn nit #

Messages