Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23557004:
Save password bubble pops up automatically (Closed)
|
Created:
7 years, 3 months ago by npentrel Modified:
7 years, 3 months ago CC:
chromium-reviews, tfarina, markusheintz_ Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSave password bubble pops up automatically when the save password icon is triggered upon logging in to a page.
BUG=261628
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221414
Patch Set 1 #Patch Set 2 : Review 1 #Patch Set 3 : Review 2 #Patch Set 4 : Adding comment #
Total comments: 19
Patch Set 5 : Review 3 #Patch Set 6 : Readding UpdatePostLayout #
Total comments: 14
Patch Set 7 : Review 4 #Patch Set 8 : minor change #
Total comments: 20
Patch Set 9 : Review 5 #Patch Set 10 : minor change #
Messages
Total messages: 21 (0 generated)
Please have a look at the following CL. It makes the save password bubble pop up automatically if the save password icon is triggered.
At a glance, I'm not sure any of the plumbing here is right. * The tab_specific_content_settings code inserts an extra call to trigger this bubble. This seems unnecessary. The icon can probably trigger the bubble directly more simply. * Besides this, I'm never a fan of anything going through browser_commands.cc on principle. It's basically a huge collection of random global functions. There's usually a better way. * In general, LocationBarView should not be exposing this icon via an accessor. Such accessors are often a signal of something like a layering violation. (There are several such accessors already, and I'm not sure I've been given the opportunity to review them the way I should have.) It seems like instead of this patch, the icon should trigger its own bubble directly when needed. The relevant code from ContentSettingImageView::OnClick() could be factored out and then probably triggered from ContentSettingImageView::Update() when some appropriate TabSpecificContentSettings or ContentSettingImageModel getter said doing so was appropriate, much like how the animation is currently triggered if the explanatory_string_id() is nonzero.
One issue I faced when trying to trigger the bubble from ContentSettingImageView::Update() is that this method is called multiple times. When testing it, the bubble opens twice: once when it should, the other time when the user logs off a page and the icon is still visible for a split second. What I could do is have a boolean in content_setting_image_model that saves whether the bubble has already been called. Once the bubble is called this is set to true and no new bubble will be opened afterwards until the boolean is reset (which happens when the icon should be displayed the next time). Does this sound like an adequate solution for you or do you have a better idea for me to do this? Thanks Naomi On 2013/08/28 12:36:07, Peter Kasting wrote: > At a glance, I'm not sure any of the plumbing here is right. > > * The tab_specific_content_settings code inserts an extra call to trigger this > bubble. This seems unnecessary. The icon can probably trigger the bubble > directly more simply. > > * Besides this, I'm never a fan of anything going through browser_commands.cc on > principle. It's basically a huge collection of random global functions. > There's usually a better way. > > * In general, LocationBarView should not be exposing this icon via an accessor. > Such accessors are often a signal of something like a layering violation. > (There are several such accessors already, and I'm not sure I've been given the > opportunity to review them the way I should have.) > > It seems like instead of this patch, the icon should trigger its own bubble > directly when needed. The relevant code from > ContentSettingImageView::OnClick() could be factored out and then probably > triggered from ContentSettingImageView::Update() when some appropriate > TabSpecificContentSettings or ContentSettingImageModel getter said doing so was > appropriate, much like how the animation is currently triggered if the > explanatory_string_id() is nonzero.
On 2013/08/29 16:10:22, npentrel wrote: > One issue I faced when trying to trigger the bubble from > ContentSettingImageView::Update() is that this method is called multiple times. > When testing it, the bubble opens twice: once when it should, the other time > when the user logs off a page and the icon is still visible for a split second. I'd like to understand more detail about this second opening. This sounds like we have call order wrong somewhere or an extra Update() being triggered when it shouldn't be.
On 2013/08/29 18:19:48, Peter Kasting wrote: > On 2013/08/29 16:10:22, npentrel wrote: > > One issue I faced when trying to trigger the bubble from > > ContentSettingImageView::Update() is that this method is called multiple > times. > > When testing it, the bubble opens twice: once when it should, the other time > > when the user logs off a page and the icon is still visible for a split > second. > > I'd like to understand more detail about this second opening. This sounds like > we have call order wrong somewhere or an extra Update() being triggered when it > shouldn't be. Update is called both from LocationBarView, everytime a ContentSetting changes, and also from ToolBar, whenever that is updated, which explains the many Update calls. On first glance it doesn't seem to break anything if the call from ToolBar is removed but it could be breaking something. There is a problem with the visibility of the icon, i.e. the icon is not visible as soon as it is set to visible in the Update method. That means if I make the opening of the bubble dependent on IsBlockageIndicated(CONTENT_SETTING_TYPE_SAVE_PASSWORD), it pops up in the wrong place as the x and y coordinates of the icon's view are still set to 0. Apparently this is due to the scheduling that happens when SetVisible is called. The next time Update is called the coordinates are set and therefore the bubble could be opened in the right place. If we could fix that the icon is actually visible when it is set to visible, it wouldn't matter how often Update was called because I could base it on that and as soon as the blockage is indicated we would not open a new bubble. But as it currently is I cannot base it on IsBlockageIndicated and therefore the next bubble will open on the next navigation. Is there something we can do to fix this? If this is not possible, it would be best if we could have a boolean that would save whether the bubble has already popped up, which means it would be guaranteed that it only opens once.
On 2013/08/30 13:57:26, npentrel wrote: > On 2013/08/29 18:19:48, Peter Kasting wrote: > Update is called both from LocationBarView, everytime a ContentSetting changes, > and also from ToolBar, whenever that is updated, which explains the many Update > calls. On first glance it doesn't seem to break anything if the call from > ToolBar is removed but it could be breaking something. I'd spelunk the revision history of the Toolbar call and find out why it was added. Perhaps it's necessary so that we update our content settings appropriately when e.g. the current page is navigated, though I'd expect that to go through LocationBarView. > There is a problem with the visibility of the icon, i.e. the icon is not visible > as soon as it is set to visible in the Update method. That means if I make the > opening of the bubble dependent on > IsBlockageIndicated(CONTENT_SETTING_TYPE_SAVE_PASSWORD), it pops up in the wrong > place as the x and y coordinates of the icon's view are still set to 0. I presume that we can't address this in the obvious way (lay things out when something changes) because it's LocationBarView and not the icon itself that needs to do Layout(), and that's expensive do we only want to do it once all icons' visibility state is calculated. You could probably work around this by breaking things into two phases, one where we update the icon state, then after the LBV does Layout() a second phase where any icons that wanted to do something based on their new position and visibility (e.g. show a bubble) get a chance to do so. I don't know for sure, since I haven't stepped through the code. It seems like this problem generally ought to be tractable enough that you can come up with some kind of a functional solution.
As you suggested, I broke the issue down into two phases. After Layout is called, I now add the bubble. Let me know what you think.
Seems like these methods should be called UpdatePreLayout() and UpdatePostLayout(), or maybe UpdatePreParentLayout() and UpdatePostParentLayout(). This emphasizes why the phases are broken the way they are. You can also add comments in the code noting that the bubble has to be opened post-layout so it can target the correct screen coordinates. This will also force an audit of all the places that currently call Update(). For example, LocationBarView::Update() ends up calling Update() without subsequently calling your new method. Is that bad? It's not clear to me why the post-layout method needs to update the content settings model again. What happens if the content setting icons move around while the bubble is open? Does it move too?
On 2013/09/03 21:32:43, Peter Kasting wrote: > Seems like these methods should be called UpdatePreLayout() and > UpdatePostLayout(), or maybe UpdatePreParentLayout() and > UpdatePostParentLayout(). This emphasizes why the phases are broken the way > they are. You can also add comments in the code noting that the bubble has to > be opened post-layout so it can target the correct screen coordinates. Agreed, and done, I will add more comments tomorrow. > This will also force an audit of all the places that currently call Update(). > For example, LocationBarView::Update() ends up calling Update() without > subsequently calling your new method. Is that bad? The bubble should only show up when LocationBarView::UpdateContentSettingsIcons was called, as this is the only time the save password icon can start showing. Therefore, it makes sense for other updates not to call the new method. > It's not clear to me why the post-layout method needs to update the content > settings model again. Removed it. > What happens if the content setting icons move around while the bubble is open? > Does it move too? I don't think this should happen. If new content setting icons are added they will appear to the left of the current icon. That is what I have seen so far while testing it.
https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:97: void ContentSettingImageView::UpdatePostLayout( Function definition order needs to match declaration order. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:99: if (!content_setting_image_model_->is_visible()) { Nit: No {} https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:103: TabSpecificContentSettings* content_settings = web_contents ? Nit: This should be done inside the below conditional https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:107: CONTENT_SETTINGS_TYPE_SAVE_PASSWORD) { It would be nice if this was more generic than "is this a save password icon"... perhaps some kind of "should auto-show bubble on blockage" method that the save password model overrides to return true. Nit: Maybe reverse this and early return (but see comment in next function) https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:163: // bubble pops up. Nit: Say why, not what. This comment currently just restates the code; instead talk about why this needs to be, i.e. that the bubble needs to be anchored by the laid-out icon, which isn't positioned correctly until after this returns. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:167: content_setting_image_model_->get_content_settings_type()); I wonder if for consistency's sake, we should move this out of this function entirely and make the post-layout function more like: if (should show bubble) show bubble; set blockage has been indicated; This way, we'd always call the "set indicated" method at the same time in the call chain. This might reduce the chance of weird timing bugs later? https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.h:46: // icons. This comment seems like it's on the wrong function. Nit: You might also want something slightly more generic here, and then a more specific comment in the implementation. Something like "Perform any updates which depend on the image having already been laid out by the owning LocationBarView." https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:1138: void LocationBarView::RefreshContentSettingViews() { I realize you said that only one place can make the Save Password icon start showing. But it's not obvious to me why it's technically impossible for that to happen at any other callsite of this function. It's also not clear to me that, even if that holds, we won't make another change in the future to add an UpdatePostLayout() action for some other content setting which _can_ change at another callsite. So I'm still very worried about how this function can be called from multiple places, but neither it nor all callers ensure UpdatePostLayout() gets called. This either needs fixing, or a lot more commentary about why it's not only correct now but guaranteed to hold in the future. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:1146: void LocationBarView::UpdatePostLayout() { I think calling this UpdatePostLayout() is somewhat misleading. Since it's only called in one place, maybe we should just avoid the problem of finding a good name and inline it into the caller.
https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:97: void ContentSettingImageView::UpdatePostLayout( On 2013/09/03 23:50:43, Peter Kasting wrote: > Function definition order needs to match declaration order. Done. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:99: if (!content_setting_image_model_->is_visible()) { On 2013/09/03 23:50:43, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:103: TabSpecificContentSettings* content_settings = web_contents ? On 2013/09/03 23:50:43, Peter Kasting wrote: > Nit: This should be done inside the below conditional Done. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:107: CONTENT_SETTINGS_TYPE_SAVE_PASSWORD) { On 2013/09/03 23:50:43, Peter Kasting wrote: > It would be nice if this was more generic than "is this a save password icon"... > perhaps some kind of "should auto-show bubble on blockage" method that the save > password model overrides to return true. > > Nit: Maybe reverse this and early return (but see comment in next function) Done. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:163: // bubble pops up. On 2013/09/03 23:50:43, Peter Kasting wrote: > Nit: Say why, not what. This comment currently just restates the code; instead > talk about why this needs to be, i.e. that the bubble needs to be anchored by > the laid-out icon, which isn't positioned correctly until after this returns. Done. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:167: content_setting_image_model_->get_content_settings_type()); On 2013/09/03 23:50:43, Peter Kasting wrote: > I wonder if for consistency's sake, we should move this out of this function > entirely and make the post-layout function more like: > > if (should show bubble) > show bubble; > set blockage has been indicated; > > This way, we'd always call the "set indicated" method at the same time in the > call chain. This might reduce the chance of weird timing bugs later? Done. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.h:46: // icons. On 2013/09/03 23:50:43, Peter Kasting wrote: > This comment seems like it's on the wrong function. > > Nit: You might also want something slightly more generic here, and then a more > specific comment in the implementation. Something like "Perform any updates > which depend on the image having already been laid out by the owning > LocationBarView." Done. https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:1138: void LocationBarView::RefreshContentSettingViews() { On 2013/09/03 23:50:43, Peter Kasting wrote: > I realize you said that only one place can make the Save Password icon start > showing. But it's not obvious to me why it's technically impossible for that to > happen at any other callsite of this function. It's also not clear to me that, > even if that holds, we won't make another change in the future to add an > UpdatePostLayout() action for some other content setting which _can_ change at > another callsite. > > So I'm still very worried about how this function can be called from multiple > places, but neither it nor all callers ensure UpdatePostLayout() gets called. > This either needs fixing, or a lot more commentary about why it's not only > correct now but guaranteed to hold in the future. In the other place where RefreshCOntentSettingViews() is called, we do not call layout as far as I can see. So it makes little sense to call UpdatePostLayout, if layouting doesn't happen. Am I wrong? https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:1146: void LocationBarView::UpdatePostLayout() { On 2013/09/03 23:50:43, Peter Kasting wrote: > I think calling this UpdatePostLayout() is somewhat misleading. > > Since it's only called in one place, maybe we should just avoid the problem of > finding a good name and inline it into the caller. Done.
https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/23557004/diff/29002/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:1138: void LocationBarView::RefreshContentSettingViews() { On 2013/09/04 09:55:36, npentrel wrote: > On 2013/09/03 23:50:43, Peter Kasting wrote: > > I realize you said that only one place can make the Save Password icon start > > showing. But it's not obvious to me why it's technically impossible for that > to > > happen at any other callsite of this function. It's also not clear to me > that, > > even if that holds, we won't make another change in the future to add an > > UpdatePostLayout() action for some other content setting which _can_ change at > > another callsite. > > > > So I'm still very worried about how this function can be called from multiple > > places, but neither it nor all callers ensure UpdatePostLayout() gets called. > > This either needs fixing, or a lot more commentary about why it's not only > > correct now but guaranteed to hold in the future. > > In the other place where RefreshCOntentSettingViews() is called, we do not call > layout as far as I can see. So it makes little sense to call UpdatePostLayout, > if layouting doesn't happen. Am I wrong? Never mind, it is called. I added the call to UpdatePostLayout.
https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_image_model.cc:67: virtual bool ShouldShowBubbleOnBlockage(); OVERRIDE https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:143: content_setting_image_model_->get_content_settings_type())) { Nit: Indent 4, not 9 https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:145: if (web_contents && !bubble_widget_) { |web_contents| cannot be NULL here or we'd have returned above. When is |bubble_widget_| non-NULL? Is it possible to have a visible bubble but IsBlockageIndicated() returns false? Nit: Please factor the code here and in OnClick() out to a helper function instead of copy-and-pasting. https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:156: content_settings->SetBlockageHasBeenIndicated( This statement is one layer too deep. https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:389: OnChanged(); This call can be nuked since it happens at the end of Update(). https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:1139: void LocationBarView::RefreshContentSettingViews() { Nit: Why don't e name these functions UpdateContentSettingViewsPreLayout() and UpdateContentSettingViewsPostLayout().
https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_image_model.cc (right): https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_image_model.cc:67: virtual bool ShouldShowBubbleOnBlockage(); On 2013/09/04 20:59:11, Peter Kasting wrote: > OVERRIDE Done. https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:143: content_setting_image_model_->get_content_settings_type())) { On 2013/09/04 20:59:11, Peter Kasting wrote: > Nit: Indent 4, not 9 Done. https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:145: if (web_contents && !bubble_widget_) { On 2013/09/04 20:59:11, Peter Kasting wrote: > |web_contents| cannot be NULL here or we'd have returned above. true. > > When is |bubble_widget_| non-NULL? Is it possible to have a visible bubble but > IsBlockageIndicated() returns false? It can't be visible while IsBlockageIndicated is false, you're right. > Nit: Please factor the code here and in OnClick() out to a helper function > instead of copy-and-pasting. Done https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:156: content_settings->SetBlockageHasBeenIndicated( On 2013/09/04 20:59:11, Peter Kasting wrote: > This statement is one layer too deep. Done. https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:389: OnChanged(); On 2013/09/04 20:59:11, Peter Kasting wrote: > This call can be nuked since it happens at the end of Update(). I don't think I understand what you mean. https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:1139: void LocationBarView::RefreshContentSettingViews() { On 2013/09/04 20:59:11, Peter Kasting wrote: > Nit: Why don't e name these functions UpdateContentSettingViewsPreLayout() and > UpdateContentSettingViewsPostLayout(). Done.
https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:389: OnChanged(); On 2013/09/04 21:50:18, npentrel wrote: > On 2013/09/04 20:59:11, Peter Kasting wrote: > > This call can be nuked since it happens at the end of Update(). > > I don't think I understand what you mean. Update(NULL); // This calls OnChanged() directly before returning. OnChanged(); // This isn't necessary, Update(NULL) just called it. Remove.
https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/23557004/diff/30005/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:389: OnChanged(); On 2013/09/04 21:52:01, Peter Kasting wrote: > On 2013/09/04 21:50:18, npentrel wrote: > > On 2013/09/04 20:59:11, Peter Kasting wrote: > > > This call can be nuked since it happens at the end of Update(). > > > > I don't think I understand what you mean. > > Update(NULL); // This calls OnChanged() directly before returning. > OnChanged(); // This isn't necessary, Update(NULL) just called it. Remove. Done.
LGTM https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_image_model.h:32: // setting is indicated. Nit: Active voice: Returns true if the icon should automatically show its bubble as part of indicating that the relevant content was blocked. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:131: } Nit: Add comment at end of function: Since indicating blockage may include showing a bubble, which must be done in UpdatePostLayout() so the bubble has the the right anchor coordinates, we delay calling SetBlockageHasBeeenIndicated() until that function completes. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:142: if (!content_settings->IsBlockageIndicated( Nit: Either put a blank line here, or remove the one above https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:144: if (content_setting_image_model_->ShouldShowBubbleOnBlockage()) { Nit: No {} https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.h:44: // Update the decoration from the shown WebContents. Nit: While here: Function comments should be descriptive, not imperative ("Updates", "Performs", not "Update", "Perform") (2 places) https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:523: OnChanged(); Nit: Add EOL comment: // NOTE: Calls Layout(). https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:529: Nit: Remove blank line https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.h:386: // Update the visibility state of the Content Blocked icons to reflect what is Nit: Update -> Updates https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.h:387: // actually blocked on the current page. Nit: Add to end: Calling this function should always eventually be followed by calling Layout() and then UpdateContentSettingViewsPostLayout(), to ensure the icons can completely update their state. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.h:390: // Updates after the correct screen coordinates have been set for icons. Nit: How about: Allows content setting icons to perform any updating which can't complete until after the icons have been correctly laid out. This should be called after UpdateContentSettingViewsPreLayout() and a subsequent Layout().
https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_image_model.h (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_image_model.h:32: // setting is indicated. On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: Active voice: > > Returns true if the icon should automatically show its bubble as part of > indicating that the relevant content was blocked. Done. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:131: } On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: Add comment at end of function: > > Since indicating blockage may include showing a bubble, which must be done in > UpdatePostLayout() so the bubble has the the right anchor coordinates, we delay > calling SetBlockageHasBeeenIndicated() until that function completes. Done. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:142: if (!content_settings->IsBlockageIndicated( On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: Either put a blank line here, or remove the one above Done. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:144: if (content_setting_image_model_->ShouldShowBubbleOnBlockage()) { On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/content_setting_image_view.h:44: // Update the decoration from the shown WebContents. On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: While here: Function comments should be descriptive, not imperative > ("Updates", "Performs", not "Update", "Perform") (2 places) Done. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:523: OnChanged(); On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: Add EOL comment: // NOTE: Calls Layout(). Done. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:529: On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: Remove blank line Done. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.h:386: // Update the visibility state of the Content Blocked icons to reflect what is On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: Update -> Updates Done. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.h:387: // actually blocked on the current page. On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: Add to end: > > Calling this function should always eventually be followed by calling Layout() > and then UpdateContentSettingViewsPostLayout(), to ensure the icons can > completely update their state. Done. https://codereview.chromium.org/23557004/diff/53001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.h:390: // Updates after the correct screen coordinates have been set for icons. On 2013/09/05 06:55:19, Peter Kasting wrote: > Nit: How about: > > Allows content setting icons to perform any updating which can't complete until > after the icons have been correctly laid out. This should be called after > UpdateContentSettingViewsPreLayout() and a subsequent Layout(). Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/npentrel@chromium.org/23557004/58001
Message was sent while issue was closed.
Change committed as 221414 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
