Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10892002:
GTK: Do not depend on the notification observer order... (Closed)
|
Created:
8 years, 3 months ago by Lei Zhang Modified:
8 years, 3 months ago Reviewers:
sail CC:
chromium-reviews Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionGTK: Do not depend on the notification observer order between WrenchMenu and WrenchMenuModel.
BUG=118823
TEST=Turn on sync, verify zoom buttons in the wrench menu work correctly.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156481
Patch Set 1 : #
Total comments: 8
Patch Set 2 : just call UpdateZoomControls instead #Messages
Total messages: 11 (0 generated)
This probably regressed in r104783. https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... chrome/browser/ui/toolbar/wrench_menu_model.cc:696: Clear(); SimpleMenu::Clear() says, "Note that it does not free MenuModel of submenu." Am I missing some cleanup here?
Hi Lei. Sorry for causing this regression. I'm not sure about how MenuModel works. Maybe just put a printf in the destructor for the submenu's model and verify that it gets called? Also, could you add a unit test that creates a menu model then calls Rebuild()? Thanks https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... File chrome/browser/ui/toolbar/wrench_menu_model.h (right): https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... chrome/browser/ui/toolbar/wrench_menu_model.h:125: #if defined(TOOLKIT_GTK) I don't think you need this #ifdef. https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... chrome/browser/ui/toolbar/wrench_menu_model.h:128: // for notifications to be non-deterministic - http://crbug.com/118823. This is kind of vague. Why does the GTK code depend on notifications to be in a certain order? Can't we fix that?
https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... File chrome/browser/ui/toolbar/wrench_menu_model.h (right): https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... chrome/browser/ui/toolbar/wrench_menu_model.h:125: #if defined(TOOLKIT_GTK) On 2012/08/28 16:46:55, sail wrote: > I don't think you need this #ifdef. No other platform needs this functionality right now though. https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... chrome/browser/ui/toolbar/wrench_menu_model.h:128: // for notifications to be non-deterministic - http://crbug.com/118823. On 2012/08/28 16:46:55, sail wrote: > This is kind of vague. Why does the GTK code depend on notifications to be in a > certain order? Can't we fix that? All platform depends on the zoom notification observers to process the zoom notification in the right order. The WrenchMenuModel needs to observe the notification first so it can update its internal zoom value. Then whoever draws the menu gets the notifications and fetches the value from WrenchMenuModel. Because we are destroying the WrenchMenuModel and recreating it on GTK, we end up recreating the observer, which then only gets the zoom notification after the redraw.
https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... File chrome/browser/ui/toolbar/wrench_menu_model.h (right): https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... chrome/browser/ui/toolbar/wrench_menu_model.h:125: #if defined(TOOLKIT_GTK) On 2012/08/28 19:52:54, Lei Zhang wrote: > On 2012/08/28 16:46:55, sail wrote: > > I don't think you need this #ifdef. > > No other platform needs this functionality right now though. I think that's ok. Even if this is unused the test will be cross platform and that's enough. https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/t... chrome/browser/ui/toolbar/wrench_menu_model.h:128: // for notifications to be non-deterministic - http://crbug.com/118823. On 2012/08/28 19:52:54, Lei Zhang wrote: > On 2012/08/28 16:46:55, sail wrote: > > This is kind of vague. Why does the GTK code depend on notifications to be in > a > > certain order? Can't we fix that? > > All platform depends on the zoom notification observers to process the zoom > notification in the right order. The WrenchMenuModel needs to observe the > notification first so it can update its internal zoom value. Then whoever draws > the menu gets the notifications and fetches the value from WrenchMenuModel. > > Because we are destroying the WrenchMenuModel and recreating it on GTK, we end > up recreating the observer, which then only gets the zoom notification after the > redraw. I think I understand but that still seems like a bug. It seems like what we want is a the menu model to have its own way to update the later observers.
https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/g... File chrome/browser/ui/gtk/browser_toolbar_gtk.cc (right): https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_toolbar_gtk.cc:415: wrench_menu_->UpdateMenu(); > It seems like what we want is a the menu model to have > its own way to update the later observers. The alternative to depending on the ordering is to just add a one-liner call to wrench_menu_model_->UpdateZoomLevel() here. Which is certainly easier, but it feels hacky too, since non of the other platforms re-creates their WrenchMenuModels. Who knows what other subtle breakages re-creating WrenchMenuModels can cause in the future?
On 2012/08/28 20:57:22, Lei Zhang wrote: > https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/g... > File chrome/browser/ui/gtk/browser_toolbar_gtk.cc (right): > > https://chromiumcodereview.appspot.com/10892002/diff/3002/chrome/browser/ui/g... > chrome/browser/ui/gtk/browser_toolbar_gtk.cc:415: wrench_menu_->UpdateMenu(); > > It seems like what we want is a the menu model to have > > its own way to update the later observers. > > The alternative to depending on the ordering is to just add a one-liner call to > wrench_menu_model_->UpdateZoomLevel() here. Which is certainly easier, but it > feels hacky too, since non of the other platforms re-creates their > WrenchMenuModels. Who knows what other subtle breakages re-creating > WrenchMenuModels can cause in the future? I think depending on notification order isn't very reliable. It'll be easy for people to break this again without realizing it. I'm ok with things either way though.
I'll just do it the simpler way.
On 2012/09/05 23:14:43, Lei Zhang wrote: > I'll just do it the simpler way. ping :)
On 2012/09/13 00:08:05, Lei Zhang wrote: > On 2012/09/05 23:14:43, Lei Zhang wrote: > > I'll just do it the simpler way. > > ping :) Oops, so sorry about not getting back to this. New patch LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/10892002/10001
Change committed as 156481 |
