Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
789763004:
Fix disappearing page actions bug. (Closed)
|
Created:
6 years ago by erikchen Modified:
5 years, 11 months ago Reviewers:
Mike Wittman, Peter Kasting, Jay Civelli, danduong, Robert Sesek, Scott Hess - ex-Googler CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix disappearing page actions bug.
The logic in LocationBarViewMac implicitly relied on the assumption that there
is a 1:1 correspondence between |page_actions_| and |page_action_decorations_|.
The implementation of DeletePageActionDecorations() cleared one vector but not
the other, breaking this implicit assumption and causing the bug. This CL
removes |page_actions_|, as the information is already contained within
|page_action_decorations_|, which fixes the bug.
The same was true for LocationBarView, modulo some minor renaming of members
and methods. There was a second bug in LocationBarView, where
InvalidatePageActions() deleted the page actions instead of refreshing them.
The documentation for InvalidatePageActions() and UpdatePageActions() implied
that the two methods were called at different times, and should have different
effects. Looking at the call sites, UpdatePageActions() was being called in
places where the documentation implied InvalidatePageActions() should be
called. This CL deletes InvalidatePageActions().
BUG=173055
TBR=jcivelli@chromium.org
Committed: https://crrev.com/545918a1b072eed5e15f28c772c682f9167b6677
Cr-Commit-Position: refs/heads/master@{#309797}
Patch Set 1 : Minor rename. #
Total comments: 11
Patch Set 2 : Significant clean up. #
Total comments: 4
Patch Set 3 : Comments from pkasting. #
Total comments: 6
Patch Set 4 : Comments from pkasting. #Patch Set 5 : Comments from wittman. #
Messages
Total messages: 42 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
erikchen@chromium.org changed reviewers: + danduong@chromium.org
Merry christmas!
danduong@chromium.org changed reviewers: + pkasting@chromium.org
Thanks, Erik!
https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:1089: page_action_views.clear(); This should be page_action_views_. Also we need to remove them from the view hierarchy.
wittman@chromium.org changed reviewers: + wittman@chromium.org
Thanks! Page actions are owned by the extension system, so we should just clear them rather than deleting.
danduong@chromium.org changed reviewers: - pkasting@chromium.org
I might have added pkastings prematurely. Erik will clean it up in the beginning of the week. https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.h:484: PageActionViews page_action_views_; page_action_views_ actually have a ref their corresponding page_action. I think we can just drop the page_actions_ altogether.
https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:686: page_action_decorations_.clear(); do you have to call [[field_ cell] clearDecorations] here?
https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (left): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:249: std::vector<ExtensionAction*> page_actions_; Same comment as the views implementation. We might want to drop this vector altogether.
https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:1297: DeletePageActionsAndViews(); Hah! I think the bug is actually here. This probably should be refresh page action views NOT delete. If you use Chrome's task manager and kill an extension's background page. This bug is 100% reproducible. Also makes the crash bug really easy to reproduce too.
https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:1297: DeletePageActionsAndViews(); On 2014/12/20 04:28:40, danduong wrote: > Hah! I think the bug is actually here. > > This probably should be refresh page action views NOT delete. If you use > Chrome's task manager and kill an extension's background page. This bug is 100% > reproducible. Also makes the crash bug really easy to reproduce too. I just realized that you commented on this in the bug. Is there any reason why we shouldn't just call Refresh at this point?
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
danduong: PTAL I rewrote the CL to incorporate your feedback. Check out the new CL description for a summary. https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (left): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:249: std::vector<ExtensionAction*> page_actions_; On 2014/12/20 03:22:14, danduong wrote: > Same comment as the views implementation. We might want to drop this vector > altogether. Done. https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:686: page_action_decorations_.clear(); On 2014/12/20 03:21:24, danduong wrote: > do you have to call [[field_ cell] clearDecorations] here? Yes, you're right. https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:1089: page_action_views.clear(); On 2014/12/20 03:09:22, danduong wrote: > This should be page_action_views_. Also we need to remove them from the view > hierarchy. Done. https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:1297: DeletePageActionsAndViews(); On 2014/12/20 04:59:04, danduong wrote: > On 2014/12/20 04:28:40, danduong wrote: > > Hah! I think the bug is actually here. > > > > This probably should be refresh page action views NOT delete. If you use > > Chrome's task manager and kill an extension's background page. This bug is > 100% > > reproducible. Also makes the crash bug really easy to reproduce too. > > I just realized that you commented on this in the bug. Is there any reason why > we shouldn't just call Refresh at this point? We should be calling Refresh. This is another bug. https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/789763004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.h:484: PageActionViews page_action_views_; On 2014/12/20 03:17:02, danduong wrote: > page_action_views_ actually have a ref their corresponding page_action. I think > we can just drop the page_actions_ altogether. Yup, done.
lgtm
erikchen@chromium.org changed reviewers: + pkasting@chromium.org
pkasting: Please review.
I'm not qualified to review the Mac stuff, but the views stuff LGTM. I confess I don't really understand the ownership model deeply enough to do more than a style review. I assume your other reviewers are covering that. https://codereview.chromium.org/789763004/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/789763004/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1309: RefreshPageActionViews(); Why not just inline the contents of this method here, and then call this function from the current callers of RefreshPageActionViews()? Optionally, we could rename this "RefreshPageActions()" or something to make it clearer what it does ("Invalidate" is somewhat vague). https://codereview.chromium.org/789763004/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/789763004/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:315: bool PageActionsDiffer(const std::vector<ExtensionAction*>& page_actions); Nit: Is there a typedef for this type?
Patchset #3 (id:120001) has been deleted
erikchen@chromium.org changed reviewers: + shess@chromium.org
shess: Looking for an OWNER review of chrome/browser/ui/cocoa/location_bar/* https://codereview.chromium.org/789763004/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/789763004/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1309: RefreshPageActionViews(); On 2014/12/23 00:20:05, Peter Kasting wrote: > Why not just inline the contents of this method here, and then call this > function from the current callers of RefreshPageActionViews()? > > Optionally, we could rename this "RefreshPageActions()" or something to make it > clearer what it does ("Invalidate" is somewhat vague). 1. I renamed InvalidatePageActions->RefreshPageActions. 2. Upon further consideration, the implementation of this method was incorrect. It should have performed a re-layout and re-paint, to match the implementation of UpdatePageActions(). https://codereview.chromium.org/789763004/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/789763004/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:315: bool PageActionsDiffer(const std::vector<ExtensionAction*>& page_actions); On 2014/12/23 00:20:05, Peter Kasting wrote: > Nit: Is there a typedef for this type? Yes. Changed to use the typedef.
erikchen@chromium.org changed reviewers: + jcivelli@chromium.org
jcivelli: Looking for an OWNER review of chrome/test/base/test_browser_window.h.
https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.h:58: // Updates the state of the page actions. So how are "update" and "refresh" different? We now have two methods that sound like they do the same thing, and in views at least, one just calls the other. Seems like these should just be one method?
https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.h:58: // Updates the state of the page actions. On 2014/12/23 01:00:42, Peter Kasting wrote: > So how are "update" and "refresh" different? We now have two methods that sound > like they do the same thing, and in views at least, one just calls the other. > > Seems like these should just be one method? I deleted Refresh/InvalidatePageActions, and updated the CL description. Looking at the call sites for both methods, UpdatePageActions() was being called where this documentation claimed that RefreshPageActions() should be called. Furthermore, the implementation is the same in views, and should be the same in cocoa.
lgtm https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1118: if (!page_action_views_.empty() && web_contents) { nit: this check can be removed entirely and the code inside can be run unconditionally if page_action_views_ is empty the loop will not run, and if web_contents is null, page_action_views_ will be empty https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:315: bool PageActionsDiffer(const PageActions& page_actions); make const?
Patchset #5 (id:150008) has been deleted
https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1118: if (!page_action_views_.empty() && web_contents) { On 2014/12/23 01:21:55, Mike Wittman wrote: > nit: this check can be removed entirely and the code inside can be run > unconditionally > > if page_action_views_ is empty the loop will not run, and if web_contents is > null, page_action_views_ will be empty Done. https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/789763004/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:315: bool PageActionsDiffer(const PageActions& page_actions); On 2014/12/23 01:21:56, Mike Wittman wrote: > make const? Done.
erikchen@chromium.org changed reviewers: + rsesek@chromium.org - shess@chromium.org
rsesek: Looking for an OWNER review of chrome/browser/ui/cocoa/*.
lgtm
jcivelli: ping?
TBR'ing jcivelli since the change to chrome/test/base/test_browser_window.h is just a 1-liner.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789763004/190001
Message was sent while issue was closed.
Committed patchset #5 (id:190001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/545918a1b072eed5e15f28c772c682f9167b6677 Cr-Commit-Position: refs/heads/master@{#309797}
Message was sent while issue was closed.
On 2014/12/23 00:55:04, erikchen wrote: > shess: Looking for an OWNER review of chrome/browser/ui/cocoa/location_bar/* Oops, sorry for missing this - in any case, ++LGTM++, I encourage you to dive into this code to clarify and add lightness, now that we're getting closer to one implementation. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
