Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6692001:
Add in DOMBrowserView and Frame related classes (Closed)
|
Created:
9 years, 9 months ago by rharrison Modified:
9 years, 6 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, whywhat Visibility:
Public. |
DescriptionAdd in DOMBrowserView and Frame related classes
This is the final CL for a sequence of CLs that will add in the display infrastructure that I have built for the DOM Login and touch keyboard. I have created a reference CL for what the end product of this sequence is intended to be, so that I can give context for the CL under review. This can be found at: http://codereview.chromium.org/6577003/
This CL adds in DOMBrowserView, DOMBrowserFrame, and related classes to touchui==1 and chromeos==1 builds. DOMBrowserView classes inherit from the equivalent chromeos::BrowserView classes. DOMBrowserFrame classes inherit from TouchBrowserFrame classes, since it has the keyboards implemented in it. In addition to the direct parent I have had to make changes to other classes in the hiearchy as needed to ease implementation. Additional checks and refatoring have been done to get the build to work and minize the size of this CL. I have actively chosen to not refactor chromeos::BrowserViewLoayout out of the file it current resides in since DOMBrowserViewLayout wouldn't inherit much from it if I subclassed it.
Patch from Ryan Harrison <rharrison@chromium.org>;
BUG=none
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79018
Patch Set 1 #
Total comments: 42
Patch Set 2 : Updated CL in response to comments by oshima and rjkroege #
Total comments: 70
Patch Set 3 : Updated CL in response to comments by oshima and nkostylev #
Total comments: 15
Patch Set 4 : Removed DOMBrowserFrame* classes and added changes in line with oshima's comments #Messages
Total messages: 17 (0 generated)
looks reasonable to me. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/browser/dom_browser_view.h (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:23: // BrowserView overrides You should have the OVERRIDE macro?
http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/browser/dom_browser.cc (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser.cc:12: // DOMBrowser: public I thought the new format is // text ------------- no? http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/browser/dom_browser_view.h (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:13: class LayoutManager; do you need this? I assume this is already done in browser_view.h http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:23: // BrowserView overrides On 2011/03/14 17:59:00, rjkroege wrote: > You should have the OVERRIDE macro? Good idea. I'd say new code should have it. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:25: // DOMBrowserViewLayout public: could you please add brief description of how components are laid out? http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:27: remove extra new line http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:37: views::View* view) { indent http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:46: // In the normal and the compact navigation bar mode, ChromeOS Compact nav bar is gone long time ago. I guess it's cut© error. Can you update the comment? http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:77: bv_bounds.set_height(bv_bounds.height()); looks like no-op? http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:83: return HTNOWHERE; what is this condition for? http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:108: if (bounds.IsEmpty()) Do we want to support virtual tabs in touch? You can keep this if you want, but you may want to disable and delete if there is no plan to support this. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:35: DOMBrowserView* dom_browser_view(); GetDOMBrowserView() http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. 2011 and for the rest. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc:26: new DOMBrowserFrame(browser_view, profile); may fit to single line? http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h:14: class DOMBrowserFrame : public BrowserFrameChromeos { please add comment http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h:22: protected: // BrowserFrameChromeos overrides: (or whatever the format you like) http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:12: class BrowserView; Aren't these defined in touch_browser_frame_view? http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:23: CreateBrowserNonClientFrameView(BrowserFrame* frame, indent http://codereview.chromium.org/6692001/diff/1/chrome/browser/ui/views/frame/b... File chrome/browser/ui/views/frame/browser_view.cc (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:518: if (toolbar_->location_bar()) if (toolbar_ && toolbar_->location_bar()) http://codereview.chromium.org/6692001/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:1667: if (!tabstrip_->IsTabStripCloseable()) if (tabstrip_ && ...) http://codereview.chromium.org/6692001/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:2531: return; Can this happen? If not, use CHECK/DCHECK. If yes, who deletes the toolbar?
http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/browser/dom_browser.cc (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser.cc:12: // DOMBrowser: public On 2011/03/14 20:09:22, oshima wrote: > I thought the new format is > > // text ------------- > > no? Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/browser/dom_browser_view.h (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:13: class LayoutManager; On 2011/03/14 20:09:22, oshima wrote: > do you need this? I assume this is already done in browser_view.h Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:23: // BrowserView overrides On 2011/03/14 17:59:00, rjkroege wrote: > You should have the OVERRIDE macro? Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:23: // BrowserView overrides On 2011/03/14 20:09:22, oshima wrote: > On 2011/03/14 17:59:00, rjkroege wrote: > > You should have the OVERRIDE macro? > > Good idea. I'd say new code should have it. Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:25: // DOMBrowserViewLayout public: On 2011/03/14 20:09:22, oshima wrote: > could you please add brief description of how components are laid out? Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:27: On 2011/03/14 20:09:22, oshima wrote: > remove extra new line Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:37: views::View* view) { On 2011/03/14 20:09:22, oshima wrote: > indent Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:46: // In the normal and the compact navigation bar mode, ChromeOS On 2011/03/14 20:09:22, oshima wrote: > Compact nav bar is gone long time ago. I guess it's cut© error. Can you > update the comment? Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:77: bv_bounds.set_height(bv_bounds.height()); On 2011/03/14 20:09:22, oshima wrote: > looks like no-op? Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:83: return HTNOWHERE; On 2011/03/14 20:09:22, oshima wrote: > what is this condition for? Not really sure, I think it was an artifact from basing my code off of chromeos::BrowserViewLayout http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:108: if (bounds.IsEmpty()) On 2011/03/14 20:09:22, oshima wrote: > Do we want to support virtual tabs in touch? You can keep this if you want, but > you may want to disable and delete if there is no plan to support this. We are not supporting vertical tabs, I should have removed this code when I was modifying it for touch. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:35: DOMBrowserView* dom_browser_view(); On 2011/03/14 20:09:22, oshima wrote: > GetDOMBrowserView() Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2011/03/14 20:09:22, oshima wrote: > 2011 and for the rest. Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc:26: new DOMBrowserFrame(browser_view, profile); On 2011/03/14 20:09:22, oshima wrote: > may fit to single line? Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h:14: class DOMBrowserFrame : public BrowserFrameChromeos { On 2011/03/14 20:09:22, oshima wrote: > please add comment Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h:22: protected: On 2011/03/14 20:09:22, oshima wrote: > // BrowserFrameChromeos overrides: > (or whatever the format you like) Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:12: class BrowserView; On 2011/03/14 20:09:22, oshima wrote: > Aren't these defined in touch_browser_frame_view? Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/chromeos/webui/l... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:23: CreateBrowserNonClientFrameView(BrowserFrame* frame, On 2011/03/14 20:09:22, oshima wrote: > indent Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/ui/views/frame/b... File chrome/browser/ui/views/frame/browser_view.cc (right): http://codereview.chromium.org/6692001/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:518: if (toolbar_->location_bar()) On 2011/03/14 20:09:22, oshima wrote: > if (toolbar_ && toolbar_->location_bar()) Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:1667: if (!tabstrip_->IsTabStripCloseable()) On 2011/03/14 20:09:22, oshima wrote: > if (tabstrip_ && ...) Done. http://codereview.chromium.org/6692001/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:2531: return; On 2011/03/14 20:09:22, oshima wrote: > Can this happen? If not, use CHECK/DCHECK. If yes, who deletes the toolbar? Yes, since we override the function CreateToolbar that is used to generate the value for this function, this method should be changed to be more robhust.
http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:27: #include "ui/views/layout/layout_manager.h" I believe some of these includes are not necessary (like browser_dialog, status_bubble_view, toolbar_view). Can you check and eliminate them? http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:31: // DOMBrowserView, public // DOMBrowserView, public ------------------ and for the rest (if any) http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:49: return gfx::Rect(0, 0, 0, 0); just gfx::Rect() http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:62: nuke extra line http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:110: return true; just return status_area_->HitTest(point_in_status_area_coords); http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:11: #include "chrome/browser/chromeos/status/status_area_host.h" These two includes can be replaced with forward decls? http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc:11: remove extra line http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h:12: namespace chromeos { new line after namspace http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h:29: virtual BrowserNonClientFrameView* CreateBrowserNonClientFrameView(); OVERRIDE http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc:13: #include "views/window/window.h" Can you eliminate includes that are not necessary? http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc:37: return gfx::Rect(0, 0, 0, 0); just gfx::Rect() (note to myself too) http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:22: // TouchBrowserFrameView overrides. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:23: virtual gfx::Rect GetBoundsForTabStrip(views::View* tabstrip) const; OVERRIDE http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:26: virtual void LayoutOTRAvatar(); OVERRIDE http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:30: }; new line between class and namespace http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view.cc (left): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.cc:1824: // BrowserView, private: Can you put this back to appropriate place in new format? http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/opaque_browser_frame_view.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/opaque_browser_frame_view.h:79: virtual void LayoutOTRAvatar(); OVERRIDE http://codereview.chromium.org/6692001/diff/6001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/6692001/diff/6001/chrome/chrome_browser.gypi#n... chrome/chrome_browser.gypi:718: 'browser/chromeos/webui/login/frame/dom_browser_frame_view.h', sorry for missing this in previous review. webui should be for the content code, not a frame/window of webui. Can you move them to browser/chromeos/frame? It's consistent with chrome/browser/ui/views/frame.
http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:40: // Create a browser view for chromeos. Move this comment to .h file and make more specific. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:107: // BrowserView, protected Please follow suggested comment style // ... -------- http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:107: // BrowserView, protected BrowserView > DOMBrowserView http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:16: class DOMBrowserView : public chromeos::BrowserView { Please add comment for this class. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:16: // Amount to offset the toolbar by when vertical tabs are enabled. Do you need this considering that DOMBrowserView lacks tabs? http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:24: // LayoutManager for DOMBrowserView, which lays out the StatusAreaView in the Please move comment to header. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:78: return top; nit: fix indent. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:96: // If the point is somewhere else, delegate to the default implementation. nit: fix indent. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:102: // Tests if the point is on one of views that are within the Move comment to header. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:115: // Lays out tabstrip and status area in the title bar area (given by Move comment to header. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:120: status_area_->SetBounds(bounds.right() - status_size.width(), bounds.y(), nit: 1 param per line please. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:17: // the status views nit: dot at the end. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:20: DOMBrowserViewLayout() : ::BrowserViewLayout() {} Move ctor body into .cc file. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:21: virtual ~DOMBrowserViewLayout() {} Move body to .cc file. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc:17: // DOMBrowserFrameView, public: // ... ------ http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc:33: // DOMBrowserFrameView, protected: // ... ------
http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:27: #include "ui/views/layout/layout_manager.h" On 2011/03/16 21:10:25, oshima wrote: > I believe some of these includes are not necessary (like browser_dialog, > status_bubble_view, toolbar_view). Can you check and eliminate them? Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:31: // DOMBrowserView, public On 2011/03/16 21:10:25, oshima wrote: > // DOMBrowserView, public ------------------ > > and for the rest (if any) Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:40: // Create a browser view for chromeos. On 2011/03/16 21:33:45, Nikita Kostylev wrote: > Move this comment to .h file and make more specific. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:49: return gfx::Rect(0, 0, 0, 0); On 2011/03/16 21:10:25, oshima wrote: > just gfx::Rect() Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:107: // BrowserView, protected On 2011/03/16 21:33:45, Nikita Kostylev wrote: > Please follow suggested comment style > // ... -------- Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.cc:107: // BrowserView, protected On 2011/03/16 21:33:45, Nikita Kostylev wrote: > BrowserView > DOMBrowserView Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:16: class DOMBrowserView : public chromeos::BrowserView { On 2011/03/16 21:33:45, Nikita Kostylev wrote: > Please add comment for this class. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view.h:62: On 2011/03/16 21:10:25, oshima wrote: > nuke extra line Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:16: // Amount to offset the toolbar by when vertical tabs are enabled. On 2011/03/16 21:33:45, Nikita Kostylev wrote: > Do you need this considering that DOMBrowserView lacks tabs? No, forgot to clean it up. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:24: // LayoutManager for DOMBrowserView, which lays out the StatusAreaView in the On 2011/03/16 21:33:45, Nikita Kostylev wrote: > Please move comment to header. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:78: return top; On 2011/03/16 21:33:45, Nikita Kostylev wrote: > nit: fix indent. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:96: // If the point is somewhere else, delegate to the default implementation. On 2011/03/16 21:33:45, Nikita Kostylev wrote: > nit: fix indent. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:102: // Tests if the point is on one of views that are within the On 2011/03/16 21:33:45, Nikita Kostylev wrote: > Move comment to header. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:110: return true; On 2011/03/16 21:10:25, oshima wrote: > just > > return status_area_->HitTest(point_in_status_area_coords); Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:115: // Lays out tabstrip and status area in the title bar area (given by On 2011/03/16 21:33:45, Nikita Kostylev wrote: > Move comment to header. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:120: status_area_->SetBounds(bounds.right() - status_size.width(), bounds.y(), On 2011/03/16 21:33:45, Nikita Kostylev wrote: > nit: 1 param per line please. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:11: #include "chrome/browser/chromeos/status/status_area_host.h" On 2011/03/16 21:10:25, oshima wrote: > These two includes can be replaced with forward decls? Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:17: // the status views On 2011/03/16 21:33:45, Nikita Kostylev wrote: > nit: dot at the end. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:20: DOMBrowserViewLayout() : ::BrowserViewLayout() {} On 2011/03/16 21:33:45, Nikita Kostylev wrote: > Move ctor body into .cc file. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:21: virtual ~DOMBrowserViewLayout() {} On 2011/03/16 21:33:45, Nikita Kostylev wrote: > Move body to .cc file. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc:11: On 2011/03/16 21:10:25, oshima wrote: > remove extra line Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h:12: namespace chromeos { On 2011/03/16 21:10:25, oshima wrote: > new line after namspace Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h:29: virtual BrowserNonClientFrameView* CreateBrowserNonClientFrameView(); On 2011/03/16 21:10:25, oshima wrote: > OVERRIDE Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc:13: #include "views/window/window.h" On 2011/03/16 21:10:25, oshima wrote: > Can you eliminate includes that are not necessary? Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc:17: // DOMBrowserFrameView, public: On 2011/03/16 21:33:45, Nikita Kostylev wrote: > // ... ------ Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc:33: // DOMBrowserFrameView, protected: On 2011/03/16 21:33:45, Nikita Kostylev wrote: > // ... ------ Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.cc:37: return gfx::Rect(0, 0, 0, 0); On 2011/03/16 21:10:25, oshima wrote: > just > > gfx::Rect() > > (note to myself too) Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:22: On 2011/03/16 21:10:25, oshima wrote: > // TouchBrowserFrameView overrides. Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:23: virtual gfx::Rect GetBoundsForTabStrip(views::View* tabstrip) const; On 2011/03/16 21:10:25, oshima wrote: > OVERRIDE Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:26: virtual void LayoutOTRAvatar(); On 2011/03/16 21:10:25, oshima wrote: > OVERRIDE Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame_view.h:30: }; On 2011/03/16 21:10:25, oshima wrote: > new line between class and namespace Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view.cc (left): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.cc:1824: // BrowserView, private: On 2011/03/16 21:10:25, oshima wrote: > Can you put this back to appropriate place in new format? Done. http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/opaque_browser_frame_view.h (right): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/opaque_browser_frame_view.h:79: virtual void LayoutOTRAvatar(); On 2011/03/16 21:10:25, oshima wrote: > OVERRIDE Done. http://codereview.chromium.org/6692001/diff/6001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/6692001/diff/6001/chrome/chrome_browser.gypi#n... chrome/chrome_browser.gypi:718: 'browser/chromeos/webui/login/frame/dom_browser_frame_view.h', On 2011/03/16 21:10:25, oshima wrote: > sorry for missing this in previous review. > > webui should be for the content code, not a frame/window of webui. > Can you move them to > browser/chromeos/frame? It's consistent with chrome/browser/ui/views/frame. Since this would involve moving a lot of code around I would prefer to leave it where it is for this CL and then follow this CL up with a cleanup CL that covers moving all of the code. Is this acceptable to you?
http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view.cc (left): http://codereview.chromium.org/6692001/diff/6001/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.cc:1824: // BrowserView, private: On 2011/03/17 17:00:50, rharrison wrote: > On 2011/03/16 21:10:25, oshima wrote: > > Can you put this back to appropriate place in new format? > Where it is now? I couldn't find it in new patch. > Done. http://codereview.chromium.org/6692001/diff/6001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/6692001/diff/6001/chrome/chrome_browser.gypi#n... chrome/chrome_browser.gypi:718: 'browser/chromeos/webui/login/frame/dom_browser_frame_view.h', On 2011/03/17 17:00:50, rharrison wrote: > On 2011/03/16 21:10:25, oshima wrote: > > sorry for missing this in previous review. > > > > webui should be for the content code, not a frame/window of webui. > > Can you move them to > > browser/chromeos/frame? It's consistent with chrome/browser/ui/views/frame. > > Since this would involve moving a lot of code around I would prefer to leave it > where it is for this CL and then follow this CL up with a cleanup CL that covers > moving all of the code. Is this acceptable to you? That's fine. http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc (right): http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:11: #include "chrome/browser/ui/views/toolbar_view.h" do you need this? http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:14: remove extra line http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:53: } new line in between http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h (right): http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:61: DOMBrowserView* GetDOMBrowserView(); const http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc (right): http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc:15: : BrowserFrameChromeos(browser_view, profile) { } {} (though i'm not sure if this is in styleguide) http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.cc:17: DOMBrowserFrame::~DOMBrowserFrame() { } same here http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h (right): http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/frame/dom_browser_frame.h:36: } // namespace chroemos chromeos http://codereview.chromium.org/6692001/diff/2006/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view.h (right): http://codereview.chromium.org/6692001/diff/2006/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.h:408: // override to implement different layout potentially. policy? http://codereview.chromium.org/6692001/diff/2006/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.h:416: // Factory Methods. Method.
I removed the DOMBrowserFrame* classes, since refactoring had caused most of their functionality to be moved to other classes. They were mostly boilerplate and code to hook themselves into the system. This also means that there isn't a difference between supporting touch and non-touch versions of the DOM screens, since the appropriate BrowserFrame is used by default instead of being explicitly chosen by inheritance. http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc (right): http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:11: #include "chrome/browser/ui/views/toolbar_view.h" On 2011/03/17 18:49:31, oshima wrote: > do you need this? Done. http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:14: On 2011/03/17 18:49:31, oshima wrote: > remove extra line Done. http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.cc:53: } On 2011/03/17 18:49:31, oshima wrote: > new line in between Done. http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... File chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h (right): http://codereview.chromium.org/6692001/diff/2006/chrome/browser/chromeos/webu... chrome/browser/chromeos/webui/login/browser/dom_browser_view_layout.h:61: DOMBrowserView* GetDOMBrowserView(); On 2011/03/17 18:49:31, oshima wrote: > const Done. http://codereview.chromium.org/6692001/diff/2006/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view.h (right): http://codereview.chromium.org/6692001/diff/2006/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.h:408: // override to implement different layout potentially. On 2011/03/17 18:49:31, oshima wrote: > policy? Done. http://codereview.chromium.org/6692001/diff/2006/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.h:416: // Factory Methods. On 2011/03/17 18:49:31, oshima wrote: > Method. Done.
LGTM
LGTM Thanks
>> This also means that there isn't a difference between supporting touch and non-touch versions of the DOM screens Thanks for using same base classes for touch/non-touch versions.
Don't forget to run trybots, I don't see their results for this CL.
LGTM
Try job failure for 6692001-20001 on mac: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
On 2011/03/22 18:33:53, commit-bot wrote: > Try job failure for 6692001-20001 on mac: > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... The tree went red due to Mac Clang from a commit that landed right before this try went in. I am kicking it again now that the the tree is green
Change committed as 79018 |
