Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6973029:
Integrate WebUI Login with cros. (Closed)
|
Created:
9 years, 7 months ago by rharrison Modified:
9 years, 6 months ago CC:
chromium-reviews, nkostylev+cc_chromium.org, Erik does not do reviews, Aaron Boodman, pam+watch_chromium.org, davemoore+watch_chromium.org, bryeung, Dmitry Polukhin Visibility:
Public. |
DescriptionIntegrate WebUI Login with cros.
This patch covers integrating the the WebUI based login with cros, so that when --dom-login is passed in on boot the WebUI login screen is used, keyboard comes up and the user can login.
I have refactored the WebUI login code to not use a subclass of the Browser class. I have removed the unneeded DOMBrowser* classes. This work was based off a combination of the BackgroundView and the TouchBrowserFrameView. If there is comments or variable names that betray this please let me know. I have endevoured to update the comments, but significant portions of code were pulled in.
Since both of the TouchBrowserFrameView and WebUILoginView have common code for having a a touch keyboard there is some refactoring to be done that will remove duplicated code between the two. This will be dealt with in a follow on CL.
The animation of the keyboard has not been included in this CL, mostly because I was having issues getting it to correctly size and this CL is pretty large already. This will be resolved in a follow on CL.
I am also converting from the use of DOM to WebUI at the moment, so some of the new code uses WebUI instead of DOM. I will be uploading a follow on CL that finishes this conversion.
This CL depends on http://gerrit.chromium.org/gerrit/706 and http://gerrit.chromium.org/gerrit/759 landing before it to work correctly.
It also depends on http://codereview.chromium.org/7014021/ to build.
Patch from Ryan Harrison <rharrison@chromium.org>;
BUG=none
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86659
Patch Set 1 #Patch Set 2 : Implemented login screen using widget #
Total comments: 62
Patch Set 3 : Updated code in response to review comments #
Total comments: 15
Patch Set 4 : Resolved more issues pointed out by reviewers #Patch Set 5 : Removed ResetXCursor and did_paint_and fixed segfault in WebUILoginView #
Total comments: 6
Patch Set 6 : Updated CL in response to sky's comments #Patch Set 7 : Fixed patch staleness #Messages
Total messages: 30 (0 generated)
When you list multiple reviewers please indicate what you expect each reviewer to look at. -Scott
On 2011/05/12 16:30:23, sky wrote: > When you list multiple reviewers please indicate what you expect each reviewer > to look at. > > -Scott Sorry about not doing that. sky: Can you look at the browser specific changes? They are just adding checks for the location bar that I saw cause seg faults in the login screen. bryeung: Can you look at the extension change to the touch browser frame view? nkostylev: Can you look at the login specific changes? oshima: Can you look at the views changes?
Why do we need to add these checks? In fact why are we subclassing Browser and BrowserView here? Wouldn't you be better served by a putting a TabContents in a window? Browser wasn't really designed for subclassing and I suspect patching all the places you need to insert custom logic is going to be tricky and error prone. -Scott
Indeed. It seems like the WebUI based login flow would be best served by creating a toplevel widget containing a DOMView. Also, can we rename all these classes to WebUIView, etc now that the underlying class has been renamed? -Ben On Thu, May 12, 2011 at 11:18 AM, <sky@chromium.org> wrote: > Why do we need to add these checks? > > In fact why are we subclassing Browser and BrowserView here? Wouldn't you > be > better served by a putting a TabContents in a window? > Browser wasn't really designed for subclassing and I suspect patching all > the > places you need to insert custom logic is going to be tricky and error > prone. > > -Scott > > > http://codereview.chromium.org/6973029/ >
On 2011/05/12 18:20:50, Ben Goodger wrote: > Indeed. It seems like the WebUI based login flow would be best served by > creating a toplevel widget containing a DOMView. > > Also, can we rename all these classes to WebUIView, etc now that the > underlying class has been renamed? > > -Ben > > On Thu, May 12, 2011 at 11:18 AM, <mailto:sky@chromium.org> wrote: > > > Why do we need to add these checks? > > > > In fact why are we subclassing Browser and BrowserView here? Wouldn't you > > be > > better served by a putting a TabContents in a window? > > Browser wasn't really designed for subclassing and I suspect patching all > > the > > places you need to insert custom logic is going to be tricky and error > > prone. > > > > -Scott > > > > > > http://codereview.chromium.org/6973029/ > > I have previously attempted to implement a top level widget with a DOMView, but the need to support an on-screen keyboard made this impossible from my perspective. I am going to be in MTV next week, Monday through Thursday morning. Can we schedule some time to meet and discuss this? If we can work out a simpler way to implement this without subclassing Browser and related classes I would be interested in going with it for a design. I have created a CL that deals with the renaming from DOM to WebUI (http://codereview.chromium.org/7015024). I don't think rolling the renaming into this CL would be a good idea, since it makes the diff very difficult to follow since so many files are moved.
What challenges did you run into? Can't you just embed the keyboard on the new window as well? I would like us to stop subclassing Browser and BrowserView. -Ben On Thu, May 12, 2011 at 11:59 AM, <rharrison@chromium.org> wrote: > On 2011/05/12 18:20:50, Ben Goodger wrote: > >> Indeed. It seems like the WebUI based login flow would be best served by >> creating a toplevel widget containing a DOMView. >> > > Also, can we rename all these classes to WebUIView, etc now that the >> underlying class has been renamed? >> > > -Ben >> > > On Thu, May 12, 2011 at 11:18 AM, <mailto:sky@chromium.org> wrote: >> > > > Why do we need to add these checks? >> > >> > In fact why are we subclassing Browser and BrowserView here? Wouldn't >> you >> > be >> > better served by a putting a TabContents in a window? >> > Browser wasn't really designed for subclassing and I suspect patching >> all >> > the >> > places you need to insert custom logic is going to be tricky and error >> > prone. >> > >> > -Scott >> > >> > >> > http://codereview.chromium.org/6973029/ >> > >> > > I have previously attempted to implement a top level widget with a DOMView, > but > the need to support an on-screen keyboard made this impossible from my > perspective. > > I am going to be in MTV next week, Monday through Thursday morning. Can we > schedule some time to meet and discuss this? If we can work out a simpler > way to > implement this without subclassing Browser and related classes I would be > interested in going with it for a design. > > I have created a CL that deals with the renaming from DOM to WebUI > (http://codereview.chromium.org/7015024). I don't think rolling the > renaming > into this CL would be a good idea, since it makes the diff very difficult > to > follow since so many files are moved. > > > > http://codereview.chromium.org/6973029/ >
The keyboard is implemented as an extension embedded in the touch browser frame and I am/was under the impression that the Browser is needed for extension support is the major issue. On 2011/05/12 19:41:49, Ben Goodger wrote: > What challenges did you run into? Can't you just embed the keyboard on the > new window as well? > > I would like us to stop subclassing Browser and BrowserView. > > -Ben > > On Thu, May 12, 2011 at 11:59 AM, <mailto:rharrison@chromium.org> wrote: > > > On 2011/05/12 18:20:50, Ben Goodger wrote: > > > >> Indeed. It seems like the WebUI based login flow would be best served by > >> creating a toplevel widget containing a DOMView. > >> > > > > Also, can we rename all these classes to WebUIView, etc now that the > >> underlying class has been renamed? > >> > > > > -Ben > >> > > > > On Thu, May 12, 2011 at 11:18 AM, <mailto:sky@chromium.org> wrote: > >> > > > > > Why do we need to add these checks? > >> > > >> > In fact why are we subclassing Browser and BrowserView here? Wouldn't > >> you > >> > be > >> > better served by a putting a TabContents in a window? > >> > Browser wasn't really designed for subclassing and I suspect patching > >> all > >> > the > >> > places you need to insert custom logic is going to be tricky and error > >> > prone. > >> > > >> > -Scott > >> > > >> > > >> > http://codereview.chromium.org/6973029/ > >> > > >> > > > > I have previously attempted to implement a top level widget with a DOMView, > > but > > the need to support an on-screen keyboard made this impossible from my > > perspective. > > > > I am going to be in MTV next week, Monday through Thursday morning. Can we > > schedule some time to meet and discuss this? If we can work out a simpler > > way to > > implement this without subclassing Browser and related classes I would be > > interested in going with it for a design. > > > > I have created a CL that deals with the renaming from DOM to WebUI > > (http://codereview.chromium.org/7015024). I don't think rolling the > > renaming > > into this CL would be a good idea, since it makes the diff very difficult > > to > > follow since so many files are moved. > > > > > > > > http://codereview.chromium.org/6973029/ > >
On 2011/05/12 19:53:38, rharrison wrote:
> The keyboard is implemented as an extension embedded in the touch browser
frame
> and I am/was under the impression that the Browser is needed for extension
> support is the major issue.
Looking at browser.h, I don't see anything there that's required for extensions
to work (and I would be surprised if aa/erikkay designed it this way). You
should be able to load your extension in any TabContents, including one wrapped
by a vanilla WebUIView in a standalone Widget.
It's more likely that the extensions service hangs off of Profile. While the
Browser has a pointer to the Profile, it's passed through its ctor at creation
time. Whoever would construct the Browser for the login screen could just as
easily construct a new LoginWidget with the same Profile.
I am proposing you end up with:
Widget
ContentsView
WebUIView (hosting the login UI in its attached TabContents)
WebUIView (hosting the keyboard UI via extension in its attached
TabContents)
-Ben
Ryan, as long as keyboard is a component extension, it will work on sign in screen with an empty Incognito profile that is used there. This way extensions like HelpApp are used. Dmitry enabled component extensions recently.
On 2011/05/13 06:12:50, Nikita Kostylev wrote: > Ryan, > as long as keyboard is a component extension, it will work on sign in screen > with an empty Incognito profile that is used there. This way extensions like > HelpApp are used. > Dmitry enabled component extensions recently. I have mostly completed a rewriting that removes subclassing the Browser. I am working on the final details and plan on uploading it or atleast a provisional version later today.
On Tue, May 17, 2011 at 4:35 PM, <rharrison@chromium.org> wrote: > On 2011/05/13 06:12:50, Nikita Kostylev wrote: >> >> Ryan, >> as long as keyboard is a component extension, it will work on sign in >> screen >> with an empty Incognito profile that is used there. This way extensions >> like >> HelpApp are used. >> Dmitry enabled component extensions recently. > > Â I have mostly completed a rewriting that removes subclassing the Browser. I > am > working on the final details and plan on uploading it or atleast a > provisional > version later today. Glad to hear you've made progress. -Scott
I have uploaded the refactored version of the code. This removes all of the DOMBrowser code and implements the WebUI login screen as a standalone widget. This has been tested locally on my desktop, but not on a full build on a device, since I am currently traveling. I will do a full build on Friday and update this CL as needed, but I am not expecting it to require any significant changes and I would like to let people see the changes that I am intending as early as possible. I have updated the description of the CL to reflect the changes that I have made and to list the known issues with the CL that I have left for follow on CLs. I think most of the people on the review list are still good candidates for reviewing the CL/have some direct interest in it, so I am not going to change the reviewer list. If you don't think it is appropriate for you to review this CL any more feel free to remove yourself from the reviewer list.
This is an awesome improvement thanks for doing it! You should find this code is more robust against changes to Browser, too :-) I'll let oshima/et al. comment on the specifics of the login display, but from a Browser/integration perspective LGTM with nits. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.cc:36: nit: delete the empty line http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.cc:44: webui_login_window_.reset(NULL); nit: reset() works http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/webui_login_view.h (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:65: // Overriden from Views. nit: // Overridden from views::View: http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:71: // views::FocusChangeListener implementation nit: Overridden from views::FocusChangeListener: http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:75: // Toggles status area visibility. nit: move these public setters above the interfaces/overrides. Don't forget to update the .cc to match order. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:107: virtual void InitVirtualKeyboard(); Why would someone override these? (I see they're virtual). Can you add a comment?
http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.cc:22: webui_login_window_->Close(); if you want to keep scoped_ptr, you have to release it i think. Or maybe simply call Destroy()? http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.cc:44: webui_login_window_.reset(NULL); Don't you have to Close it? http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display.h (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.h:85: scoped_ptr<views::Widget> webui_login_window_; widget manages its memory by itself, so you don't need scoped_ptr here. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display_host.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display_host.cc:15: remove empty line. same for the rest. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display_host.cc:32: return NULL; indent http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/webui_login_view.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:142: views::View* focused_now) { indent http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:200: } nuke {} http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:277: std::string cname = GetClassName(); isn't this view->GetClassName? http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/webui_login_view.h (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:9: #include "chrome/browser/chromeos/cros/cros_library.h" do you need this? http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:123: bool did_paint_; do we still need this?
http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display.h (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.h:52: static views::Widget* GetCurrentLoginWindow(); Are we planning to have multiple windows? http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.h:54: views::Widget* LoginWindow() { return webui_login_window_.get(); } Please move implementation to cc file. It's not a simple getter. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/webui_login_view.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:33: // NetworkLibrary header which defines enum "Status". This issue is absolete. Please remove comment and move these includes after <vector> it a separate section. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... I believe // NOLINT is not needed too. http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/views/touchui/touch_... http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:43: static void ResetXCursor() { background_view.cc has the same code. Please move it to chrome/browser/chromeos/login/helper.cc http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:86: if (!profile_->GetExtensionService()) { Are you sure that you need this block? Extensions will be initialized in ProfileManager. http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/profi... http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:181: if (!keyboard_) It's touch specific while it shouldn't be. Since we'll be reusing this (a bit refactored) class for non-touch builds with WebUI too, please take that into consideration in future CLs. Please at least place a TODO here and hide all keyboard related code under TOUCH_UI define in future CLs. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:227: void WebUILoginView::OnLocaleChanged() { Generic question: are you planning to have UI language selection control somewhere in the WebUI login implementation? In current views implementation it is placed at the new user pod which is not applicable in case of GAIA Web Auth. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/webui_login_view.h (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:35: class WebUILoginView : public views::View, Seems that we'll be reusing it later for OOBE/ScreenLock too. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:52: // Initializes the webui login view. It backgroun_url is given (non empty), nit: It > if nit: backgroun_url ? Should it be |login_url| ? http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:56: // Creates a window containing an instance of WizardContentsView as the root nit: WizardContentsView > WebUILoginView http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:58: // widget. The WebUILoginView is set in |view|. If webui login_url is non nit: |login_url| http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:87: virtual Profile* GetProfile() const OVERRIDE { return NULL; } Please move to the .cc file since it's a virtual function. http://dev.chromium.org/developers/coding-style#TOC-Inline-functions Same applies to the subsequent virtual functions with empty bodies. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:111: // Overridden from NotificationObserver. nit: indent
http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.cc:22: webui_login_window_->Close(); On 2011/05/18 18:24:54, oshima wrote: > if you want to keep scoped_ptr, you have to release it i think. > > Or maybe simply call Destroy()? Removed the scoped ptr http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.cc:36: On 2011/05/18 16:50:51, Ben Goodger wrote: > nit: delete the empty line Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.cc:44: webui_login_window_.reset(NULL); On 2011/05/18 18:24:54, oshima wrote: > Don't you have to Close it? This correct, my mistake. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.cc:44: webui_login_window_.reset(NULL); On 2011/05/18 16:50:51, Ben Goodger wrote: > nit: reset() works I changed this to a normal pointer instead of a scoped_ptr http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display.h (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.h:52: static views::Widget* GetCurrentLoginWindow(); On 2011/05/18 20:22:12, Nikita Kostylev wrote: > Are we planning to have multiple windows? No, this is a convenience method, so that we can get the Widget associated with the login screen for plumbing the extension events. I preferred to not have that code need to know that this class is a Singleton, since that may change in the future. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.h:54: views::Widget* LoginWindow() { return webui_login_window_.get(); } On 2011/05/18 20:22:12, Nikita Kostylev wrote: > Please move implementation to cc file. It's not a simple getter. Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.h:85: scoped_ptr<views::Widget> webui_login_window_; On 2011/05/18 18:24:54, oshima wrote: > widget manages its memory by itself, so you don't need scoped_ptr here. Did not know that, thanks for the catch. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display_host.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display_host.cc:15: On 2011/05/18 18:24:54, oshima wrote: > remove empty line. same for the rest. Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display_host.cc:32: return NULL; On 2011/05/18 18:24:54, oshima wrote: > indent Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/webui_login_view.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:33: // NetworkLibrary header which defines enum "Status". On 2011/05/18 20:22:12, Nikita Kostylev wrote: > This issue is absolete. Please remove comment and move these includes after > <vector> it a separate section. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... > > I believe // NOLINT is not needed too. > http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/views/touchui/touch_... Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:43: static void ResetXCursor() { On 2011/05/18 20:22:12, Nikita Kostylev wrote: > background_view.cc has the same code. > Please move it to chrome/browser/chromeos/login/helper.cc Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:86: if (!profile_->GetExtensionService()) { On 2011/05/18 20:22:12, Nikita Kostylev wrote: > Are you sure that you need this block? > Extensions will be initialized in ProfileManager. > http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/profi... Since we are not in a proper logged in browser I don't think that the extensions will be enabled, since right above in the ProfileManager it tests if we are logged in. This was done to allow the keyboard to be used. If there is a preferred way to do this I would like to know about it, since this seems a bit hacky to me. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:142: views::View* focused_now) { On 2011/05/18 18:24:54, oshima wrote: > indent Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:181: if (!keyboard_) On 2011/05/18 20:22:12, Nikita Kostylev wrote: > It's touch specific while it shouldn't be. > Since we'll be reusing this (a bit refactored) class for non-touch builds with > WebUI too, please take that into consideration in future CLs. > > Please at least place a TODO here and hide all keyboard related code under > TOUCH_UI define in future CLs. Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:200: } On 2011/05/18 18:24:54, oshima wrote: > nuke {} Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:227: void WebUILoginView::OnLocaleChanged() { On 2011/05/18 20:22:12, Nikita Kostylev wrote: > Generic question: are you planning to have UI language selection control > somewhere in the WebUI login implementation? > > In current views implementation it is placed at the new user pod which is not > applicable in case of GAIA Web Auth. I had not thought about it. I think we will have to figure out how to handle it correctly. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:277: std::string cname = GetClassName(); On 2011/05/18 18:24:54, oshima wrote: > isn't this view->GetClassName? yes http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/webui_login_view.h (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:9: #include "chrome/browser/chromeos/cros/cros_library.h" On 2011/05/18 18:24:54, oshima wrote: > do you need this? nope http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:35: class WebUILoginView : public views::View, On 2011/05/18 20:22:12, Nikita Kostylev wrote: > Seems that we'll be reusing it later for OOBE/ScreenLock too. Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:52: // Initializes the webui login view. It backgroun_url is given (non empty), On 2011/05/18 20:22:12, Nikita Kostylev wrote: > nit: It > if > nit: backgroun_url ? > Should it be |login_url| ? Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:56: // Creates a window containing an instance of WizardContentsView as the root On 2011/05/18 20:22:12, Nikita Kostylev wrote: > nit: WizardContentsView > WebUILoginView Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:58: // widget. The WebUILoginView is set in |view|. If webui login_url is non On 2011/05/18 20:22:12, Nikita Kostylev wrote: > nit: |login_url| Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:65: // Overriden from Views. On 2011/05/18 16:50:51, Ben Goodger wrote: > nit: // Overridden from views::View: Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:71: // views::FocusChangeListener implementation On 2011/05/18 16:50:51, Ben Goodger wrote: > nit: Overridden from views::FocusChangeListener: Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:75: // Toggles status area visibility. On 2011/05/18 16:50:51, Ben Goodger wrote: > nit: move these public setters above the interfaces/overrides. Don't forget to > update the .cc to match order. Removed them since I don't actually use them. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:87: virtual Profile* GetProfile() const OVERRIDE { return NULL; } On 2011/05/18 20:22:12, Nikita Kostylev wrote: > Please move to the .cc file since it's a virtual function. > http://dev.chromium.org/developers/coding-style#TOC-Inline-functions > > Same applies to the subsequent virtual functions with empty bodies. Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:107: virtual void InitVirtualKeyboard(); On 2011/05/18 16:50:51, Ben Goodger wrote: > Why would someone override these? (I see they're virtual). Can you add a > comment? We don't need to override them. That was an artifact of having cribbed this from the TouchBrowserFrameView. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:111: // Overridden from NotificationObserver. On 2011/05/18 20:22:12, Nikita Kostylev wrote: > nit: indent Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.h:123: bool did_paint_; On 2011/05/18 18:24:54, oshima wrote: > do we still need this? I do not know. I left it in since it was in the code that I was cribbing from TouchBrowserFrameView. Should I contact sky about this?
http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/dom_login_display.cc (right): http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.cc:49: } nuke {} http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.cc:124: webui_login_window_(NULL) { webui_login_view_(NULL) http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.cc:125: remove empty line http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/dom_login_display.h (right): http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.h:53: views::Widget* LoginWindow(); Please add comments for these method. http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.h:83: WebUILoginView* webui_login_view_; // owned by webui_login_window_ http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/dom_login_display_host.h (right): http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display_host.h:12: #include "ui/gfx/rect.h" forward decl seems to be enough. http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/helper.cc (right): http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/helper.cc:226: // TODO(sky): nuke this once new window manager is in place. can you double chceck with Dan (derat) if this is still true? http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/webui_login_view.h (right): http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/webui_login_view.h:117: bool did_paint_; can you check with Dan if this is still necessary. WM should know if it's painted by now.
http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/webui_login_view.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:86: if (!profile_->GetExtensionService()) { On 2011/05/19 00:31:43, rharrison wrote: > On 2011/05/18 20:22:12, Nikita Kostylev wrote: > > Are you sure that you need this block? > > Extensions will be initialized in ProfileManager. > > > http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/profi... > > Since we are not in a proper logged in browser I don't think that the extensions > will be enabled, since right above in the ProfileManager it tests if we are > logged in. This was done to allow the keyboard to be used. If there is a > preferred way to do this I would like to know about it, since this seems a bit > hacky to me. Let's clarify 2 things: 1. Is keyboard extension a component one? I think that's the case. If it's not, it should be. 2. As you could see in ProfileManager, in case if we're not logged, only component extensions should be enabled. That's all you need. So you should remove this code and check that keyboard would still work. if (!logged_in_ && (!command_line.HasSwitch(switches::kTestType) || command_line.HasSwitch(switches::kLoginProfile))) { init_extensions = false; } Also you're checking whether profile is incognito one, it will be (as it is a pre-logged in state). I'm not sure what fetching original one would return in that case, have to check code. In either way, component extensions initialization that you need should not be handled in this place.
http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display.h (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.h:52: static views::Widget* GetCurrentLoginWindow(); On 2011/05/19 00:31:43, rharrison wrote: > On 2011/05/18 20:22:12, Nikita Kostylev wrote: > > Are we planning to have multiple windows? > No, this is a convenience method, so that we can get the Widget associated with > the login screen for plumbing the extension events. I preferred to not have that > code need to know that this class is a Singleton, since that may change in the > future. Ok, so my point was to rename it to just GetLoginWindow() then.
I have made changes for most of the comments. I have sent an email to sky and derat about the comments related to the wm, awating a response. I will be travelling for the rest of the day, so the soonest I will be able to respond to more comments is Friday. oshima, ben, and nkostylev has been responding to this CL. Is this sufficient for this CL? Should I trim the other reviewer? http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/dom_login_display.h (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/dom_login_display.h:52: static views::Widget* GetCurrentLoginWindow(); On 2011/05/19 03:57:22, Nikita Kostylev wrote: > On 2011/05/19 00:31:43, rharrison wrote: > > On 2011/05/18 20:22:12, Nikita Kostylev wrote: > > > Are we planning to have multiple windows? > > No, this is a convenience method, so that we can get the Widget associated > with > > the login screen for plumbing the extension events. I preferred to not have > that > > code need to know that this class is a Singleton, since that may change in the > > future. > > Ok, so my point was to rename it to just GetLoginWindow() then. Done. http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/webui_login_view.cc (right): http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/webui_login_view.cc:86: if (!profile_->GetExtensionService()) { On 2011/05/19 03:53:33, Nikita Kostylev wrote: > On 2011/05/19 00:31:43, rharrison wrote: > > On 2011/05/18 20:22:12, Nikita Kostylev wrote: > > > Are you sure that you need this block? > > > Extensions will be initialized in ProfileManager. > > > > > > http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/profi... > > > > Since we are not in a proper logged in browser I don't think that the > extensions > > will be enabled, since right above in the ProfileManager it tests if we are > > logged in. This was done to allow the keyboard to be used. If there is a > > preferred way to do this I would like to know about it, since this seems a bit > > hacky to me. > > Let's clarify 2 things: > 1. Is keyboard extension a component one? I think that's the case. > If it's not, it should be. > 2. As you could see in ProfileManager, in case if we're not logged, only > component extensions should be enabled. That's all you need. So you should > remove this code and check that keyboard would still work. > > if (!logged_in_ && > (!command_line.HasSwitch(switches::kTestType) || > command_line.HasSwitch(switches::kLoginProfile))) { > init_extensions = false; > } > > Also you're checking whether profile is incognito one, it will be (as it is a > pre-logged in state). I'm not sure what fetching original one would return in > that case, have to check code. > > In either way, component extensions initialization that you need should not be > handled in this place. I think it is a component extension. I misunderstood how extensions are initialized. You are correct, I shouldn't need this hack. I will remove it and test that everything sets up correctly. http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/dom_login_display.cc (right): http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.cc:49: } On 2011/05/19 01:45:19, oshima wrote: > nuke {} Done. http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.cc:124: webui_login_window_(NULL) { On 2011/05/19 01:45:19, oshima wrote: > webui_login_view_(NULL) Done. http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.cc:125: On 2011/05/19 01:45:19, oshima wrote: > remove empty line Done. http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/dom_login_display.h (right): http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.h:53: views::Widget* LoginWindow(); On 2011/05/19 01:45:19, oshima wrote: > Please add comments for these method. Done. http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display.h:83: WebUILoginView* webui_login_view_; On 2011/05/19 01:45:19, oshima wrote: > // owned by webui_login_window_ Done. http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/dom_login_display_host.h (right): http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/dom_login_display_host.h:12: #include "ui/gfx/rect.h" On 2011/05/19 01:45:19, oshima wrote: > forward decl seems to be enough. Done. http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/helper.cc (right): http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... chrome/browser/chromeos/login/helper.cc:226: // TODO(sky): nuke this once new window manager is in place. On 2011/05/19 01:45:19, oshima wrote: > can you double chceck with Dan (derat) if this is still true? Sent him an e-mail, awaiting a response.
LGTM on my bits. Pleas address Nikita's comments for extension before submit. WM/Cursor change isn't the scope of this change so you can go ahead if you didn't get answer in time, but please do fix if it's necessary in follow up CL.
Typically anyone that comments with a request should give LGTM to acknowledge their request was satisfied one way or another. e.g. I gave a suggestion about the overall design and said LGTM because I was satisfied that my comments were addressed. The set of people giving LGTM for an individual changelist should also encompass OWNERS for affected files. -Ben On Thu, May 19, 2011 at 9:45 AM, <rharrison@chromium.org> wrote: > I have made changes for most of the comments. I have sent an email to sky > and > derat about the comments related to the wm, awating a response. I will be > travelling for the rest of the day, so the soonest I will be able to > respond to > more comments is Friday. > > oshima, ben, and nkostylev has been responding to this CL. Is this > sufficient > for this CL? Should I trim the other reviewer? > > > > > http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... > File chrome/browser/chromeos/login/dom_login_display.h (right): > > > http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/dom_login_display.h:52: static > views::Widget* GetCurrentLoginWindow(); > On 2011/05/19 03:57:22, Nikita Kostylev wrote: > >> On 2011/05/19 00:31:43, rharrison wrote: >> > On 2011/05/18 20:22:12, Nikita Kostylev wrote: >> > > Are we planning to have multiple windows? >> > No, this is a convenience method, so that we can get the Widget >> > associated > >> with >> > the login screen for plumbing the extension events. I preferred to >> > not have > >> that >> > code need to know that this class is a Singleton, since that may >> > change in the > >> > future. >> > > Ok, so my point was to rename it to just GetLoginWindow() then. >> > > Done. > > > > http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... > File chrome/browser/chromeos/login/webui_login_view.cc (right): > > > http://codereview.chromium.org/6973029/diff/9001/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/webui_login_view.cc:86: if > (!profile_->GetExtensionService()) { > On 2011/05/19 03:53:33, Nikita Kostylev wrote: > >> On 2011/05/19 00:31:43, rharrison wrote: >> > On 2011/05/18 20:22:12, Nikita Kostylev wrote: >> > > Are you sure that you need this block? >> > > Extensions will be initialized in ProfileManager. >> > > >> > >> > > > http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/profi... > >> > >> > Since we are not in a proper logged in browser I don't think that >> > the > >> extensions >> > will be enabled, since right above in the ProfileManager it tests if >> > we are > >> > logged in. This was done to allow the keyboard to be used. If there >> > is a > >> > preferred way to do this I would like to know about it, since this >> > seems a bit > >> > hacky to me. >> > > Let's clarify 2 things: >> 1. Is keyboard extension a component one? I think that's the case. >> If it's not, it should be. >> 2. As you could see in ProfileManager, in case if we're not logged, >> > only > >> component extensions should be enabled. That's all you need. So you >> > should > >> remove this code and check that keyboard would still work. >> > > if (!logged_in_ && >> (!command_line.HasSwitch(switches::kTestType) || >> command_line.HasSwitch(switches::kLoginProfile))) { >> init_extensions = false; >> } >> > > Also you're checking whether profile is incognito one, it will be (as >> > it is a > >> pre-logged in state). I'm not sure what fetching original one would >> > return in > >> that case, have to check code. >> > > In either way, component extensions initialization that you need >> > should not be > >> handled in this place. >> > > I think it is a component extension. I misunderstood how extensions are > initialized. You are correct, I shouldn't need this hack. I will remove > it and test that everything sets up correctly. > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/dom_login_display.cc (right): > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/dom_login_display.cc:49: } > On 2011/05/19 01:45:19, oshima wrote: > >> nuke {} >> > > Done. > > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/dom_login_display.cc:124: > webui_login_window_(NULL) { > On 2011/05/19 01:45:19, oshima wrote: > >> webui_login_view_(NULL) >> > > Done. > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/dom_login_display.cc:125: > > On 2011/05/19 01:45:19, oshima wrote: > >> remove empty line >> > > Done. > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > > File chrome/browser/chromeos/login/dom_login_display.h (right): > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/dom_login_display.h:53: views::Widget* > LoginWindow(); > > On 2011/05/19 01:45:19, oshima wrote: > >> Please add comments for these method. >> > > Done. > > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/dom_login_display.h:83: WebUILoginView* > webui_login_view_; > On 2011/05/19 01:45:19, oshima wrote: > >> // owned by webui_login_window_ >> > > Done. > > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/dom_login_display_host.h (right): > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/dom_login_display_host.h:12: #include > "ui/gfx/rect.h" > On 2011/05/19 01:45:19, oshima wrote: > >> forward decl seems to be enough. >> > > Done. > > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/helper.cc (right): > > > http://codereview.chromium.org/6973029/diff/11012/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/helper.cc:226: // TODO(sky): nuke this > once new window manager is in place. > On 2011/05/19 01:45:19, oshima wrote: > >> can you double chceck with Dan (derat) if this is still true? >> > > Sent him an e-mail, awaiting a response. > > > http://codereview.chromium.org/6973029/ >
Removed unneeded code and fixed a segfault that was occurring due to me forgetting it initialize profile_
LGTM On Fri, May 20, 2011 at 11:02 AM, <rharrison@chromium.org> wrote: > Removed unneeded code and fixed a segfault that was occurring due to me > forgetting it initialize profile_ > > > http://codereview.chromium.org/6973029/ >
LGTM
http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/background_view.cc (left): http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/background_view.cc:419: params.push_back(did_paint_ ? 1 : 0); Did you also update the window manager side of things? http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/webui_login_view.cc (right): http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/webui_login_view.cc:104: GdkWindow* gdk_window = window->GetNativeView()->window; Are you sure we still need this? I ask as we don't do it for all windows. For example, we don't do it for the browser. http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/webui_login_view.h (right): http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/webui_login_view.h:105: virtual void Observe(NotificationType type, OVERRIDE
http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/background_view.cc (left): http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/background_view.cc:419: params.push_back(did_paint_ ? 1 : 0); On 2011/05/23 15:16:39, sky wrote: > Did you also update the window manager side of things? The window manager does not appear to do anything with the parameters for this window type. I think the change was made in the window manager to not require this and this code was never cleaned up. The comments in chromeos_wm_ipc_enums.h need to be cleaned up though. I will have to put that in an new CL, since it is in platform/cros. http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/webui_login_view.cc (right): http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/webui_login_view.cc:104: GdkWindow* gdk_window = window->GetNativeView()->window; On 2011/05/23 15:16:39, sky wrote: > Are you sure we still need this? I ask as we don't do it for all windows. For > example, we don't do it for the browser. This doesn't appear to be needed anymore. http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/webui_login_view.h (right): http://codereview.chromium.org/6973029/diff/20001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/webui_login_view.h:105: virtual void Observe(NotificationType type, On 2011/05/23 15:16:39, sky wrote: > OVERRIDE Done.
LGTM
Can't apply patch for file chrome/browser/chromeos/frame/dom_browser_view.h. patching file chrome/browser/chromeos/frame/dom_browser_view.h svn: warning: 'chrome/browser/chromeos/frame/dom_browser_view.h' not found svn: Can't open file 'chrome/browser/chromeos/frame/dom_browser_view.h': No such file or directory |
