GTK: Do not depend on the notification observer order...

chrome/browser/ui/toolbar/wrench_menu_model.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_TOOLBAR_WRENCH_MENU_MODEL_H_
 #define CHROME_BROWSER_UI_TOOLBAR_WRENCH_MENU_MODEL_H_
 
 #include "base/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
@@ skipping 104 matching lines @@
   // Getters.
   Browser* browser() const { return browser_; }
 
   BookmarkSubMenuModel* bookmark_sub_menu_model() const {
     return bookmark_sub_menu_model_.get();
   }
 
   // Calculates |zoom_label_| in response to a zoom change.
   void UpdateZoomControls();
 
+#if defined(TOOLKIT_GTK)

[sail, 2012/08/28 16:46:55]

I don't think you need this #ifdef.

[Lei Zhang, 2012/08/28 19:52:54]

No other platform needs this functionality right now though.

[sail, 2012/08/28 20:42:52]

I think that's ok. Even if this is unused the test will be cross platform and
that's enough.

+  // Rebuilds the model, rather than requiring the object to be recreated. Thus
+  // avoids having to re-add notification observers, which causes the ordering
+  // for notifications to be non-deterministic - http://crbug.com/118823.

[sail, 2012/08/28 16:46:55]

This is kind of vague. Why does the GTK code depend on notifications to be in a
certain order? Can't we fix that?

[Lei Zhang, 2012/08/28 19:52:54]

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.

[sail, 2012/08/28 20:42:52]

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.

+  void Rebuild();
+#endif
+
  private:
   // Testing constructor used for mocking.
   friend class ::MockWrenchMenuModel;
   WrenchMenuModel();
 
   void Build();
 
   void AddGlobalErrorMenuItems();
 
   // Appends everything needed for the clipboard menu: a menu break, the
@@ skipping 18 matching lines @@
 
   // Tools menu.
   scoped_ptr<ToolsMenuModel> tools_menu_model_;
 
   // Bookmark submenu.
   scoped_ptr<BookmarkSubMenuModel> bookmark_sub_menu_model_;
 
   ui::AcceleratorProvider* provider_;  // weak
 
   Browser* browser_;  // weak
-  TabStripModel* tab_strip_model_; // weak
+  TabStripModel* tab_strip_model_;  // weak
 
   content::NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(WrenchMenuModel);
 };
 
 #endif  // CHROME_BROWSER_UI_TOOLBAR_WRENCH_MENU_MODEL_H_