Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11009013:
NTP5: Starting implementation of a native menu for showing other device sessions. (Closed)
|
Created:
8 years, 2 months ago by jeremycho Modified:
8 years, 1 month ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, dhollowa+watch_chromium.org, arv (Not doing code reviews), estade+watch_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNTP5: Starting implementation of a native menu for showing other device sessions.
BUG=148770
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164578
Patch Set 1 #
Total comments: 32
Patch Set 2 : Respond to Dave's comments. #
Total comments: 4
Patch Set 3 : Respond to Dave's comments. #Patch Set 4 : Rebase and get rid of testing code. #Patch Set 5 : Use screenX instead of clientX. #
Total comments: 24
Patch Set 6 : Addressing Vadim's comment. #
Total comments: 2
Patch Set 7 : Respond to comments. #
Total comments: 6
Patch Set 8 : Respond to comments. #
Total comments: 4
Patch Set 9 : Respond to comments. #
Total comments: 6
Patch Set 10 : Respond to comments. #
Total comments: 20
Patch Set 11 : Rebase #Patch Set 12 : Respond to Dan's comments. #
Total comments: 25
Patch Set 13 : Respond to comments. #Patch Set 14 : Decouple model from view. #Patch Set 15 : Typo fix. #
Total comments: 2
Patch Set 16 : Respond to comments. #
Total comments: 6
Patch Set 17 : Responding to comments. #
Total comments: 2
Patch Set 18 : Rebase and respond to comments. #Patch Set 19 : Rebase and exclude Android. #Patch Set 20 : Fix dumb typo. #
Total comments: 12
Patch Set 21 : Respond to tfarina's comments. #Messages
Total messages: 47 (0 generated)
Hi Pedro and Vadim, This is a starting point for the native menu which shows other device sessions. It's missing some features (e.g. favicons), but I wanted feedback before proceeding further (this is my first major Chrome C++ CL). Some of the code is just for testing and won't be submitted (this way, you shouldn't have to patch any additional CLs). Once 10996064 and 11003002 are in, I'll use the functionality provided there instead.
http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:8: #include "base/utf_string_conversions.h" This should be removed. We'll put the string in resources instead. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:38: // TODO(jeremycho): Limit the number of menu items? I see browser_sync::kMaxSessionsToShow. Use that? And maybe a create a local |const int kMaxTabsToShow = 256;| locally to prevent DoS'ing the menu? http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:39: if (associator && associator->GetForeignSession(session_id, &windows)) { Should |associator| ever be NULL here? If not prefer a DCHECK(associator); http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:41: bool lastWindowHasTabs = false; C++ style is: |last_window_has_tabs|. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:62: // TODO(jeremycho): Use tab_value.GetString("url", &url) to Please add a link to associated bug in this TODO. I.e.: // TODO(jeremycho): http://crbug.com/12345 ... http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:81: menu_model_.AddItem(command_id, UTF8ToUTF16("Show all")); Let's do this now. We shouldn't be hard coding strings into source. See a good example of how to do this in |BackgroundModeManager::BackgroundModeData::BuildProfileMenu| and store the string in src/chrome/app/generated_resources.grd http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:91: content::WebContents* web_contents = web_ui_->GetWebContents(); Is it possible to get a NULL result back here? I'd be concerned that the web_contents, browser, and/or widget could be NULL if the menu is somehow triggered during a tear-down sequence. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:98: if (menu_runner_->RunMenuAt(widget, NULL, gfx::Rect(location_, gfx::Size()), Prefer: RunResult result = menu_runner_->RunMenuAt(...); DCHECK_EQ(result, MENU_DELETED); Maybe with comment as to why you expect this. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:118: if (session_data_.count(command_id) == 0) { Chrome style typically does not mask logic errors with |return| like you have here. We prefer to DCHECK and crash so we see things turn up in the crash logs, and we avoid silent failures. So, this should be simply: DCHECK_EQ(session_data_.count(command_id), 0) << "Invalid..."; http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:136: // TODO(jeremycho): Figure out what we want to log. If in doubt, log nothing. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:25: typedef std::map<int, linked_ptr<DictionaryValue> > SessionData; A comment here would be nice. What does the |int| represent? What's in the dictionary? http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:25: typedef std::map<int, linked_ptr<DictionaryValue> > SessionData; This should be pulled inside the class - possibly inside the private section if it is not used externally. Chrome generally follows the "principle of narrowest scope". http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:27: class OtherDeviceMenu : public ui::SimpleMenuModel::Delegate { Seems like |OtherDevicesMenu| would be more appropriate since it can hold plural devices. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:27: class OtherDeviceMenu : public ui::SimpleMenuModel::Delegate { A brief comment for the class would be helpful. Why it needs to exist, its relation to search_ntp, etc. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:48: content::WebUI* web_ui_; Comment please, indicating ownership, such as "Weak." http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:50: const gfx::Point& location_; Comment please. Origin of top-left of menu, or where mouse down occurred? Maybe same?
Thanks for the helpful comments! http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:8: #include "base/utf_string_conversions.h" On 2012/10/01 17:37:38, dhollowa wrote: > This should be removed. We'll put the string in resources instead. Done. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:38: // TODO(jeremycho): Limit the number of menu items? On 2012/10/01 17:37:38, dhollowa wrote: > I see browser_sync::kMaxSessionsToShow. Use that? And maybe a create a local > |const int kMaxTabsToShow = 256;| locally to prevent DoS'ing the menu? A menu should only contain a single session. Added kMaxTabsToShow. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:39: if (associator && associator->GetForeignSession(session_id, &windows)) { On 2012/10/01 17:37:38, dhollowa wrote: > Should |associator| ever be NULL here? If not prefer a DCHECK(associator); Yes it can be, e.g. if it hasn't finished syncing. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:41: bool lastWindowHasTabs = false; On 2012/10/01 17:37:38, dhollowa wrote: > C++ style is: |last_window_has_tabs|. Done. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:62: // TODO(jeremycho): Use tab_value.GetString("url", &url) to On 2012/10/01 17:37:38, dhollowa wrote: > Please add a link to associated bug in this TODO. I.e.: > // TODO(jeremycho): http://crbug.com/12345 ... Done. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:81: menu_model_.AddItem(command_id, UTF8ToUTF16("Show all")); On 2012/10/01 17:37:38, dhollowa wrote: > Let's do this now. We shouldn't be hard coding strings into source. See a good > example of how to do this in > |BackgroundModeManager::BackgroundModeData::BuildProfileMenu| and store the > string in src/chrome/app/generated_resources.grd Reused the existing string from NTP4's other device menu. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:91: content::WebContents* web_contents = web_ui_->GetWebContents(); On 2012/10/01 17:37:38, dhollowa wrote: > Is it possible to get a NULL result back here? I'd be concerned that the > web_contents, browser, and/or widget could be NULL if the menu is somehow > triggered during a tear-down sequence. WebContents* shouldn't be NULL, but the others could. Done. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:98: if (menu_runner_->RunMenuAt(widget, NULL, gfx::Rect(location_, gfx::Size()), On 2012/10/01 17:37:38, dhollowa wrote: > Prefer: RunResult result = menu_runner_->RunMenuAt(...); > DCHECK_EQ(result, MENU_DELETED); > Maybe with comment as to why you expect this. We don't expect that necessarily - only if the MenuRunner was deleted. This idiom is used throughout, e.g.: http://code.google.com/codesearch#OAMlx_jo-ck/src/ash/shell_context_menu.cc&e... http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:118: if (session_data_.count(command_id) == 0) { On 2012/10/01 17:37:38, dhollowa wrote: > Chrome style typically does not mask logic errors with |return| like you have > here. We prefer to DCHECK and crash so we see things turn up in the crash logs, > and we avoid silent failures. So, this should be simply: > > DCHECK_EQ(session_data_.count(command_id), 0) << "Invalid..."; Done. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.cc:136: // TODO(jeremycho): Figure out what we want to log. On 2012/10/01 17:37:38, dhollowa wrote: > If in doubt, log nothing. other_sessions.js logs session clicks, so most likely we will want to as well, but we may want additional data too (e.g. tab index). http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:25: typedef std::map<int, linked_ptr<DictionaryValue> > SessionData; On 2012/10/01 17:37:38, dhollowa wrote: > This should be pulled inside the class - possibly inside the private section if > it is not used externally. Chrome generally follows the "principle of narrowest > scope". Done. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:25: typedef std::map<int, linked_ptr<DictionaryValue> > SessionData; On 2012/10/01 17:37:38, dhollowa wrote: > A comment here would be nice. What does the |int| represent? What's in the > dictionary? Done. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:27: class OtherDeviceMenu : public ui::SimpleMenuModel::Delegate { On 2012/10/01 17:37:38, dhollowa wrote: > Seems like |OtherDevicesMenu| would be more appropriate since it can hold plural > devices. This menu will only hold the session of a single device. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:27: class OtherDeviceMenu : public ui::SimpleMenuModel::Delegate { On 2012/10/01 17:37:38, dhollowa wrote: > A brief comment for the class would be helpful. Why it needs to exist, its > relation to search_ntp, etc. Done. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:48: content::WebUI* web_ui_; On 2012/10/01 17:37:38, dhollowa wrote: > Comment please, indicating ownership, such as "Weak." Done. http://codereview.chromium.org/11009013/diff/1/chrome/browser/ui/search/other... chrome/browser/ui/search/other_device_menu.h:50: const gfx::Point& location_; On 2012/10/01 17:37:38, dhollowa wrote: > Comment please. Origin of top-left of menu, or where mouse down occurred? > Maybe same? Done.
http://codereview.chromium.org/11009013/diff/4003/chrome/browser/ui/search/ot... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/4003/chrome/browser/ui/search/ot... chrome/browser/ui/search/other_device_menu.cc:66: if (browser) { nit: you can avoid nesting with pattern: if (!browser) return; ... if (!widget) return; http://codereview.chromium.org/11009013/diff/4003/chrome/browser/ui/search/ot... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/4003/chrome/browser/ui/search/ot... chrome/browser/ui/search/other_device_menu.h:63: typedef std::vector<linked_ptr<DictionaryValue> > TabData; nit: typedefs usually live just under their enclosing public/protected/private section (line 51).
http://codereview.chromium.org/11009013/diff/4003/chrome/browser/ui/search/ot... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/4003/chrome/browser/ui/search/ot... chrome/browser/ui/search/other_device_menu.cc:66: if (browser) { On 2012/10/02 00:02:08, dhollowa wrote: > nit: you can avoid nesting with pattern: > if (!browser) > return; > ... > if (!widget) > return; Done. http://codereview.chromium.org/11009013/diff/4003/chrome/browser/ui/search/ot... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/4003/chrome/browser/ui/search/ot... chrome/browser/ui/search/other_device_menu.h:63: typedef std::vector<linked_ptr<DictionaryValue> > TabData; On 2012/10/02 00:02:08, dhollowa wrote: > nit: typedefs usually live just under their enclosing public/protected/private > section (line 51). Done.
This is ready for another look - I merged with Vadim's changes and deleted the test code. Per-device sessions are now being shown. Thanks!
http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:344: // Extract horizontal coordinate of the click within the application's client Since you've switched in JS from client coordinates to screen coordinates, could you update comments in C++?
http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:344: // Extract horizontal coordinate of the click within the application's client On 2012/10/08 18:10:18, vadimt wrote: > Since you've switched in JS from client coordinates to screen coordinates, could > you update comments in C++? Done.
http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:14: #include "chrome/browser/sync/glue/session_model_associator.h" nit: order http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:23: using browser_sync::ForeignSessionHandler; nit: please remove these using directives. The trend on Chrome is to stick with full qualification of namespaces. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:27: // TODO(jeremycho): Discuss details with UX, e.g. is it prefereable to show only Please log a bug for this TODO on this and cite it here as http://crbug.com/12345 http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:113: // TODO(jeremycho): Figure out what to log. Will you resolve this in the current CL? http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:117: int command_id, nit: indent seems off here. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:31: nit: remove blank line. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:57: const std::string& session_id_; This should be a copy not a reference. You're currently taking a reference to a temporary variable when you construct this at the call site. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:59: const gfx::Point& location_; Similarly, this should be a copy not a reference. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:107: int selected_index = tab.current_navigation_index; nit: combine lines 107 and 108? http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:122: // TODO(jeremycho): Rename to tabId? What's the answer?
http://codereview.chromium.org/11009013/diff/23001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/23001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:346: double window_x; Hi Jeremy! Thanks for the correction! However, per this doc (http://www.w3.org/TR/DOM-Level-3-Events/#events-mouseevents), screenX: The horizontal coordinate at which the event occurred relative to the origin of the screen coordinate system. screenY: The vertical coordinate at which the event occurred relative to the origin of the screen coordinate system. Based on this, it seems reasonable for me to rephrase the comment and variable name to use "screen" word instead of "window"... Thanks!
http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:14: #include "chrome/browser/sync/glue/session_model_associator.h" On 2012/10/08 19:15:09, dhollowa wrote: > nit: order Done. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:23: using browser_sync::ForeignSessionHandler; On 2012/10/08 19:15:09, dhollowa wrote: > nit: please remove these using directives. The trend on Chrome is to stick with > full qualification of namespaces. Done. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:27: // TODO(jeremycho): Discuss details with UX, e.g. is it prefereable to show only On 2012/10/08 19:15:09, dhollowa wrote: > Please log a bug for this TODO on this and cite it here as > http://crbug.com/12345 Look like we have resolution on this issue. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:113: // TODO(jeremycho): Figure out what to log. On 2012/10/08 19:15:09, dhollowa wrote: > Will you resolve this in the current CL? Done. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:117: int command_id, On 2012/10/08 19:15:09, dhollowa wrote: > nit: indent seems off here. Done. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:31: On 2012/10/08 19:15:09, dhollowa wrote: > nit: remove blank line. Done. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:57: const std::string& session_id_; On 2012/10/08 19:15:09, dhollowa wrote: > This should be a copy not a reference. You're currently taking a reference to a > temporary variable when you construct this at the call site. Done. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:59: const gfx::Point& location_; On 2012/10/08 19:15:09, dhollowa wrote: > Similarly, this should be a copy not a reference. Done. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:107: int selected_index = tab.current_navigation_index; On 2012/10/08 19:15:09, dhollowa wrote: > nit: combine lines 107 and 108? Done. http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:122: // TODO(jeremycho): Rename to tabId? On 2012/10/08 19:15:09, dhollowa wrote: > What's the answer? I think it makes sense, but this name is used in the JS, other handlers, and possibly other places, so I prefer to that separately. http://codereview.chromium.org/11009013/diff/23001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/23001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:346: double window_x; Sorry, my bad. I had screen in the JS too... On 2012/10/08 19:39:12, vadimt wrote: > Hi Jeremy! Thanks for the correction! > > However, per this doc > (http://www.w3.org/TR/DOM-Level-3-Events/#events-mouseevents), > > screenX: > The horizontal coordinate at which the event occurred relative to the origin of > the screen coordinate system. > > screenY: > The vertical coordinate at which the event occurred relative to the origin of > the screen coordinate system. > > Based on this, it seems reasonable for me to rephrase the comment and variable > name to use "screen" word instead of "window"... > > Thanks!
http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:122: // TODO(jeremycho): Rename to tabId? On 2012/10/09 01:52:13, jeremycho wrote: > On 2012/10/08 19:15:09, dhollowa wrote: > > What's the answer? > I think it makes sense, but this name is used in the JS, other handlers, and > possibly other places, so I prefer to that separately. Ok, then please reflect this intention in the comment. Optionally track it with a bug if you think it might get forgotten. http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:29: // Enumerates the different menu item types. This enum should be private, or better yet declared inside an anonymous namespace within the .cc file. I.e.: namespace { enum ItemType { ... } } // namespace http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:42: std::string session_id, These should remain const references. Just the member variables needed to change.
http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:343: // Extract horizontal coordinate of the click within the application's screen Jeremy, I'm really sorry, I feel that I'm evil, but this wording is still not accurate. There is no application's screen area. Can you rewrite this as: Extract horizontal coordinate of the click relative to the origin of the screen coordinate system. etc.
http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/18001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:122: // TODO(jeremycho): Rename to tabId? On 2012/10/09 15:13:42, dhollowa wrote: > On 2012/10/09 01:52:13, jeremycho wrote: > > On 2012/10/08 19:15:09, dhollowa wrote: > > > What's the answer? > > I think it makes sense, but this name is used in the JS, other handlers, and > > possibly other places, so I prefer to that separately. > > Ok, then please reflect this intention in the comment. Optionally track it with > a bug if you think it might get forgotten. Done. http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:29: // Enumerates the different menu item types. On 2012/10/09 15:13:42, dhollowa wrote: > This enum should be private, or better yet declared inside an anonymous > namespace within the .cc file. I.e.: > > namespace { > enum ItemType { > ... > } > } // namespace Done. http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:42: std::string session_id, Ah sorry. Done. On 2012/10/09 15:13:42, dhollowa wrote: > These should remain const references. Just the member variables needed to > change. http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/23003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:343: // Extract horizontal coordinate of the click within the application's screen No it's my bad - I should have read your comment more closely. Hopefully done now :) On 2012/10/09 18:21:45, vadimt wrote: > Jeremy, I'm really sorry, I feel that I'm evil, but this wording is still not > accurate. There is no application's screen area. Can you rewrite this as: > > Extract horizontal coordinate of the click relative to the origin of the screen > coordinate system. > > etc.
lgtm
LGTM w/ nits. http://codereview.chromium.org/11009013/diff/25004/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/25004/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:43: double getMaxTabTimestamp(const SessionWindow* window) { nit: Mixed case not camel-case on functions: GetMaxTabTimestamp(...) http://codereview.chromium.org/11009013/diff/25004/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:150: OPEN_ALL : TAB; nit: indent
Thanks for the review - very educational. Pedro - did you want to take a look before I send this out to dbeam? http://codereview.chromium.org/11009013/diff/25004/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/25004/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:43: double getMaxTabTimestamp(const SessionWindow* window) { On 2012/10/09 20:04:36, dhollowa wrote: > nit: Mixed case not camel-case on functions: GetMaxTabTimestamp(...) Done. http://codereview.chromium.org/11009013/diff/25004/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:150: OPEN_ALL : TAB; On 2012/10/09 20:04:36, dhollowa wrote: > nit: indent Done.
On 2012/10/09 20:31:05, jeremycho wrote: > Thanks for the review - very educational. > > Pedro - did you want to take a look before I send this out to dbeam? > > http://codereview.chromium.org/11009013/diff/25004/chrome/browser/ui/search/o... > File chrome/browser/ui/search/other_device_menu.cc (right): > > http://codereview.chromium.org/11009013/diff/25004/chrome/browser/ui/search/o... > chrome/browser/ui/search/other_device_menu.cc:43: double > getMaxTabTimestamp(const SessionWindow* window) { > On 2012/10/09 20:04:36, dhollowa wrote: > > nit: Mixed case not camel-case on functions: GetMaxTabTimestamp(...) > > Done. > > http://codereview.chromium.org/11009013/diff/25004/chrome/browser/ui/search/o... > chrome/browser/ui/search/other_device_menu.cc:150: OPEN_ALL : TAB; > On 2012/10/09 20:04:36, dhollowa wrote: > > nit: indent > > Done. I was following David's and Vadim's comments, but I'll take another look now just to make sure we haven't left anything behind.
lgtm LGTM with nits. http://codereview.chromium.org/11009013/diff/21006/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/21006/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:43: double GetMaxTabTimestamp(const SessionWindow* window) { nit: Max timestamp is not something intuitive IMO. What about GetWindowMostRecentTab? http://codereview.chromium.org/11009013/diff/21006/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:45: for (size_t i = 0; i < window->tabs.size(); ++i) { nit: Store |window->tabs.size()| in a variable to avoid looking up for this value multiple times. http://codereview.chromium.org/11009013/diff/21006/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:177: for (size_t i = 0; i < window->tabs.size(); ++i) { nit: Store |window->tabs.size()| in a variable?
http://codereview.chromium.org/11009013/diff/21006/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/21006/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:43: double GetMaxTabTimestamp(const SessionWindow* window) { On 2012/10/09 21:19:02, pedrosimonetti wrote: > nit: Max timestamp is not something intuitive IMO. What about > GetWindowMostRecentTab? It's returning a timestamp though, not a tab. But recent sounds better - changed to GetMostRecentTabTimestamp. http://codereview.chromium.org/11009013/diff/21006/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:45: for (size_t i = 0; i < window->tabs.size(); ++i) { On 2012/10/09 21:19:02, pedrosimonetti wrote: > nit: Store |window->tabs.size()| in a variable to avoid looking up for this > value multiple times. Done. http://codereview.chromium.org/11009013/diff/21006/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:177: for (size_t i = 0; i < window->tabs.size(); ++i) { On 2012/10/09 21:19:02, pedrosimonetti wrote: > nit: Store |window->tabs.size()| in a variable? Done.
Hi Dan - this is ready for your review. Thanks!
Hi Dan - this is ready for your review (sorry I previously sent it to your other account). Thanks!
got part way done, flushing http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:28: static const int kMaxWidth = 375; nit: can you move these static members into the anonymous namespace and remove static? http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:71: web_ui_(web_ui), session_id_(session_id), location_(location), nit: I often see initializer lists in chrome as web_ui_(web_ui), session_id_(session_id), location_(location), ALLOW_THIS_IN_INITIALIZER_LIST(menu_model_(this)) { with a \n after each , don't know if it's a hard and fast rule or anything, but I think it makes the code prettier http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:76: if (tab_data_.size() > 1) { nit: if (!tab_data.empty()) http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:98: content::WebContents* web_contents = web_ui_->GetWebContents(); is web_contents guaranteed to be non-NULL? http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:113: views::MenuRunner::MENU_DELETED) nit: {} when either the conditional or conditional body is over 1 line http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:138: tab_data->GetInteger("sessionId", &tab_id); should you be checking that this worked? http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:141: tab_data->GetInteger("windowId", &window_id); and this? http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:12: #include "ui/gfx/point.h" nit: can you not forward this class because it's instantiated as location_? http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:37: // ui::SimpleMenuModel::Delegate overrides: nit: make these private until somebody needs to inherit from this class http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:51: nit: I think you could describe some of these a little more
Thanks for the comments! http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:28: static const int kMaxWidth = 375; On 2012/10/12 01:10:57, Dan Beam wrote: > nit: can you move these static members into the anonymous namespace and remove > static? Done. http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:71: web_ui_(web_ui), session_id_(session_id), location_(location), On 2012/10/12 01:10:57, Dan Beam wrote: > nit: I often see initializer lists in chrome as > > web_ui_(web_ui), > session_id_(session_id), > location_(location), > ALLOW_THIS_IN_INITIALIZER_LIST(menu_model_(this)) { > > with a \n after each , > > don't know if it's a hard and fast rule or anything, but I think it makes the > code prettier Done. http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:76: if (tab_data_.size() > 1) { On 2012/10/12 01:10:57, Dan Beam wrote: > nit: if (!tab_data.empty()) Done. http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:98: content::WebContents* web_contents = web_ui_->GetWebContents(); On 2012/10/12 01:10:57, Dan Beam wrote: > is web_contents guaranteed to be non-NULL? I believe so. In the internals of web_ui_impl.cc, it assumes this. http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:113: views::MenuRunner::MENU_DELETED) On 2012/10/12 01:10:57, Dan Beam wrote: > nit: {} when either the conditional or conditional body is over 1 line Done. http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:138: tab_data->GetInteger("sessionId", &tab_id); On 2012/10/12 01:10:57, Dan Beam wrote: > should you be checking that this worked? In SessionTabToValue, this field is always set if the function returns true, so I think it's safe to assume (other_sessions.js doesn't check either). http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:141: tab_data->GetInteger("windowId", &window_id); On 2012/10/12 01:10:57, Dan Beam wrote: > and this? I think this is also safe (similar as above). http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.h (right): http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:12: #include "ui/gfx/point.h" On 2012/10/12 01:10:57, Dan Beam wrote: > nit: can you not forward this class because it's instantiated as location_? Correct. http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:37: // ui::SimpleMenuModel::Delegate overrides: On 2012/10/12 01:10:57, Dan Beam wrote: > nit: make these private until somebody needs to inherit from this class Done. http://codereview.chromium.org/11009013/diff/35002/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.h:51: On 2012/10/12 01:10:57, Dan Beam wrote: > nit: I think you could describe some of these a little more Done.
friendly ping.
does this compile on mac or linux? https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:77: LOG(ERROR) << "Failed to load foreign tab."; aren't we supposed to use VLOG(1) rather than LOG(ERROR) https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:95: windows.begin() + ((window_num == kInvalidId) ? 0 : window_num); remove excess parens https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:97: ((window_num == kInvalidId) ? here you have 2 sets of excess parens
will be able to review tomorrow morning, but estade is also an owner :D
lg to me, but wait for Evan, he has a lot more native UI experience https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... File chrome/browser/ui/search/other_device_menu.cc (right): https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/search/other_device_menu.cc:66: } // namespace nit: 2 \s before ending namespace comments https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/search/other_device_menu.cc:101: Browser* browser = browser::FindBrowserWithWebContents(web_contents); nit: s/ = / = / https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/search/other_device_menu.cc:184: *window->tabs[i], tab_value.get())) { nit: prefer if (!browser_sync::ForeignSessionHandler::SessionTabToValue( *window->tabs[i], tab_value.get())) { continue; } https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/search/other_device_menu.cc:193: // request favicons. http://crbug.com/153410. nit: 1 \s between sentences, you can probably make this longer (I don't think you're taking up 80 cols) https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:80: if (tab->navigations.size() == 0) { nit: .empty() https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:85: web_ui->GetWebContents(), *tab, disposition); nit: return and no else, IMO https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:110: return false; nit: \n IMO https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:112: 0, nit: you don't really need the std::max() if you've verified that navigations.size() > 0 (with the .empty()) and assuming tab.current_navigation_index can't be < 0 https://chromiumcodereview.appspot.com/11009013/diff/43003/chrome/browser/ui/... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:119: return false; nit: \n IMO
Hi Evan, It's not compiling on Linux. When linking other_devices_menu.cc, I'm getting error undefined reference to views::Widget, views::MenuModelAdapter, views::MenuRunner. I believe this is because other_devices_menu.cc is in browser/ui/search which gets included on all platforms, while ui/views is only included on ChromeOS? Originally I had it in ui/views, but was getting presubmit errors due to the dependency from foreign_session_handler. It looks like it'll have to be decoupled from foreign_session_handler. Do you have any suggestions? It's fine for now if this works for ChromeOS only. Thanks. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/search/o... File chrome/browser/ui/search/other_device_menu.cc (right): http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:66: } // namespace On 2012/10/19 04:18:47, Dan Beam wrote: > nit: 2 \s before ending namespace comments Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:101: Browser* browser = browser::FindBrowserWithWebContents(web_contents); On 2012/10/19 04:18:47, Dan Beam wrote: > nit: s/ = / = / Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:184: *window->tabs[i], tab_value.get())) { On 2012/10/19 04:18:47, Dan Beam wrote: > nit: prefer > > if (!browser_sync::ForeignSessionHandler::SessionTabToValue( > *window->tabs[i], tab_value.get())) { > continue; > } Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/search/o... chrome/browser/ui/search/other_device_menu.cc:193: // request favicons. http://crbug.com/153410. On 2012/10/19 04:18:47, Dan Beam wrote: > nit: 1 \s between sentences, you can probably make this longer (I don't think > you're taking up 80 cols) Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:77: LOG(ERROR) << "Failed to load foreign tab."; On 2012/10/17 21:41:40, Evan Stade wrote: > aren't we supposed to use VLOG(1) rather than LOG(ERROR) Not sure, it was here already and I see it throughout this file. Happy to switch though. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:80: if (tab->navigations.size() == 0) { On 2012/10/19 04:18:47, Dan Beam wrote: > nit: .empty() Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:85: web_ui->GetWebContents(), *tab, disposition); On 2012/10/19 04:18:47, Dan Beam wrote: > nit: return and no else, IMO Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:95: windows.begin() + ((window_num == kInvalidId) ? 0 : window_num); On 2012/10/17 21:41:40, Evan Stade wrote: > remove excess parens Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:97: ((window_num == kInvalidId) ? On 2012/10/17 21:41:40, Evan Stade wrote: > here you have 2 sets of excess parens Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:110: return false; On 2012/10/19 04:18:47, Dan Beam wrote: > nit: \n IMO Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:112: 0, On 2012/10/19 04:18:47, Dan Beam wrote: > nit: you don't really need the std::max() if you've verified that > navigations.size() > 0 (with the .empty()) and assuming > tab.current_navigation_index can't be < 0 Done. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:119: return false; On 2012/10/19 04:18:47, Dan Beam wrote: > nit: \n IMO Done.
Views is the UI toolkit for Windows and ChromeOS. Mac uses Cocoa and Linux uses Gtk. You'll need to de-couple the Views code (i.e. ShowMenu) from the model code. Make a controller (cross-platform) which instantiates the model (also cross-platform) and drives a view (platform-specific) which is stubbed out on mac and linux. Instantiate the view with a factory function which is implemented with a different one-liner on each platform. You can see this pattern in a lot of places, for example MediaGalleriesDialog::Create. http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:77: LOG(ERROR) << "Failed to load foreign tab."; On 2012/10/20 23:38:15, jeremycho wrote: > On 2012/10/17 21:41:40, Evan Stade wrote: > > aren't we supposed to use VLOG(1) rather than LOG(ERROR) > > Not sure, it was here already and I see it throughout this file. Happy to > switch though. nevermind. VLOG(1) replaces LOG(INFO) not LOG(ERROR)
Thanks for the helpful advice. I've done the decoupling and verified it compiles on Linux. If this approach looks reasonable, I'll add the Mac view files (once I figure out how to compile for it). On 2012/10/22 19:06:41, Evan Stade wrote: > Views is the UI toolkit for Windows and ChromeOS. Mac uses Cocoa and Linux uses > Gtk. You'll need to de-couple the Views code (i.e. ShowMenu) from the model > code. Make a controller (cross-platform) which instantiates the model (also > cross-platform) and drives a view (platform-specific) which is stubbed out on > mac and linux. > > Instantiate the view with a factory function which is implemented with a > different one-liner on each platform. You can see this pattern in a lot of > places, for example MediaGalleriesDialog::Create. > > http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... > File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): > > http://codereview.chromium.org/11009013/diff/43003/chrome/browser/ui/webui/nt... > chrome/browser/ui/webui/ntp/foreign_session_handler.cc:77: LOG(ERROR) << "Failed > to load foreign tab."; > On 2012/10/20 23:38:15, jeremycho wrote: > > On 2012/10/17 21:41:40, Evan Stade wrote: > > > aren't we supposed to use VLOG(1) rather than LOG(ERROR) > > > > Not sure, it was here already and I see it throughout this file. Happy to > > switch though. > > nevermind. VLOG(1) replaces LOG(INFO) not LOG(ERROR)
looking better. Doesn't the view need to have some way of informing the controller when the menu has been dismissed (in the case where the user presses escape or clicks away)?
http://codereview.chromium.org/11009013/diff/58010/chrome/browser/ui/views/ot... File chrome/browser/ui/views/other_device_menu_view.h (right): http://codereview.chromium.org/11009013/diff/58010/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_view.h:23: class OtherDeviceMenuView : public OtherDeviceMenu { nit: I would call this OtherDeviceMenuViews
Menu runner handles the menu dismissal internally, so no additional work should be required by the controller (the view in this case is basically a wrapper of menu runner). I stubbed out the menu population code and verified that it gets shown/dismissed correctly. http://codereview.chromium.org/11009013/diff/58010/chrome/browser/ui/views/ot... File chrome/browser/ui/views/other_device_menu_view.h (right): http://codereview.chromium.org/11009013/diff/58010/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_view.h:23: class OtherDeviceMenuView : public OtherDeviceMenu { On 2012/10/24 00:49:07, Evan Stade wrote: > nit: I would call this OtherDeviceMenuViews Done.
I'm happy with this approach. You can proceed to make mac compile. http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/gtk/othe... File chrome/browser/ui/gtk/other_device_menu_views_gtk.cc (right): http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/gtk/othe... chrome/browser/ui/gtk/other_device_menu_views_gtk.cc:15: void OtherDeviceMenuViews::ShowMenu(gfx::NativeWindow window, this class should be named OtherDeviceMenuGtk "Views" is a framework, "View" refers to MVC. So while this is a view, the class name doesn't need to reflect that (you could make it OtherDeviceMenuViewGtk but I'm of the opinion that's more verbose than need be, and we don't usually do it). http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/gtk/othe... chrome/browser/ui/gtk/other_device_menu_views_gtk.cc:16: const gfx::Point& location) { wrong indent http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:362: menu->ShowMenu(); wrong indent
Added cocoa files. http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/gtk/othe... File chrome/browser/ui/gtk/other_device_menu_views_gtk.cc (right): http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/gtk/othe... chrome/browser/ui/gtk/other_device_menu_views_gtk.cc:15: void OtherDeviceMenuViews::ShowMenu(gfx::NativeWindow window, On 2012/10/24 22:51:34, Evan Stade wrote: > this class should be named OtherDeviceMenuGtk > > "Views" is a framework, "View" refers to MVC. So while this is a view, the class > name doesn't need to reflect that (you could make it OtherDeviceMenuViewGtk but > I'm of the opinion that's more verbose than need be, and we don't usually do > it). Done. http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/gtk/othe... chrome/browser/ui/gtk/other_device_menu_views_gtk.cc:16: const gfx::Point& location) { On 2012/10/24 22:51:34, Evan Stade wrote: > wrong indent Done. http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/foreign_session_handler.cc (right): http://codereview.chromium.org/11009013/diff/69001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/foreign_session_handler.cc:362: menu->ShowMenu(); On 2012/10/24 22:51:34, Evan Stade wrote: > wrong indent Done.
lgtm, you need some OWNERS reviews though.
Thanks for the reviews. Scott - would you be able to review for ui/views? Thanks, Jeremy
views LGTM http://codereview.chromium.org/11009013/diff/75001/chrome/browser/ui/views/ot... File chrome/browser/ui/views/other_device_menu_views.h (right): http://codereview.chromium.org/11009013/diff/75001/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_views.h:30: nit: remove newline.
http://codereview.chromium.org/11009013/diff/75001/chrome/browser/ui/views/ot... File chrome/browser/ui/views/other_device_menu_views.h (right): http://codereview.chromium.org/11009013/diff/75001/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_views.h:30: On 2012/10/26 19:34:11, sky wrote: > nit: remove newline. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/11009013/83001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/11009013/90002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/11009013/89003
Change committed as 164578
somehow I wasn't copied into this change :( but per WATCHLIST I should have, did you change the CC list? https://chromiumcodereview.appspot.com/11009013/diff/89003/chrome/browser/ui/... File chrome/browser/ui/views/other_device_menu_views.cc (right): https://chromiumcodereview.appspot.com/11009013/diff/89003/chrome/browser/ui/... chrome/browser/ui/views/other_device_menu_views.cc:20: const gfx::Point& location) { nit: indentation is off here! https://chromiumcodereview.appspot.com/11009013/diff/89003/chrome/browser/ui/... chrome/browser/ui/views/other_device_menu_views.cc:35: // OtherDeviceMenuController --------------------------------------------------- nit: s/OtherDeviceMenuController/OtherDeviceMenu To be honest, I wouldn't have this useless comment at all, and I'd put this at the top of this file right after the includes, not a big deal though. https://chromiumcodereview.appspot.com/11009013/diff/89003/chrome/browser/ui/... File chrome/browser/ui/views/other_device_menu_views.h (right): https://chromiumcodereview.appspot.com/11009013/diff/89003/chrome/browser/ui/... chrome/browser/ui/views/other_device_menu_views.h:8: #include "base/memory/scoped_ptr.h" include basictypes for DISALLOW_ and compiler_specific for OVERRIDE! https://chromiumcodereview.appspot.com/11009013/diff/89003/chrome/browser/ui/... chrome/browser/ui/views/other_device_menu_views.h:11: namespace gfx{ nit: one whitespace between gfx and { https://chromiumcodereview.appspot.com/11009013/diff/89003/chrome/browser/ui/... chrome/browser/ui/views/other_device_menu_views.h:29: gfx::NativeWindow window, const gfx::Point& location) OVERRIDE; nit: one arg per line! https://chromiumcodereview.appspot.com/11009013/diff/89003/chrome/browser/ui/... chrome/browser/ui/views/other_device_menu_views.h:32: ui::SimpleMenuModel* menu_model_; // Owned by the controller. which controller? better to write the class name.
Hi tfarina, Not sure why you weren't copied - I don't believe I modified cc. I've responded to your comments. http://codereview.chromium.org/11009013/diff/89003/chrome/browser/ui/views/ot... File chrome/browser/ui/views/other_device_menu_views.cc (right): http://codereview.chromium.org/11009013/diff/89003/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_views.cc:20: const gfx::Point& location) { On 2012/10/29 01:37:43, tfarina wrote: > nit: indentation is off here! Done. http://codereview.chromium.org/11009013/diff/89003/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_views.cc:35: // OtherDeviceMenuController --------------------------------------------------- On 2012/10/29 01:37:43, tfarina wrote: > nit: s/OtherDeviceMenuController/OtherDeviceMenu > > To be honest, I wouldn't have this useless comment at all, and I'd put this at > the top of this file right after the includes, not a big deal though. Done. http://codereview.chromium.org/11009013/diff/89003/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_views.cc:35: // OtherDeviceMenuController --------------------------------------------------- On 2012/10/29 01:37:43, tfarina wrote: > nit: s/OtherDeviceMenuController/OtherDeviceMenu > > To be honest, I wouldn't have this useless comment at all, and I'd put this at > the top of this file right after the includes, not a big deal though. Done. http://codereview.chromium.org/11009013/diff/89003/chrome/browser/ui/views/ot... File chrome/browser/ui/views/other_device_menu_views.h (right): http://codereview.chromium.org/11009013/diff/89003/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_views.h:11: namespace gfx{ On 2012/10/29 01:37:43, tfarina wrote: > nit: one whitespace between gfx and { Done. http://codereview.chromium.org/11009013/diff/89003/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_views.h:29: gfx::NativeWindow window, const gfx::Point& location) OVERRIDE; On 2012/10/29 01:37:43, tfarina wrote: > nit: one arg per line! Done. http://codereview.chromium.org/11009013/diff/89003/chrome/browser/ui/views/ot... chrome/browser/ui/views/other_device_menu_views.h:32: ui::SimpleMenuModel* menu_model_; // Owned by the controller. On 2012/10/29 01:37:43, tfarina wrote: > which controller? better to write the class name. Done. |
