Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1572743002:
Make sure bubbles in Views default to close before their RenderFrameHosts. (Closed)
|
Created:
4 years, 11 months ago by Jeffrey Yasskin Modified:
4 years, 10 months ago Reviewers:
Reilly Grant (use Gerrit), Avi (use Gerrit), Alexei Svitkine (slow), hcarmona, Peter Kasting CC:
chromium-reviews, asvitkine+watch_chromium.org, groby+bubble_chromium.org, msw+watch_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake sure bubbles in Views close before their RenderFrameHosts.
This is probably covered by navigation, but that isn't obvious from
reading the code, so this change adds code to close bubbles directly.
BUG=571466
Committed: https://crrev.com/4a12e67ac24b290e3da1ce7403bcd73be0a4dc8e
Cr-Commit-Position: refs/heads/master@{#374503}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Give each bubble an owner #Patch Set 3 : Clean up #
Total comments: 13
Patch Set 4 : Update names and matching computation #Patch Set 5 : Add tests #Patch Set 6 : Add a test and only allow one filter in CloseAllMatchingBubbles. #
Total comments: 2
Patch Set 7 : Move DCHECK string into longer comment #Messages
Total messages: 73 (23 generated)
jyasskin@chromium.org changed reviewers: + avi@chromium.org, hcarmona@chromium.org, pkasting@chromium.org
How's this look?
That closes the bubble when any frame on the page goes away?
On 2016/01/08 22:53:54, Avi wrote: > That closes the bubble when any frame on the page goes away? I believe so, but I suspect the NavigationEntryCommitted hook already closes the bubble when any frame on the page navigates, which should be somewhat more common, so I don't think this change will be disruptive. That said, I don't know this system well, so I could easily be wrong.
On 2016/01/08 22:57:14, Jeffrey Yasskin wrote: > On 2016/01/08 22:53:54, Avi wrote: > > That closes the bubble when any frame on the page goes away? > > I believe so, but I suspect the NavigationEntryCommitted hook already closes the > bubble when any frame on the page navigates, which should be somewhat more > common, so I don't think this change will be disruptive. That said, I don't know > this system well, so I could easily be wrong. I also think this is any frame in the same window (::Browser is a window, right?), not just the tab showing the bubble.
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572743002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572743002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... File components/bubble/bubble_close_reason.h (right): https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... components/bubble/bubble_close_reason.h:13: BUBBLE_CLOSE_FORCED, Can't we just use this? We only need the new value to track this in a histogram, but it's not clear we need that to be distinct from this value in the histogram.
https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... File components/bubble/bubble_close_reason.h (right): https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... components/bubble/bubble_close_reason.h:13: BUBBLE_CLOSE_FORCED, On 2016/01/09 01:19:35, Peter Kasting wrote: > Can't we just use this? We only need the new value to track this in a > histogram, but it's not clear we need that to be distinct from this value in the > histogram. If a bubble needs to avoid closing when a frame is closed, (like the extensions bubble avoids closing for navigation), it'll need to use the new value. I guess you're saying that we shouldn't think ahead, and instead let that bubble add the value when it needs it? Which of these other values would you merge? The histogram may also be useful in telling us whether the new value was actually needed.
https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... File components/bubble/bubble_close_reason.h (right): https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... components/bubble/bubble_close_reason.h:13: BUBBLE_CLOSE_FORCED, On 2016/01/09 02:22:34, Jeffrey Yasskin wrote: > On 2016/01/09 01:19:35, Peter Kasting wrote: > > Can't we just use this? We only need the new value to track this in a > > histogram, but it's not clear we need that to be distinct from this value in > the > > histogram. > > If a bubble needs to avoid closing when a frame is closed, (like the extensions > bubble avoids closing for navigation), it'll need to use the new value. Ah. > I guess > you're saying that we shouldn't think ahead, and instead let that bubble add the > value when it needs it? I wouldn't add the complexity if we don't know we need it. > Which of these other values would you merge? I don't think I understand the question? Are you asking if there are other values here that should be cleaned up at the same time? > The histogram may also be useful in telling us whether the new value was > actually needed. That would be a good reason, if you have firm plans to monitor this and then remove the new code you're adding if it turns out to be unnecessary.
On 2016/01/08 22:59:18, Jeffrey Yasskin wrote: > I also think this is any frame in the same window (::Browser is a window, > right?), not just the tab showing the bubble. A browser is a window with multiple tabs. I don't know your calling structure, but if your code would respond to a frame going away by killing the bubble even if the frame belonged to a WebContents that wasn't in front, then yeah.
On 2016/01/10 00:38:18, Avi wrote: > On 2016/01/08 22:59:18, Jeffrey Yasskin wrote: > > I also think this is any frame in the same window (::Browser is a window, > > right?), not just the tab showing the bubble. > > A browser is a window with multiple tabs. I don't know your calling structure, > but if your code would respond to a frame going away by killing the bubble even > if the frame belonged to a WebContents that wasn't in front, then yeah. The ChromeBubbleBrowser only observes the active tab on each browser. |ActiveTabChanged| ensures that we update the web contents we observe. https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... File components/bubble/bubble_close_reason.h (right): https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... components/bubble/bubble_close_reason.h:13: BUBBLE_CLOSE_FORCED, On 2016/01/09 02:26:18, Peter Kasting wrote: > On 2016/01/09 02:22:34, Jeffrey Yasskin wrote: > > On 2016/01/09 01:19:35, Peter Kasting wrote: > > > Can't we just use this? We only need the new value to track this in a > > > histogram, but it's not clear we need that to be distinct from this value in > > the > > > histogram. > > > > If a bubble needs to avoid closing when a frame is closed, (like the > extensions > > bubble avoids closing for navigation), it'll need to use the new value. > > Ah. > > > I guess > > you're saying that we shouldn't think ahead, and instead let that bubble add > the > > value when it needs it? > > I wouldn't add the complexity if we don't know we need it. > > > Which of these other values would you merge? > > I don't think I understand the question? Are you asking if there are other > values here that should be cleaned up at the same time? > > > The histogram may also be useful in telling us whether the new value was > > actually needed. > > That would be a good reason, if you have firm plans to monitor this and then > remove the new code you're adding if it turns out to be unnecessary. If we have a reason to close a bubble, it should be added here in order to avoid having weird logic later for an overloaded close reason. Close reasons should be explicit IMHO. This leads to simpler bubble logic and more concise stats. https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... components/bubble/bubble_close_reason.h:40: // A frame was closed, so references in the bubble may be invalid. Let's be more specific here about what references are invalid. https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_de... File components/bubble/bubble_delegate.h (right): https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_de... components/bubble/bubble_delegate.h:27: // are destroyed before the bubble. I'm not 100% happy with this comment because it seems very specific to BUBBLE_CLOSE_FRAME_DESTROYED. Should this warning go with that enum? Any close reason has the potential of cleaning up data that a bubble depends on. The onus is on the bubble to know what it can and cannot survive. https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_ma... File components/bubble/bubble_manager.h (right): https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_ma... components/bubble/bubble_manager.h:51: // BubbleDelegate will be destroyed before any RenderFrameHost is destroyed. Same concern as above about this being very specific to the RenderFrameHost. Can this comment go with the enum?
https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... File components/bubble/bubble_close_reason.h (right): https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... components/bubble/bubble_close_reason.h:13: BUBBLE_CLOSE_FORCED, On 2016/01/11 19:07:22, Hector Carmona wrote: > On 2016/01/09 02:26:18, Peter Kasting wrote: > > On 2016/01/09 02:22:34, Jeffrey Yasskin wrote: > > > On 2016/01/09 01:19:35, Peter Kasting wrote: > > > > Can't we just use this? We only need the new value to track this in a > > > > histogram, but it's not clear we need that to be distinct from this value > in > > > the > > > > histogram. > > > > > > If a bubble needs to avoid closing when a frame is closed, (like the > > extensions > > > bubble avoids closing for navigation), it'll need to use the new value. > > > > Ah. > > > > > I guess > > > you're saying that we shouldn't think ahead, and instead let that bubble add > > the > > > value when it needs it? > > > > I wouldn't add the complexity if we don't know we need it. > > > > > Which of these other values would you merge? > > > > I don't think I understand the question? Are you asking if there are other > > values here that should be cleaned up at the same time? > > > > > The histogram may also be useful in telling us whether the new value was > > > actually needed. > > > > That would be a good reason, if you have firm plans to monitor this and then > > remove the new code you're adding if it turns out to be unnecessary. > > If we have a reason to close a bubble, it should be added here in order > to avoid having weird logic later for an overloaded close reason. > Close reasons should be explicit IMHO. This leads to simpler bubble > logic and more concise stats. My argument is really that "force close" _is_ the reason, given the comments on what this is supposed to mean. If you don't think that's good, then we shouldn't have a value like "force close" at all. All existing uses of this should be replaced with distinct individual values. I don't think, in this specific case, that that results in any simpler bubble close logic (it's more complicated because we have to check more conditions; there's no "weird logic later") or more concise stats (again, they're actually less concise due to having more buckets). So I don't think that change accomplishes your stated aims; I think what I'm arguing for is precisely what you're claiming to want. Am I confused?
On 2016/01/11 21:42:19, Peter Kasting wrote: > https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... > File components/bubble/bubble_close_reason.h (right): > > https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... > components/bubble/bubble_close_reason.h:13: BUBBLE_CLOSE_FORCED, > On 2016/01/11 19:07:22, Hector Carmona wrote: > > On 2016/01/09 02:26:18, Peter Kasting wrote: > > > On 2016/01/09 02:22:34, Jeffrey Yasskin wrote: > > > > On 2016/01/09 01:19:35, Peter Kasting wrote: > > > > > Can't we just use this? We only need the new value to track this in a > > > > > histogram, but it's not clear we need that to be distinct from this > value > > in > > > > the > > > > > histogram. > > > > > > > > If a bubble needs to avoid closing when a frame is closed, (like the > > > extensions > > > > bubble avoids closing for navigation), it'll need to use the new value. > > > > > > Ah. > > > > > > > I guess > > > > you're saying that we shouldn't think ahead, and instead let that bubble > add > > > the > > > > value when it needs it? > > > > > > I wouldn't add the complexity if we don't know we need it. > > > > > > > Which of these other values would you merge? > > > > > > I don't think I understand the question? Are you asking if there are other > > > values here that should be cleaned up at the same time? > > > > > > > The histogram may also be useful in telling us whether the new value was > > > > actually needed. > > > > > > That would be a good reason, if you have firm plans to monitor this and then > > > remove the new code you're adding if it turns out to be unnecessary. > > > > If we have a reason to close a bubble, it should be added here in order > > to avoid having weird logic later for an overloaded close reason. > > Close reasons should be explicit IMHO. This leads to simpler bubble > > logic and more concise stats. > > My argument is really that "force close" _is_ the reason, given the comments on > what this is supposed to mean. > > If you don't think that's good, then we shouldn't have a value like "force > close" at all. All existing uses of this should be replaced with distinct > individual values. "Force Close" should only be used when it's not possible for ANY bubble to remain open after this event. If no more bubbles should remain open when the RenderFrameHost is destroyed, then yes it makes sense to use "Force Close". However, if any bubble can survive this, then we shouldn't use "Force Close". From looking at this: https://www.chromium.org/developers/design-documents/displaying-a-web-page-in... it looks like there can be more than one Render Host per Browser. This means that a bubble that doesn't depend on the Web Contents should have the option to remain open. If this is true, then we shouldn't use "Force Close". > I don't think, in this specific case, that that results in any simpler bubble > close logic (it's more complicated because we have to check more conditions; > there's no "weird logic later") or more concise stats (again, they're actually > less concise due to having more buckets). So I don't think that change > accomplishes your stated aims; I think what I'm arguing for is precisely what > you're claiming to want. Am I confused? What I meant by "weird logic" is that a bubble should be able to decide to stay open by looking at the close reason only. If we overlap the close reasons too much, then a bubble will need additional information when making the decision to close or not, and that is "weird logic". If the close reason is "Force Close", it doesn't matter what the bubble decides because it will be closed. My argument here is only important if we overlap with something other than "Force Close".
On 2016/01/11 22:15:10, Hector Carmona wrote: > On 2016/01/11 21:42:19, Peter Kasting wrote: > > > https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... > > File components/bubble/bubble_close_reason.h (right): > > > > > https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... > > components/bubble/bubble_close_reason.h:13: BUBBLE_CLOSE_FORCED, > > On 2016/01/11 19:07:22, Hector Carmona wrote: > > > On 2016/01/09 02:26:18, Peter Kasting wrote: > > > > On 2016/01/09 02:22:34, Jeffrey Yasskin wrote: > > > > > On 2016/01/09 01:19:35, Peter Kasting wrote: > > > > > > Can't we just use this? We only need the new value to track this in a > > > > > > histogram, but it's not clear we need that to be distinct from this > > value > > > in > > > > > the > > > > > > histogram. > > > > > > > > > > If a bubble needs to avoid closing when a frame is closed, (like the > > > > extensions > > > > > bubble avoids closing for navigation), it'll need to use the new value. > > > > > > > > Ah. > > > > > > > > > I guess > > > > > you're saying that we shouldn't think ahead, and instead let that bubble > > add > > > > the > > > > > value when it needs it? > > > > > > > > I wouldn't add the complexity if we don't know we need it. > > > > > > > > > Which of these other values would you merge? > > > > > > > > I don't think I understand the question? Are you asking if there are > other > > > > values here that should be cleaned up at the same time? > > > > > > > > > The histogram may also be useful in telling us whether the new value was > > > > > actually needed. > > > > > > > > That would be a good reason, if you have firm plans to monitor this and > then > > > > remove the new code you're adding if it turns out to be unnecessary. > > > > > > If we have a reason to close a bubble, it should be added here in order > > > to avoid having weird logic later for an overloaded close reason. > > > Close reasons should be explicit IMHO. This leads to simpler bubble > > > logic and more concise stats. > > > > My argument is really that "force close" _is_ the reason, given the comments > on > > what this is supposed to mean. > > > > If you don't think that's good, then we shouldn't have a value like "force > > close" at all. All existing uses of this should be replaced with distinct > > individual values. > > "Force Close" should only be used when it's not possible for ANY bubble > to remain open after this event. If no more bubbles should remain open > when the RenderFrameHost is destroyed, then yes it makes sense to use > "Force Close". However, if any bubble can survive this, then we > shouldn't use "Force Close". > > From looking at this: > https://www.chromium.org/developers/design-documents/displaying-a-web-page-in... > it looks like there can be more than one Render Host per Browser. This > means that a bubble that doesn't depend on the Web Contents should have > the option to remain open. If this is true, then we shouldn't use > "Force Close". There should be only one renderer host visible at a time, though, right? (For the foreground tab.) And we can't have bubbles open relating to background tabs. So I'm still thinking that this should really force everything to close and nothing should remain afterwards. (That would also be the behavior of the existing patch, which adds an ability for a bubble to stay open, but doesn't actually make any bubbles do so. What I'm really saying here is, let's not add an escape hatch for this unless we know we need it.) > > I don't think, in this specific case, that that results in any simpler bubble > > close logic (it's more complicated because we have to check more conditions; > > there's no "weird logic later") or more concise stats (again, they're actually > > less concise due to having more buckets). So I don't think that change > > accomplishes your stated aims; I think what I'm arguing for is precisely what > > you're claiming to want. Am I confused? > > What I meant by "weird logic" is that a bubble should be able to decide > to stay open by looking at the close reason only. If we overlap the > close reasons too much, then a bubble will need additional information > when making the decision to close or not, and that is "weird logic". > If the close reason is "Force Close", it doesn't matter what the bubble > decides because it will be closed. My argument here is only important > if we overlap with something other than "Force Close". Thanks, that makes sense.
On 2016/01/11 22:21:00, Peter Kasting wrote: > There should be only one renderer host visible at a time, though, right? (For > the foreground tab.) And we can't have bubbles open relating to background > tabs. Each iframe within the foreground tab will have its own RenderFrameHost, right? Ideally, I'd like to only close bubbles associated with the particular iframe that's closing/navigating, but the bubble system doesn't record enough information to do that. It sounds like you and Hector are converging on BUBBLE_CLOSE_FORCED, so I'll do that.
On 2016/01/11 22:48:17, Jeffrey Yasskin wrote: > On 2016/01/11 22:21:00, Peter Kasting wrote: > > There should be only one renderer host visible at a time, though, right? (For > > the foreground tab.) And we can't have bubbles open relating to background > > tabs. > > Each iframe within the foreground tab will have its own RenderFrameHost, right? > Ideally, I'd like to only close bubbles associated with the particular iframe > that's closing/navigating, but the bubble system doesn't record enough > information to do that. ...Because we don't track some kind of "owning" RenderFrameHost for each bubble? It seems like the behavior here ought to be very similar to navigation. Are there bubbles today which respond to the "frame navigated" close reason with "don't close"? Perhaps those are illustrative in thinking about how to handle this case. If there are no such bubbles, then maybe we should rename that enum value (frame navigated or closed?) and use it, or even collapse into the force closed case? I feel like a blind man groping his way through an unfamiliar room; I just don't understand enough about how our existing bubbles work. :/
On 2016/01/11 22:21:00, Peter Kasting wrote: > On 2016/01/11 22:15:10, Hector Carmona wrote: > > On 2016/01/11 21:42:19, Peter Kasting wrote: > > > > > > https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... > > > File components/bubble/bubble_close_reason.h (right): > > > > > > > > > https://codereview.chromium.org/1572743002/diff/1/components/bubble/bubble_cl... > > > components/bubble/bubble_close_reason.h:13: BUBBLE_CLOSE_FORCED, > > > On 2016/01/11 19:07:22, Hector Carmona wrote: > > > > On 2016/01/09 02:26:18, Peter Kasting wrote: > > > > > On 2016/01/09 02:22:34, Jeffrey Yasskin wrote: > > > > > > On 2016/01/09 01:19:35, Peter Kasting wrote: > > > > > > > Can't we just use this? We only need the new value to track this in > a > > > > > > > histogram, but it's not clear we need that to be distinct from this > > > value > > > > in > > > > > > the > > > > > > > histogram. > > > > > > > > > > > > If a bubble needs to avoid closing when a frame is closed, (like the > > > > > extensions > > > > > > bubble avoids closing for navigation), it'll need to use the new > value. > > > > > > > > > > Ah. > > > > > > > > > > > I guess > > > > > > you're saying that we shouldn't think ahead, and instead let that > bubble > > > add > > > > > the > > > > > > value when it needs it? > > > > > > > > > > I wouldn't add the complexity if we don't know we need it. > > > > > > > > > > > Which of these other values would you merge? > > > > > > > > > > I don't think I understand the question? Are you asking if there are > > other > > > > > values here that should be cleaned up at the same time? > > > > > > > > > > > The histogram may also be useful in telling us whether the new value > was > > > > > > actually needed. > > > > > > > > > > That would be a good reason, if you have firm plans to monitor this and > > then > > > > > remove the new code you're adding if it turns out to be unnecessary. > > > > > > > > If we have a reason to close a bubble, it should be added here in order > > > > to avoid having weird logic later for an overloaded close reason. > > > > Close reasons should be explicit IMHO. This leads to simpler bubble > > > > logic and more concise stats. > > > > > > My argument is really that "force close" _is_ the reason, given the comments > > on > > > what this is supposed to mean. > > > > > > If you don't think that's good, then we shouldn't have a value like "force > > > close" at all. All existing uses of this should be replaced with distinct > > > individual values. > > > > "Force Close" should only be used when it's not possible for ANY bubble > > to remain open after this event. If no more bubbles should remain open > > when the RenderFrameHost is destroyed, then yes it makes sense to use > > "Force Close". However, if any bubble can survive this, then we > > shouldn't use "Force Close". > > > > From looking at this: > > > https://www.chromium.org/developers/design-documents/displaying-a-web-page-in... > > it looks like there can be more than one Render Host per Browser. This > > means that a bubble that doesn't depend on the Web Contents should have > > the option to remain open. If this is true, then we shouldn't use > > "Force Close". > > There should be only one renderer host visible at a time, though, right? (For > the foreground tab.) And we can't have bubbles open relating to background > tabs. > > So I'm still thinking that this should really force everything to close and > nothing should remain afterwards. > > (That would also be the behavior of the existing patch, which adds an ability > for a bubble to stay open, but doesn't actually make any bubbles do so. What > I'm really saying here is, let's not add an escape hatch for this unless we know > we need it.) > > > > I don't think, in this specific case, that that results in any simpler > bubble > > > close logic (it's more complicated because we have to check more conditions; > > > there's no "weird logic later") or more concise stats (again, they're > actually > > > less concise due to having more buckets). So I don't think that change > > > accomplishes your stated aims; I think what I'm arguing for is precisely > what > > > you're claiming to want. Am I confused? > > > > What I meant by "weird logic" is that a bubble should be able to decide > > to stay open by looking at the close reason only. If we overlap the > > close reasons too much, then a bubble will need additional information > > when making the decision to close or not, and that is "weird logic". > > If the close reason is "Force Close", it doesn't matter what the bubble > > decides because it will be closed. My argument here is only important > > if we overlap with something other than "Force Close". > > Thanks, that makes sense. The Foil bubble which comes up on Windows machines with UwS is not tied to any web contents. That bubble does not yet use this API, but we had it in mind when this was designed. Should this CL use "Force Close" and a future CL add the enum when an extra-WebContent bubble needs to use the BubbleManager? I can go either way on this because we have a concrete example of where we will need it, but it is not yet needed. However, now that you mentioned that it's tied to the foreground tab, is this event the same as BUBBLE_CLOSE_TABSWITCHED?
On Mon, Jan 11, 2016 at 2:55 PM, <hcarmona@chromium.org> wrote: > The Foil bubble which comes up on Windows machines with UwS is not tied > to any web contents. That bubble does not yet use this API, but we had > it in mind when this was designed. Should this CL use "Force Close" and > a future CL add the enum when an extra-WebContent bubble needs to use > the BubbleManager? > > I can go either way on this because we have a concrete example of where > we will need it, but it is not yet needed. > Perhaps the best suggestion, as I alluded to earlier, is to have bubbles have some kind of member indicating what frame "owns" them, which for a global bubble could be "no frame". When a frame closes or is navigated or placed in a background tab, that would automatically close all bubbles "owned by" that frame. This seems clearer and safer than basing the API on the "bubble close reason" and letting bubbles opt-out on a per-reason basis. However, now that you mentioned that it's tied to the foreground tab, is > this event the same as BUBBLE_CLOSE_TABSWITCHED? I don't know. Much like with the "frame navigated" cases, it seems to me like we have a lot of enum values, but for the purposes of deciding whether a bubble should be closed, they're all effectively the same. PK -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/01/11 23:06:38, Peter Kasting wrote: > On Mon, Jan 11, 2016 at 2:55 PM, <mailto:hcarmona@chromium.org> wrote: > > > The Foil bubble which comes up on Windows machines with UwS is not tied > > to any web contents. That bubble does not yet use this API, but we had > > it in mind when this was designed. Should this CL use "Force Close" and > > a future CL add the enum when an extra-WebContent bubble needs to use > > the BubbleManager? > > > > I can go either way on this because we have a concrete example of where > > we will need it, but it is not yet needed. > > > > Perhaps the best suggestion, as I alluded to earlier, is to have bubbles > have some kind of member indicating what frame "owns" them, which for a > global bubble could be "no frame". When a frame closes or is navigated or > placed in a background tab, that would automatically close all bubbles > "owned by" that frame. I like the concept of a frame owning a bubble, but... > This seems clearer and safer than basing the API on the "bubble close > reason" and letting bubbles opt-out on a per-reason basis. Alas, bubbles will still need to opt out on a per-reason basis. Some bubbles get closed by focus loss, some opt out. Some bubbles close when you go fullscreen, some don't. Some bubbles survive navigation in the frame, some don't. The list continues. The goal of this API is to have a default behavior for all bubbles, but give individual offenders the chance to do their custom dance. Furthermore, for metrics reasons we will still need to collect all reasons why a specific bubble closed. So having an owner will merely add a second code path, while still requiring most of the current path to stay around. Or am I misunderstanding your suggestion? > > However, now that you mentioned that it's tied to the foreground tab, is > > this event the same as BUBBLE_CLOSE_TABSWITCHED? > > > I don't know. Much like with the "frame navigated" cases, it seems to me > like we have a lot of enum values, but for the purposes of deciding whether > a bubble should be closed, they're all effectively the same. Alas, no. Many of those are currently not utilized - because we had to redirect efforts temporarily away from migrating existing bubbles - but for pretty much all of the reasons there, there exists at least one bubble in Chrome that would like to ignore that event when it comes to closing. And even if they don't ignore it, we'd still like to record that reason. (UX would like to know why bubbles get dismissed. To help cull the herd, so to speak) > > PK > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2016/01/12 02:46:48, groby wrote: > On 2016/01/11 23:06:38, Peter Kasting wrote: > > On Mon, Jan 11, 2016 at 2:55 PM, <mailto:hcarmona@chromium.org> wrote: > > > > > The Foil bubble which comes up on Windows machines with UwS is not tied > > > to any web contents. That bubble does not yet use this API, but we had > > > it in mind when this was designed. Should this CL use "Force Close" and > > > a future CL add the enum when an extra-WebContent bubble needs to use > > > the BubbleManager? > > > > > > I can go either way on this because we have a concrete example of where > > > we will need it, but it is not yet needed. > > > > > > > Perhaps the best suggestion, as I alluded to earlier, is to have bubbles > > have some kind of member indicating what frame "owns" them, which for a > > global bubble could be "no frame". When a frame closes or is navigated or > > placed in a background tab, that would automatically close all bubbles > > "owned by" that frame. > > I like the concept of a frame owning a bubble, but... > > > This seems clearer and safer than basing the API on the "bubble close > > reason" and letting bubbles opt-out on a per-reason basis. > > Alas, bubbles will still need to opt out on a per-reason basis. Some bubbles get > closed by focus loss, some opt out. Some bubbles close when you go fullscreen, > some don't. Some bubbles survive navigation in the frame, some don't. The list > continues. The goal of this API is to have a default behavior for all bubbles, > but give individual offenders the chance to do their custom dance. > > Furthermore, for metrics reasons we will still need to collect all reasons why a > specific bubble closed. So having an owner will merely add a second code path, > while still requiring most of the current path to stay around. > > Or am I misunderstanding your suggestion? Maybe. My suggestion is really to have two different APIs that close bubbles. One API basically responds to a frame "going away" (where going away can be being navigated, being closed, being placed in a background tab, etc.) by telling bubbles owned by that frame the frame is gone, which should close them. The other would be something that responds to events other than frame disappearance -- where it's generally safe for a bubble to stick around if it wants -- by notifying all bubbles of that event and allowing them to decide what to do. I don't think it makes sense to truly have a frame "own" a bubble, yet have that bubble survive the frame disappearing. I'd like to know more about cases of bubbles surviving navigation in a frame. It seems like there are three possibilities: (1) While something about the frame might have originally triggered this bubble, it doesn't really refer to anything in the frame anymore. It should survive frame navigation by virtue of not being "owned" by the frame to begin with. (2) A bubble currently survives frame navigation even though it refers to something in the frame. This seems like a guaranteed bug, like the problems Avi said we keep having. (3) I've failed to account for some sort of pattern that isn't (1) or (2), and I need to update my mental image of the world.
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572743002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572743002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572743002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572743002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572743002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572743002/100001
Patchset #2 (id:40001) has been deleted
Sorry it took me so long to get back to this. Here's a patch that attaches an owning frame to each bubble, and tries to close the bubble when that frame is closed. It doesn't force-close the bubble, but ignoring the declared owner in ShouldClose() should look reasonably obvious to a reviewer. I'll add some tests for this if it looks like what y'all want.
Description was changed from ========== Make sure bubbles in Views default to close before their RenderFrameHosts. And document this so bubbles can rely on it. This is probably covered by navigation, but that isn't obvious from reading the code, so this change adds code to close bubbles directly. BUG=571466 ========== to ========== Make sure bubbles in Views close before their RenderFrameHosts. This is probably covered by navigation, but that isn't obvious from reading the code, so this change adds code to close bubbles directly. BUG=571466 ==========
https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.cc (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.cc:106: bool matches = true; If both |match| and |frame| are passed in, should only bubbles that match both conditions be closed? Or does it make sense to close any bubble that matches either condition? Or should only |match| or |frame| be specified, but not both? https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.h (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.h:90: bool CloseAllMatchingBubbles(BubbleController* match, Can we rename these params to |bubble_match| and |frame_match|? I think |match| made sense when it was the only condition that needed to be met.
LGTM https://codereview.chromium.org/1572743002/diff/120001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_installed_bubble.cc (right): https://codereview.chromium.org/1572743002/diff/120001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_installed_bubble.cc:216: return nullptr; I don't know anything about this bubble, so I just want to make sure that returning null here is safe. https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.cc (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.cc:112: if (matches && (*iter)->ShouldClose(reason)) { Nit: Shorter and, IMO, more readable due to omitting the temp: if ((!bubble || bubble == *i) && (!owner || (*i)->OwningFrameIs(owner)) && (*i)->ShouldClose(reason)) { This renames the params as I suggested in the header and renames |iter| to |i| as well (I think |i| and |it| are both better names than |iter|). https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.h (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.h:90: bool CloseAllMatchingBubbles(BubbleController* match, On 2016/02/05 22:48:24, Hector Carmona wrote: > Can we rename these params to |bubble_match| and |frame_match|? > I think |match| made sense when it was the only condition that needed to be met. I think |bubble| and |owner| would be better names.
Avi, could you double-check the addition to TestRenderFrameHost? It seems to work to destroy a RenderFrameHost, but I'm not confident it's the best interface. https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.cc (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.cc:106: bool matches = true; On 2016/02/05 22:48:24, Hector Carmona wrote: > If both |match| and |frame| are passed in, should only bubbles that match both > conditions be closed? > > Or does it make sense to close any bubble that matches either condition? > > Or should only |match| or |frame| be specified, but not both? Right now, only one is ever specified. I lean toward matching both conditions (so that's what's implemented), but since we don't have any use cases for passing both, there's no way to tell. We could instead pass in a lambda to control what matches, but that seems like overengineering for now. https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.cc:112: if (matches && (*iter)->ShouldClose(reason)) { On 2016/02/06 04:13:08, Peter Kasting wrote: > Nit: Shorter and, IMO, more readable due to omitting the temp: > > if ((!bubble || bubble == *i) && (!owner || (*i)->OwningFrameIs(owner)) && > (*i)->ShouldClose(reason)) { > > This renames the params as I suggested in the header and renames |iter| to |i| > as well (I think |i| and |it| are both better names than |iter|). I had some trouble decoding the long boolean expression, which is why I changed it to a temporary variable, but I also like getting my patch approved, so I've changed it back. https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.h (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.h:90: bool CloseAllMatchingBubbles(BubbleController* match, On 2016/02/06 04:13:08, Peter Kasting wrote: > On 2016/02/05 22:48:24, Hector Carmona wrote: > > Can we rename these params to |bubble_match| and |frame_match|? > > I think |match| made sense when it was the only condition that needed to be > met. > > I think |bubble| and |owner| would be better names. I also like |bubble| and |owner|.
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572743002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572743002/160001
components/bubble/* looks good, but please add some unit tests to components/bubble/bubble_manager_unittest.cc that verify closing with this new reason. https://codereview.chromium.org/1572743002/diff/120001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_installed_bubble.cc (right): https://codereview.chromium.org/1572743002/diff/120001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_installed_bubble.cc:216: return nullptr; On 2016/02/06 04:13:08, Peter Kasting wrote: > I don't know anything about this bubble, so I just want to make sure that > returning null here is safe. I worked on this bubble. Returning null is fine here because this bubble isn't related to anything in the render view. https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.cc (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.cc:106: bool matches = true; On 2016/02/08 23:59:09, Jeffrey Yasskin wrote: > On 2016/02/05 22:48:24, Hector Carmona wrote: > > If both |match| and |frame| are passed in, should only bubbles that match both > > conditions be closed? > > > > Or does it make sense to close any bubble that matches either condition? > > > > Or should only |match| or |frame| be specified, but not both? > > Right now, only one is ever specified. I lean toward matching both conditions > (so that's what's implemented), but since we don't have any use cases for > passing both, there's no way to tell. We could instead pass in a lambda to > control what matches, but that seems like overengineering for now. Agreed, lambda would be over-engineering. Can we add a DCHECK(!(bubble && owner)) if we don't expect |bubble| and |owner| to both be passed in. This way, if they're both passed in, whoever changes it will need to decide AND or OR as opposed to us deciding arbitrarily right now. https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.h (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.h:90: bool CloseAllMatchingBubbles(BubbleController* match, On 2016/02/08 23:59:09, Jeffrey Yasskin wrote: > On 2016/02/06 04:13:08, Peter Kasting wrote: > > On 2016/02/05 22:48:24, Hector Carmona wrote: > > > Can we rename these params to |bubble_match| and |frame_match|? > > > I think |match| made sense when it was the only condition that needed to be > > met. > > > > I think |bubble| and |owner| would be better names. > > I also like |bubble| and |owner|. |bubble| and |owner| are fine.
https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.cc (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.cc:106: bool matches = true; On 2016/02/09 00:31:05, Hector Carmona wrote: > On 2016/02/08 23:59:09, Jeffrey Yasskin wrote: > > On 2016/02/05 22:48:24, Hector Carmona wrote: > > > If both |match| and |frame| are passed in, should only bubbles that match > both > > > conditions be closed? > > > > > > Or does it make sense to close any bubble that matches either condition? > > > > > > Or should only |match| or |frame| be specified, but not both? > > > > Right now, only one is ever specified. I lean toward matching both conditions > > (so that's what's implemented), but since we don't have any use cases for > > passing both, there's no way to tell. We could instead pass in a lambda to > > control what matches, but that seems like overengineering for now. > > Agreed, lambda would be over-engineering. > > Can we add a DCHECK(!(bubble && owner)) if we don't expect |bubble| and |owner| > to both be passed in. This way, if they're both passed in, whoever changes it > will need to decide AND or OR as opposed to us deciding arbitrarily right now. Hmm. I'm a little uneasy about using DCHECK like this when there's no real reason why callers can't or even shouldn't do this. I think the implemented behavior (force both to match if you specify both) is probably OK, so I would suggest simply making sure the API comments are clear about this case. If someone passes both and doesn't read the comments, and has a reason to want a different behavior, but doesn't check to see that their code works, they kinda deserve what they get.
https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... File components/bubble/bubble_manager.cc (right): https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... components/bubble/bubble_manager.cc:106: bool matches = true; On 2016/02/09 00:42:41, Peter Kasting wrote: > On 2016/02/09 00:31:05, Hector Carmona wrote: > > On 2016/02/08 23:59:09, Jeffrey Yasskin wrote: > > > On 2016/02/05 22:48:24, Hector Carmona wrote: > > > > If both |match| and |frame| are passed in, should only bubbles that match > > both > > > > conditions be closed? > > > > > > > > Or does it make sense to close any bubble that matches either condition? > > > > > > > > Or should only |match| or |frame| be specified, but not both? > > > > > > Right now, only one is ever specified. I lean toward matching both > conditions > > > (so that's what's implemented), but since we don't have any use cases for > > > passing both, there's no way to tell. We could instead pass in a lambda to > > > control what matches, but that seems like overengineering for now. > > > > Agreed, lambda would be over-engineering. > > > > Can we add a DCHECK(!(bubble && owner)) if we don't expect |bubble| and > |owner| > > to both be passed in. This way, if they're both passed in, whoever changes it > > will need to decide AND or OR as opposed to us deciding arbitrarily right now. > > Hmm. I'm a little uneasy about using DCHECK like this when there's no real > reason why callers can't or even shouldn't do this. > > I think the implemented behavior (force both to match if you specify both) is > probably OK, so I would suggest simply making sure the API comments are clear > about this case. If someone passes both and doesn't read the comments, and has > a reason to want a different behavior, but doesn't check to see that their code > works, they kinda deserve what they get. I can't think of a reason we would specify a specific bubble to close and a frame. If we have a frame, all bubbles in that frame need to decide to close. And if we want to close a specific bubble, then it doesn't matter what frame that bubble is in, it should get the close event. ANDing these two checks means that either 1 bubble will be asked to close or no bubbles will be asked. Neither of those results seems like desired behavior. We should either OR the condition checks, or forbid filtering by both at the same time to avoid unwanted behavior.
On 2016/02/09 00:59:44, Hector Carmona wrote: > I can't think of a reason we would specify a specific bubble to close > and a frame. If we have a frame, all bubbles in that frame need to > decide to close. And if we want to close a specific bubble, then it > doesn't matter what frame that bubble is in, it should get the close > event. ANDing these two checks means that either 1 bubble will be asked > to close or no bubbles will be asked. Neither of those results seems > like desired behavior. We should either OR the condition checks, or > forbid filtering by both at the same time to avoid unwanted behavior. Fair enough; in that case I would suggest changing the "and" to an "or" and then (again) ensuring that's well-documented.
On 2016/02/09 00:59:44, Hector Carmona wrote: > https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... > File components/bubble/bubble_manager.cc (right): > > https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... > components/bubble/bubble_manager.cc:106: bool matches = true; > On 2016/02/09 00:42:41, Peter Kasting wrote: > > On 2016/02/09 00:31:05, Hector Carmona wrote: > > > On 2016/02/08 23:59:09, Jeffrey Yasskin wrote: > > > > On 2016/02/05 22:48:24, Hector Carmona wrote: > > > > > If both |match| and |frame| are passed in, should only bubbles that > match > > > both > > > > > conditions be closed? > > > > > > > > > > Or does it make sense to close any bubble that matches either condition? > > > > > > > > > > Or should only |match| or |frame| be specified, but not both? > > > > > > > > Right now, only one is ever specified. I lean toward matching both > > conditions > > > > (so that's what's implemented), but since we don't have any use cases for > > > > passing both, there's no way to tell. We could instead pass in a lambda to > > > > control what matches, but that seems like overengineering for now. > > > > > > Agreed, lambda would be over-engineering. > > > > > > Can we add a DCHECK(!(bubble && owner)) if we don't expect |bubble| and > > |owner| > > > to both be passed in. This way, if they're both passed in, whoever changes > it > > > will need to decide AND or OR as opposed to us deciding arbitrarily right > now. > > > > Hmm. I'm a little uneasy about using DCHECK like this when there's no real > > reason why callers can't or even shouldn't do this. > > > > I think the implemented behavior (force both to match if you specify both) is > > probably OK, so I would suggest simply making sure the API comments are clear > > about this case. If someone passes both and doesn't read the comments, and > has > > a reason to want a different behavior, but doesn't check to see that their > code > > works, they kinda deserve what they get. > > I can't think of a reason we would specify a specific bubble to close > and a frame. If we have a frame, all bubbles in that frame need to > decide to close. And if we want to close a specific bubble, then it > doesn't matter what frame that bubble is in, it should get the close > event. ANDing these two checks means that either 1 bubble will be asked > to close or no bubbles will be asked. Neither of those results seems > like desired behavior. We should either OR the condition checks, or > forbid filtering by both at the same time to avoid unwanted behavior. If we also want to close all bubbles when bubble==owner==null, then we get the following incoherent behavior: 0) Neither specified: close all bubbles 1) One specified: close a few bubbles 2) Both specified: close more bubbles. I hadn't noticed this until trying to implement the OR behavior.
On 2016/02/09 01:24:36, Jeffrey Yasskin wrote: > On 2016/02/09 00:59:44, Hector Carmona wrote: > > > https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... > > File components/bubble/bubble_manager.cc (right): > > > > > https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... > > components/bubble/bubble_manager.cc:106: bool matches = true; > > On 2016/02/09 00:42:41, Peter Kasting wrote: > > > On 2016/02/09 00:31:05, Hector Carmona wrote: > > > > On 2016/02/08 23:59:09, Jeffrey Yasskin wrote: > > > > > On 2016/02/05 22:48:24, Hector Carmona wrote: > > > > > > If both |match| and |frame| are passed in, should only bubbles that > > match > > > > both > > > > > > conditions be closed? > > > > > > > > > > > > Or does it make sense to close any bubble that matches either > condition? > > > > > > > > > > > > Or should only |match| or |frame| be specified, but not both? > > > > > > > > > > Right now, only one is ever specified. I lean toward matching both > > > conditions > > > > > (so that's what's implemented), but since we don't have any use cases > for > > > > > passing both, there's no way to tell. We could instead pass in a lambda > to > > > > > control what matches, but that seems like overengineering for now. > > > > > > > > Agreed, lambda would be over-engineering. > > > > > > > > Can we add a DCHECK(!(bubble && owner)) if we don't expect |bubble| and > > > |owner| > > > > to both be passed in. This way, if they're both passed in, whoever changes > > it > > > > will need to decide AND or OR as opposed to us deciding arbitrarily right > > now. > > > > > > Hmm. I'm a little uneasy about using DCHECK like this when there's no real > > > reason why callers can't or even shouldn't do this. > > > > > > I think the implemented behavior (force both to match if you specify both) > is > > > probably OK, so I would suggest simply making sure the API comments are > clear > > > about this case. If someone passes both and doesn't read the comments, and > > has > > > a reason to want a different behavior, but doesn't check to see that their > > code > > > works, they kinda deserve what they get. > > > > I can't think of a reason we would specify a specific bubble to close > > and a frame. If we have a frame, all bubbles in that frame need to > > decide to close. And if we want to close a specific bubble, then it > > doesn't matter what frame that bubble is in, it should get the close > > event. ANDing these two checks means that either 1 bubble will be asked > > to close or no bubbles will be asked. Neither of those results seems > > like desired behavior. We should either OR the condition checks, or > > forbid filtering by both at the same time to avoid unwanted behavior. > > If we also want to close all bubbles when bubble==owner==null, then we get the > following incoherent behavior: > > 0) Neither specified: close all bubbles > 1) One specified: close a few bubbles > 2) Both specified: close more bubbles. > > I hadn't noticed this until trying to implement the OR behavior. That is correct, you'll want: if (((!bubble && !owner) || (bubble == *i) || (owner && (*i)->OwningFrameIs(owner))) && (*i)->ShouldClose(reason)) { Maybe this is complex enough that the temp variable is justified.
On 2016/02/09 01:30:12, Hector Carmona wrote: > On 2016/02/09 01:24:36, Jeffrey Yasskin wrote: > > On 2016/02/09 00:59:44, Hector Carmona wrote: > > > > > > https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... > > > File components/bubble/bubble_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... > > > components/bubble/bubble_manager.cc:106: bool matches = true; > > > On 2016/02/09 00:42:41, Peter Kasting wrote: > > > > On 2016/02/09 00:31:05, Hector Carmona wrote: > > > > > On 2016/02/08 23:59:09, Jeffrey Yasskin wrote: > > > > > > On 2016/02/05 22:48:24, Hector Carmona wrote: > > > > > > > If both |match| and |frame| are passed in, should only bubbles that > > > match > > > > > both > > > > > > > conditions be closed? > > > > > > > > > > > > > > Or does it make sense to close any bubble that matches either > > condition? > > > > > > > > > > > > > > Or should only |match| or |frame| be specified, but not both? > > > > > > > > > > > > Right now, only one is ever specified. I lean toward matching both > > > > conditions > > > > > > (so that's what's implemented), but since we don't have any use cases > > for > > > > > > passing both, there's no way to tell. We could instead pass in a > lambda > > to > > > > > > control what matches, but that seems like overengineering for now. > > > > > > > > > > Agreed, lambda would be over-engineering. > > > > > > > > > > Can we add a DCHECK(!(bubble && owner)) if we don't expect |bubble| and > > > > |owner| > > > > > to both be passed in. This way, if they're both passed in, whoever > changes > > > it > > > > > will need to decide AND or OR as opposed to us deciding arbitrarily > right > > > now. > > > > > > > > Hmm. I'm a little uneasy about using DCHECK like this when there's no > real > > > > reason why callers can't or even shouldn't do this. > > > > > > > > I think the implemented behavior (force both to match if you specify both) > > is > > > > probably OK, so I would suggest simply making sure the API comments are > > clear > > > > about this case. If someone passes both and doesn't read the comments, > and > > > has > > > > a reason to want a different behavior, but doesn't check to see that their > > > code > > > > works, they kinda deserve what they get. > > > > > > I can't think of a reason we would specify a specific bubble to close > > > and a frame. If we have a frame, all bubbles in that frame need to > > > decide to close. And if we want to close a specific bubble, then it > > > doesn't matter what frame that bubble is in, it should get the close > > > event. ANDing these two checks means that either 1 bubble will be asked > > > to close or no bubbles will be asked. Neither of those results seems > > > like desired behavior. We should either OR the condition checks, or > > > forbid filtering by both at the same time to avoid unwanted behavior. > > > > If we also want to close all bubbles when bubble==owner==null, then we get the > > following incoherent behavior: > > > > 0) Neither specified: close all bubbles > > 1) One specified: close a few bubbles > > 2) Both specified: close more bubbles. > > > > I hadn't noticed this until trying to implement the OR behavior. > > That is correct, you'll want: > > if (((!bubble && !owner) || > (bubble == *i) || > (owner && (*i)->OwningFrameIs(owner))) && > (*i)->ShouldClose(reason)) { > > Maybe this is complex enough that the temp variable is justified. The problem is that specifying more of the arguments leads to non-monotonic behavior, which is hard to understand. I'm now much more comfortable DCHECKing that only one is specified than implementing the OR behavior.
On 2016/02/09 01:33:23, Jeffrey Yasskin wrote: > On 2016/02/09 01:30:12, Hector Carmona wrote: > > On 2016/02/09 01:24:36, Jeffrey Yasskin wrote: > > > On 2016/02/09 00:59:44, Hector Carmona wrote: > > > > > > > > > > https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... > > > > File components/bubble/bubble_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1572743002/diff/120001/components/bubble/bubb... > > > > components/bubble/bubble_manager.cc:106: bool matches = true; > > > > On 2016/02/09 00:42:41, Peter Kasting wrote: > > > > > On 2016/02/09 00:31:05, Hector Carmona wrote: > > > > > > On 2016/02/08 23:59:09, Jeffrey Yasskin wrote: > > > > > > > On 2016/02/05 22:48:24, Hector Carmona wrote: > > > > > > > > If both |match| and |frame| are passed in, should only bubbles > that > > > > match > > > > > > both > > > > > > > > conditions be closed? > > > > > > > > > > > > > > > > Or does it make sense to close any bubble that matches either > > > condition? > > > > > > > > > > > > > > > > Or should only |match| or |frame| be specified, but not both? > > > > > > > > > > > > > > Right now, only one is ever specified. I lean toward matching both > > > > > conditions > > > > > > > (so that's what's implemented), but since we don't have any use > cases > > > for > > > > > > > passing both, there's no way to tell. We could instead pass in a > > lambda > > > to > > > > > > > control what matches, but that seems like overengineering for now. > > > > > > > > > > > > Agreed, lambda would be over-engineering. > > > > > > > > > > > > Can we add a DCHECK(!(bubble && owner)) if we don't expect |bubble| > and > > > > > |owner| > > > > > > to both be passed in. This way, if they're both passed in, whoever > > changes > > > > it > > > > > > will need to decide AND or OR as opposed to us deciding arbitrarily > > right > > > > now. > > > > > > > > > > Hmm. I'm a little uneasy about using DCHECK like this when there's no > > real > > > > > reason why callers can't or even shouldn't do this. > > > > > > > > > > I think the implemented behavior (force both to match if you specify > both) > > > is > > > > > probably OK, so I would suggest simply making sure the API comments are > > > clear > > > > > about this case. If someone passes both and doesn't read the comments, > > and > > > > has > > > > > a reason to want a different behavior, but doesn't check to see that > their > > > > code > > > > > works, they kinda deserve what they get. > > > > > > > > I can't think of a reason we would specify a specific bubble to close > > > > and a frame. If we have a frame, all bubbles in that frame need to > > > > decide to close. And if we want to close a specific bubble, then it > > > > doesn't matter what frame that bubble is in, it should get the close > > > > event. ANDing these two checks means that either 1 bubble will be asked > > > > to close or no bubbles will be asked. Neither of those results seems > > > > like desired behavior. We should either OR the condition checks, or > > > > forbid filtering by both at the same time to avoid unwanted behavior. > > > > > > If we also want to close all bubbles when bubble==owner==null, then we get > the > > > following incoherent behavior: > > > > > > 0) Neither specified: close all bubbles > > > 1) One specified: close a few bubbles > > > 2) Both specified: close more bubbles. > > > > > > I hadn't noticed this until trying to implement the OR behavior. > > > > That is correct, you'll want: > > > > if (((!bubble && !owner) || > > (bubble == *i) || > > (owner && (*i)->OwningFrameIs(owner))) && > > (*i)->ShouldClose(reason)) { > > > > Maybe this is complex enough that the temp variable is justified. > > The problem is that specifying more of the arguments leads to non-monotonic > behavior, which is hard to understand. I'm now much more comfortable DCHECKing > that only one is specified than implementing the OR behavior. And I've uploaded a patch doing that, and with a non-Chrome BubbleManager test.
LGTM for components/bubble/* Thanks for the test :-)
Exposing detach in that way lgtm
LGTM https://codereview.chromium.org/1572743002/diff/180001/components/bubble/bubb... File components/bubble/bubble_manager.cc (right): https://codereview.chromium.org/1572743002/diff/180001/components/bubble/bubb... components/bubble/bubble_manager.cc:101: "bubble iff it's owned by a particular frame."; Nit: Rather than an output string here, I would leave a (more detailed) comment above the DCHECK explaining a little about why this doesn't make sense (from the info previously shared on the CR thread).
jyasskin@chromium.org changed reviewers: + asvitkine@chromium.org, reillyg@chromium.org
Reilly, PTAL at the changes to chrome/browser/usb. Alexei, PTAL at histograms.xml. Thanks! https://codereview.chromium.org/1572743002/diff/180001/components/bubble/bubb... File components/bubble/bubble_manager.cc (right): https://codereview.chromium.org/1572743002/diff/180001/components/bubble/bubb... components/bubble/bubble_manager.cc:101: "bubble iff it's owned by a particular frame."; On 2016/02/09 02:10:52, Peter Kasting wrote: > Nit: Rather than an output string here, I would leave a (more detailed) comment > above the DCHECK explaining a little about why this doesn't make sense (from the > info previously shared on the CR thread). Done; how's this comment look?
chrome/browser/usb lgtm
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572743002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572743002/200001
lgtm
The CQ bit was unchecked by jyasskin@chromium.org
The CQ bit was checked by jyasskin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, pkasting@chromium.org, hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/1572743002/#ps200001 (title: "Move DCHECK string into longer comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572743002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572743002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. Transient error: No JSON object could be decoded
The CQ bit was checked by jyasskin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572743002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572743002/200001
Message was sent while issue was closed.
Description was changed from ========== Make sure bubbles in Views close before their RenderFrameHosts. This is probably covered by navigation, but that isn't obvious from reading the code, so this change adds code to close bubbles directly. BUG=571466 ========== to ========== Make sure bubbles in Views close before their RenderFrameHosts. This is probably covered by navigation, but that isn't obvious from reading the code, so this change adds code to close bubbles directly. BUG=571466 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Make sure bubbles in Views close before their RenderFrameHosts. This is probably covered by navigation, but that isn't obvious from reading the code, so this change adds code to close bubbles directly. BUG=571466 ========== to ========== Make sure bubbles in Views close before their RenderFrameHosts. This is probably covered by navigation, but that isn't obvious from reading the code, so this change adds code to close bubbles directly. BUG=571466 Committed: https://crrev.com/4a12e67ac24b290e3da1ce7403bcd73be0a4dc8e Cr-Commit-Position: refs/heads/master@{#374503} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4a12e67ac24b290e3da1ce7403bcd73be0a4dc8e Cr-Commit-Position: refs/heads/master@{#374503} |
