Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1261433013:
Implement online reauth UI for Locked Profiles on Mac (Closed)
DescriptionImplement online reauth UI for Locked Profiles on Mac
This CL implements the Mac UI for online reauth on Mac to match the
flow already implemented on other desktop platforms.
BUG=472596
TEST=
1. Create a supervised user
2. In a regular profile, click on the Avatar button
3. Click on "Exit and Childlock"
4. Select the locked profile and enter the wrong password
5. A window should open allowing online reauth
Committed: https://crrev.com/b857f9db847bf0e56eefbf00869bfb98ceb61d2a
Cr-Commit-Position: refs/heads/master@{#343110}
Patch Set 1 #Patch Set 2 : Fix Linux/Windows build #Patch Set 3 : Fix views code #
Total comments: 6
Patch Set 4 : Address review feedback #
Total comments: 20
Patch Set 5 : Address review feedback #Patch Set 6 : Address feedback #Patch Set 7 : Use ConstrainedWindow. #
Total comments: 2
Messages
Total messages: 33 (8 generated)
anthonyvd@chromium.org changed reviewers: + rogerta@chromium.org
Hi Roger, can you please take a look at this CL before I add more owners (specifically the code moved from user_manager_view to profile_window)? Thanks a lot!
Thanks for handling the mac implementation Anthony. Some comments below. https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:265: GURL url = web_contents_->GetURL(); I don't believe this is correct. It should be |web_contents()->GetURL()|. Lines 286 and 288 below are correct, but could be replaced with web_contents() too. The if() check at like 278 essentially sees if we are still observing the WebView's web_content. (In retrospect, I'm not sure if my comment at lines 275-277 is descriptive enough). https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.h (right): https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_window.h:45: class ReauthDialogObserver : public content::WebContentsObserver { Please add class comment. https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:90: Observe(web_contents); Can you put this Observe() call into the ctor of ReauthDialogObserver? That way it is not duplicated in the mac and views impl. If we do this, do we still need a |web_contents_| member variable in this class and in the ReauthDialogObserver class? WebContentsObserver already saves the pointer and makes it available via the web_contents() method. See comment in profile_window.cc too.
https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:265: GURL url = web_contents_->GetURL(); On 2015/08/05 13:36:05, Roger Tawa wrote: > I don't believe this is correct. It should be |web_contents()->GetURL()|. > > Lines 286 and 288 below are correct, but could be replaced with web_contents() > too. The if() check at like 278 essentially sees if we are still observing the > WebView's web_content. (In retrospect, I'm not sure if my comment at lines > 275-277 is descriptive enough). Done. https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.h (right): https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_window.h:45: class ReauthDialogObserver : public content::WebContentsObserver { On 2015/08/05 13:36:05, Roger Tawa wrote: > Please add class comment. Done. https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/1261433013/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:90: Observe(web_contents); On 2015/08/05 13:36:05, Roger Tawa wrote: > Can you put this Observe() call into the ctor of ReauthDialogObserver? That way > it is not duplicated in the mac and views impl. > > If we do this, do we still need a |web_contents_| member variable in this class > and in the ReauthDialogObserver class? WebContentsObserver already saves the > pointer and makes it available via the web_contents() method. See comment in > profile_window.cc too. Done.
anthonyvd@chromium.org changed reviewers: + groby@chromium.org, mlerman@chromium.org, sky@chromium.org
Hello everyone! This CL implements the mac UI for the work rogerta@ has done in https://codereview.chromium.org/1220843003/. Can you please take a look at the following files: groby@: chrome/browser/ui/cocoa/* sky@: chrome/browser/ui/views/* mlerman@: chrome/browser/profiles/* Thank you very much for your time :)
The code you wrote in profile_window looks good, although it has nothing to do with profiles (I really can't validate what you wrote very well) and perhaps belongs in c/b/ui/webui/signin, since it relates to the user manager?
https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.h (right): https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.h:60: base::scoped_nsobject<ReauthDialogWindowController> reauthWindow_; reauth_window_, please (Yes, the style interesection of ObjC and C++ is horrible :) https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (left): https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:28: Either keep this blank line or remove the one at the end of the namespace. (I'm in favor of keeping the blank line) https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:111: [[NSNotificationCenter defaultCenter] removeObserver:self]; removeObserver should probably be handled in -dealloc (This is _probably_ safe, but I have no idea if windowWillClose is guaranteed in all cases, so let's be safe) https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:125: UserManager::kReauthDialogHeight); You can save yourself quite a few of these lines by just using NSWindow's -center method and using a contentRect with a 0,0 origin. https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:135: [window setMinSize:NSMakeSize(UserManager::kReauthDialogWidth, That's the minimum size for the frame, including window chrome. Are you sure you don't want [window setMinSize:[window frame].size] https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = [NSApp beginModalSessionForWindow:[self window]]; IIRC, we usually run modal things as sheets, not as a modal session. (Specifically, via ConstrainedWindow) Is there a specific reason why this is introducing an entirely new UI mode? https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:341: if (reauthWindow_) { No need to check - send to nil always works. https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:386: CloseReauthDialog(); Just call CloseReauthDialog - it's perfectly fine with a nil reauthWindow_
lgtm Thanks Anthony.
LGTM
Thanks for your time everyone. Most feedback is addressed with the 2 latest patchsets, except one question regarding constrained windows. https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.h (right): https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.h:60: base::scoped_nsobject<ReauthDialogWindowController> reauthWindow_; On 2015/08/05 21:16:38, groby wrote: > reauth_window_, please > > (Yes, the style interesection of ObjC and C++ is horrible :) Done. https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (left): https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:28: On 2015/08/05 21:16:38, groby wrote: > Either keep this blank line or remove the one at the end of the namespace. (I'm > in favor of keeping the blank line) Done. https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:111: [[NSNotificationCenter defaultCenter] removeObserver:self]; On 2015/08/05 21:16:38, groby wrote: > removeObserver should probably be handled in -dealloc > > (This is _probably_ safe, but I have no idea if windowWillClose is guaranteed in > all cases, so let's be safe) Done. https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:125: UserManager::kReauthDialogHeight); On 2015/08/05 21:16:38, groby wrote: > You can save yourself quite a few of these lines by just using NSWindow's > -center method and using a contentRect with a 0,0 origin. Done. https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:135: [window setMinSize:NSMakeSize(UserManager::kReauthDialogWidth, On 2015/08/05 21:16:38, groby wrote: > That's the minimum size for the frame, including window chrome. Are you sure you > don't want > > [window setMinSize:[window frame].size] Done. https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = [NSApp beginModalSessionForWindow:[self window]]; On 2015/08/05 21:16:38, groby wrote: > IIRC, we usually run modal things as sheets, not as a modal session. > (Specifically, via ConstrainedWindow) > > Is there a specific reason why this is introducing an entirely new UI mode? To be honest it's only because I'm very unfamiliar with the way we do things on mac. Because of this, I based this class on the User Manager itself (which isn't a ConstrainedWindow) but wanted it to stay on top so that's what I figured worked. I'd be happy to change it to use sheets and ConstrainedWindows but I'd need a bit of guidance w.r.t doing so. I've tried using ShowConstrainedWebDialog: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... as well as creating the ConstrainedWindowMac manually with no success. In both cases, Chrome crashes when checking (if (delegate_)) in WebContentsModalDialogManager::ShowDialogWithManager. Do you have any guidance as to what I'm doing wrong? https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:341: if (reauthWindow_) { On 2015/08/05 21:16:38, groby wrote: > No need to check - send to nil always works. Done. https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:386: CloseReauthDialog(); On 2015/08/05 21:16:38, groby wrote: > Just call CloseReauthDialog - it's perfectly fine with a nil reauthWindow_ Done.
lgtm - you may want sky@ to take another look at what you moved into c/b/ui/user_manager
https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = [NSApp beginModalSessionForWindow:[self window]]; On 2015/08/06 19:08:04, anthonyvd wrote: > On 2015/08/05 21:16:38, groby wrote: > > IIRC, we usually run modal things as sheets, not as a modal session. > > (Specifically, via ConstrainedWindow) > > > > Is there a specific reason why this is introducing an entirely new UI mode? > > To be honest it's only because I'm very unfamiliar with the way we do things on > mac. Because of this, I based this class on the User Manager itself (which isn't > a ConstrainedWindow) but wanted it to stay on top so that's what I figured > worked. > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd need a > bit of guidance w.r.t doing so. I've tried using ShowConstrainedWebDialog: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > as well as creating the ConstrainedWindowMac manually with no success. In both > cases, Chrome crashes when checking (if (delegate_)) in > WebContentsModalDialogManager::ShowDialogWithManager. > > Do you have any guidance as to what I'm doing wrong? First, thank you for willing to make this change, even though it's a large modification to this CL Second: Do you have a CL I can take a look at? Also, happy to have a quick VC chat if you think that'll help https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = [NSApp beginModalSessionForWindow:[self window]]; On 2015/08/06 19:08:04, anthonyvd wrote: > On 2015/08/05 21:16:38, groby wrote: > > IIRC, we usually run modal things as sheets, not as a modal session. > > (Specifically, via ConstrainedWindow) > > > > Is there a specific reason why this is introducing an entirely new UI mode? > > To be honest it's only because I'm very unfamiliar with the way we do things on > mac. Because of this, I based this class on the User Manager itself (which isn't > a ConstrainedWindow) but wanted it to stay on top so that's what I figured > worked. > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd need a > bit of guidance w.r.t doing so. I've tried using ShowConstrainedWebDialog: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > as well as creating the ConstrainedWindowMac manually with no success. In both > cases, Chrome crashes when checking (if (delegate_)) in > WebContentsModalDialogManager::ShowDialogWithManager. That seems to indicate a NULL WebContentsDialogManager, which is... odd. > > Do you have any guidance as to what I'm doing wrong? General guidance: If you create ConstrainedWindowMac manually, AutofillDialogCocoa::Show would be the example. (Mostly because I wrote it, and so am familiar with it :) You'll need a (subclassed) NSWindowController to drive window behavior and that allocates a ConstrainedWindowCustomWindowMac, you need a CustomConstrainedWIndowSheet that's bound to the window controller's window, and you need a ConstrainedWindowMac that takes the sheet and the web contents. It's a bit of an elaborate dance. AutofillSignInContainer shows how to load an actual web page in there. Assuming you want to avoid that and stick with ShowConstrainedWebDialog, that does take care of this whole dance. As said above, your crash is likely a bad manager, which - first guess - is caused by a bad BrowserContext. What are you using as BrowserContext?
https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = [NSApp beginModalSessionForWindow:[self window]]; On 2015/08/06 19:49:25, groby wrote: > On 2015/08/06 19:08:04, anthonyvd wrote: > > On 2015/08/05 21:16:38, groby wrote: > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > (Specifically, via ConstrainedWindow) > > > > > > Is there a specific reason why this is introducing an entirely new UI mode? > > > > To be honest it's only because I'm very unfamiliar with the way we do things > on > > mac. Because of this, I based this class on the User Manager itself (which > isn't > > a ConstrainedWindow) but wanted it to stay on top so that's what I figured > > worked. > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd need a > > bit of guidance w.r.t doing so. I've tried using ShowConstrainedWebDialog: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > as well as creating the ConstrainedWindowMac manually with no success. In both > > cases, Chrome crashes when checking (if (delegate_)) in > > WebContentsModalDialogManager::ShowDialogWithManager. > > > > Do you have any guidance as to what I'm doing wrong? > > First, thank you for willing to make this change, even though it's a large > modification to this CL > Second: Do you have a CL I can take a look at? Also, happy to have a quick VC > chat if you think that'll help I don't have a CL but I'll create one from what you described in your next comment and send it to you if I'm still having issues. :) https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = [NSApp beginModalSessionForWindow:[self window]]; On 2015/08/06 19:49:25, groby wrote: > On 2015/08/06 19:08:04, anthonyvd wrote: > > On 2015/08/05 21:16:38, groby wrote: > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > (Specifically, via ConstrainedWindow) > > > > > > Is there a specific reason why this is introducing an entirely new UI mode? > > > > To be honest it's only because I'm very unfamiliar with the way we do things > on > > mac. Because of this, I based this class on the User Manager itself (which > isn't > > a ConstrainedWindow) but wanted it to stay on top so that's what I figured > > worked. > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd need a > > bit of guidance w.r.t doing so. I've tried using ShowConstrainedWebDialog: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > as well as creating the ConstrainedWindowMac manually with no success. In both > > cases, Chrome crashes when checking (if (delegate_)) in > > WebContentsModalDialogManager::ShowDialogWithManager. > That seems to indicate a NULL WebContentsDialogManager, which is... odd. > I thought so too :) > > > > Do you have any guidance as to what I'm doing wrong? > > General guidance: > If you create ConstrainedWindowMac manually, AutofillDialogCocoa::Show would be > the example. (Mostly because I wrote it, and so am familiar with it :) > > You'll need a (subclassed) NSWindowController to drive window behavior and that > allocates a ConstrainedWindowCustomWindowMac, you need a > CustomConstrainedWIndowSheet that's bound to the window controller's window, and > you need a ConstrainedWindowMac that takes the sheet and the web contents. It's > a bit of an elaborate dance. AutofillSignInContainer shows how to load an actual > web page in there. > > Assuming you want to avoid that and stick with ShowConstrainedWebDialog, that > does take care of this whole dance. As said above, your crash is likely a bad > manager, which - first guess - is caused by a bad BrowserContext. What are you > using as BrowserContext? Thanks a ton for this info, I'll try it out! The BrowserContext I'm using is the UserManager's web_ui's BrowserContext. If I read everything correctly, that BrowserContext would actually be the System Profile (which could be an issue since it's special in many ways). Maybe it doesn't have a WebContentsDialogManager?
On 2015/08/06 20:07:19, anthonyvd wrote: > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = [NSApp > beginModalSessionForWindow:[self window]]; > On 2015/08/06 19:49:25, groby wrote: > > On 2015/08/06 19:08:04, anthonyvd wrote: > > > On 2015/08/05 21:16:38, groby wrote: > > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > > (Specifically, via ConstrainedWindow) > > > > > > > > Is there a specific reason why this is introducing an entirely new UI > mode? > > > > > > To be honest it's only because I'm very unfamiliar with the way we do things > > on > > > mac. Because of this, I based this class on the User Manager itself (which > > isn't > > > a ConstrainedWindow) but wanted it to stay on top so that's what I figured > > > worked. > > > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd need > a > > > bit of guidance w.r.t doing so. I've tried using ShowConstrainedWebDialog: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > as well as creating the ConstrainedWindowMac manually with no success. In > both > > > cases, Chrome crashes when checking (if (delegate_)) in > > > WebContentsModalDialogManager::ShowDialogWithManager. > > > > > > Do you have any guidance as to what I'm doing wrong? > > > > First, thank you for willing to make this change, even though it's a large > > modification to this CL > > Second: Do you have a CL I can take a look at? Also, happy to have a quick VC > > chat if you think that'll help > > I don't have a CL but I'll create one from what you described in your next > comment and send it to you if I'm still having issues. :) > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = [NSApp > beginModalSessionForWindow:[self window]]; > On 2015/08/06 19:49:25, groby wrote: > > On 2015/08/06 19:08:04, anthonyvd wrote: > > > On 2015/08/05 21:16:38, groby wrote: > > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > > (Specifically, via ConstrainedWindow) > > > > > > > > Is there a specific reason why this is introducing an entirely new UI > mode? > > > > > > To be honest it's only because I'm very unfamiliar with the way we do things > > on > > > mac. Because of this, I based this class on the User Manager itself (which > > isn't > > > a ConstrainedWindow) but wanted it to stay on top so that's what I figured > > > worked. > > > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd need > a > > > bit of guidance w.r.t doing so. I've tried using ShowConstrainedWebDialog: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > as well as creating the ConstrainedWindowMac manually with no success. In > both > > > cases, Chrome crashes when checking (if (delegate_)) in > > > WebContentsModalDialogManager::ShowDialogWithManager. > > That seems to indicate a NULL WebContentsDialogManager, which is... odd. > > > > I thought so too :) > > > > > > > Do you have any guidance as to what I'm doing wrong? > > > > General guidance: > > If you create ConstrainedWindowMac manually, AutofillDialogCocoa::Show would > be > > the example. (Mostly because I wrote it, and so am familiar with it :) > > > > You'll need a (subclassed) NSWindowController to drive window behavior and > that > > allocates a ConstrainedWindowCustomWindowMac, you need a > > CustomConstrainedWIndowSheet that's bound to the window controller's window, > and > > you need a ConstrainedWindowMac that takes the sheet and the web contents. > It's > > a bit of an elaborate dance. AutofillSignInContainer shows how to load an > actual > > web page in there. > > > > Assuming you want to avoid that and stick with ShowConstrainedWebDialog, that > > does take care of this whole dance. As said above, your crash is likely a bad > > manager, which - first guess - is caused by a bad BrowserContext. What are you > > using as BrowserContext? > > Thanks a ton for this info, I'll try it out! > > The BrowserContext I'm using is the UserManager's web_ui's BrowserContext. If I > read everything correctly, that BrowserContext would actually be the System > Profile (which could be an issue since it's special in many ways). Maybe it > doesn't have a WebContentsDialogManager? That would be correct :) Only tabs, extensions windows, and app windows have one, as far as I know. You could always add one with CreateForWebContents, except that might have repercussions - I'm not that familiar with WebUI. Question: Do you have any similar dialogs on the UserManager that are already implemented for Mac and that you can lean on?
On 2015/08/06 20:27:27, groby wrote: > On 2015/08/06 20:07:19, anthonyvd wrote: > > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): > > > > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = > [NSApp > > beginModalSessionForWindow:[self window]]; > > On 2015/08/06 19:49:25, groby wrote: > > > On 2015/08/06 19:08:04, anthonyvd wrote: > > > > On 2015/08/05 21:16:38, groby wrote: > > > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > > > (Specifically, via ConstrainedWindow) > > > > > > > > > > Is there a specific reason why this is introducing an entirely new UI > > mode? > > > > > > > > To be honest it's only because I'm very unfamiliar with the way we do > things > > > on > > > > mac. Because of this, I based this class on the User Manager itself (which > > > isn't > > > > a ConstrainedWindow) but wanted it to stay on top so that's what I figured > > > > worked. > > > > > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd > need > > a > > > > bit of guidance w.r.t doing so. I've tried using ShowConstrainedWebDialog: > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > as well as creating the ConstrainedWindowMac manually with no success. In > > both > > > > cases, Chrome crashes when checking (if (delegate_)) in > > > > WebContentsModalDialogManager::ShowDialogWithManager. > > > > > > > > Do you have any guidance as to what I'm doing wrong? > > > > > > First, thank you for willing to make this change, even though it's a large > > > modification to this CL > > > Second: Do you have a CL I can take a look at? Also, happy to have a quick > VC > > > chat if you think that'll help > > > > I don't have a CL but I'll create one from what you described in your next > > comment and send it to you if I'm still having issues. :) > > > > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = > [NSApp > > beginModalSessionForWindow:[self window]]; > > On 2015/08/06 19:49:25, groby wrote: > > > On 2015/08/06 19:08:04, anthonyvd wrote: > > > > On 2015/08/05 21:16:38, groby wrote: > > > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > > > (Specifically, via ConstrainedWindow) > > > > > > > > > > Is there a specific reason why this is introducing an entirely new UI > > mode? > > > > > > > > To be honest it's only because I'm very unfamiliar with the way we do > things > > > on > > > > mac. Because of this, I based this class on the User Manager itself (which > > > isn't > > > > a ConstrainedWindow) but wanted it to stay on top so that's what I figured > > > > worked. > > > > > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd > need > > a > > > > bit of guidance w.r.t doing so. I've tried using ShowConstrainedWebDialog: > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > as well as creating the ConstrainedWindowMac manually with no success. In > > both > > > > cases, Chrome crashes when checking (if (delegate_)) in > > > > WebContentsModalDialogManager::ShowDialogWithManager. > > > That seems to indicate a NULL WebContentsDialogManager, which is... odd. > > > > > > > I thought so too :) > > > > > > > > > > Do you have any guidance as to what I'm doing wrong? > > > > > > General guidance: > > > If you create ConstrainedWindowMac manually, AutofillDialogCocoa::Show would > > be > > > the example. (Mostly because I wrote it, and so am familiar with it :) > > > > > > You'll need a (subclassed) NSWindowController to drive window behavior and > > that > > > allocates a ConstrainedWindowCustomWindowMac, you need a > > > CustomConstrainedWIndowSheet that's bound to the window controller's window, > > and > > > you need a ConstrainedWindowMac that takes the sheet and the web contents. > > It's > > > a bit of an elaborate dance. AutofillSignInContainer shows how to load an > > actual > > > web page in there. > > > > > > Assuming you want to avoid that and stick with ShowConstrainedWebDialog, > that > > > does take care of this whole dance. As said above, your crash is likely a > bad > > > manager, which - first guess - is caused by a bad BrowserContext. What are > you > > > using as BrowserContext? > > > > Thanks a ton for this info, I'll try it out! > > > > The BrowserContext I'm using is the UserManager's web_ui's BrowserContext. If > I > > read everything correctly, that BrowserContext would actually be the System > > Profile (which could be an issue since it's special in many ways). Maybe it > > doesn't have a WebContentsDialogManager? > > That would be correct :) Only tabs, extensions windows, and app windows have > one, as far as I know. You could always add one with CreateForWebContents, > except that might have repercussions - I'm not that familiar with WebUI. Ah, that makes sense! Unfortunately creating one through CreateForWebContents doesn't seem to be enough. Something like: web_modal::WebContentsModalDialogManager::CreateForWebContents( webContents_.get()); ShowConstrainedWebDialog( profile, new ReauthDelegate(), webContents_.get()); stops the crashes but doesn't show any dialog. I'm passing the User Manager's webContent's to ShowConstrainedWebDialog, should I be creating the dialog's web contents and pass that instead? I'll also try to do the entire ConstrainedWindowCustomWindowMac as you described, see what happens. > > Question: Do you have any similar dialogs on the UserManager that are already > implemented for Mac and that you can lean on? Unfortunately we don't. :( Again, thanks so much for your time, it's really appreciated!
On 2015/08/07 14:27:30, anthonyvd wrote: > On 2015/08/06 20:27:27, groby wrote: > > On 2015/08/06 20:07:19, anthonyvd wrote: > > > > > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > > > File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): > > > > > > > > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = > > [NSApp > > > beginModalSessionForWindow:[self window]]; > > > On 2015/08/06 19:49:25, groby wrote: > > > > On 2015/08/06 19:08:04, anthonyvd wrote: > > > > > On 2015/08/05 21:16:38, groby wrote: > > > > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > > > > (Specifically, via ConstrainedWindow) > > > > > > > > > > > > Is there a specific reason why this is introducing an entirely new UI > > > mode? > > > > > > > > > > To be honest it's only because I'm very unfamiliar with the way we do > > things > > > > on > > > > > mac. Because of this, I based this class on the User Manager itself > (which > > > > isn't > > > > > a ConstrainedWindow) but wanted it to stay on top so that's what I > figured > > > > > worked. > > > > > > > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd > > need > > > a > > > > > bit of guidance w.r.t doing so. I've tried using > ShowConstrainedWebDialog: > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > > > as well as creating the ConstrainedWindowMac manually with no success. > In > > > both > > > > > cases, Chrome crashes when checking (if (delegate_)) in > > > > > WebContentsModalDialogManager::ShowDialogWithManager. > > > > > > > > > > Do you have any guidance as to what I'm doing wrong? > > > > > > > > First, thank you for willing to make this change, even though it's a large > > > > modification to this CL > > > > Second: Do you have a CL I can take a look at? Also, happy to have a quick > > VC > > > > chat if you think that'll help > > > > > > I don't have a CL but I'll create one from what you described in your next > > > comment and send it to you if I'm still having issues. :) > > > > > > > > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = > > [NSApp > > > beginModalSessionForWindow:[self window]]; > > > On 2015/08/06 19:49:25, groby wrote: > > > > On 2015/08/06 19:08:04, anthonyvd wrote: > > > > > On 2015/08/05 21:16:38, groby wrote: > > > > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > > > > (Specifically, via ConstrainedWindow) > > > > > > > > > > > > Is there a specific reason why this is introducing an entirely new UI > > > mode? > > > > > > > > > > To be honest it's only because I'm very unfamiliar with the way we do > > things > > > > on > > > > > mac. Because of this, I based this class on the User Manager itself > (which > > > > isn't > > > > > a ConstrainedWindow) but wanted it to stay on top so that's what I > figured > > > > > worked. > > > > > > > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd > > need > > > a > > > > > bit of guidance w.r.t doing so. I've tried using > ShowConstrainedWebDialog: > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > > > as well as creating the ConstrainedWindowMac manually with no success. > In > > > both > > > > > cases, Chrome crashes when checking (if (delegate_)) in > > > > > WebContentsModalDialogManager::ShowDialogWithManager. > > > > That seems to indicate a NULL WebContentsDialogManager, which is... odd. > > > > > > > > > > I thought so too :) > > > > > > > > > > > > > Do you have any guidance as to what I'm doing wrong? > > > > > > > > General guidance: > > > > If you create ConstrainedWindowMac manually, AutofillDialogCocoa::Show > would > > > be > > > > the example. (Mostly because I wrote it, and so am familiar with it :) > > > > > > > > You'll need a (subclassed) NSWindowController to drive window behavior and > > > that > > > > allocates a ConstrainedWindowCustomWindowMac, you need a > > > > CustomConstrainedWIndowSheet that's bound to the window controller's > window, > > > and > > > > you need a ConstrainedWindowMac that takes the sheet and the web contents. > > > It's > > > > a bit of an elaborate dance. AutofillSignInContainer shows how to load an > > > actual > > > > web page in there. > > > > > > > > Assuming you want to avoid that and stick with ShowConstrainedWebDialog, > > that > > > > does take care of this whole dance. As said above, your crash is likely a > > bad > > > > manager, which - first guess - is caused by a bad BrowserContext. What are > > you > > > > using as BrowserContext? > > > > > > Thanks a ton for this info, I'll try it out! > > > > > > The BrowserContext I'm using is the UserManager's web_ui's BrowserContext. > If > > I > > > read everything correctly, that BrowserContext would actually be the System > > > Profile (which could be an issue since it's special in many ways). Maybe it > > > doesn't have a WebContentsDialogManager? > > > > That would be correct :) Only tabs, extensions windows, and app windows have > > one, as far as I know. You could always add one with CreateForWebContents, > > except that might have repercussions - I'm not that familiar with WebUI. > > Ah, that makes sense! Unfortunately creating one through CreateForWebContents > doesn't seem to be enough. Something like: > > web_modal::WebContentsModalDialogManager::CreateForWebContents( > webContents_.get()); > ShowConstrainedWebDialog( > profile, new ReauthDelegate(), webContents_.get()); > > stops the crashes but doesn't show any dialog. I'm passing the User Manager's > webContent's to ShowConstrainedWebDialog, should I be creating the dialog's web > contents and pass that instead? No, it's the containing contents. I'm wondering if there's some special magic you need to weave on web contents, but I'm not sure - never used this outside of a tab's web contents. > > I'll also try to do the entire ConstrainedWindowCustomWindowMac as you > described, see what happens. That should be fairly straightforward. There's a skeleton implementation of a dialog like that in https://codereview.chromium.org/904613006/, if you need a starting point. > > > > > Question: Do you have any similar dialogs on the UserManager that are already > > implemented for Mac and that you can lean on? > > Unfortunately we don't. :( > > Again, thanks so much for your time, it's really appreciated!
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
On 2015/08/07 17:25:45, groby wrote: > On 2015/08/07 14:27:30, anthonyvd wrote: > > On 2015/08/06 20:27:27, groby wrote: > > > On 2015/08/06 20:07:19, anthonyvd wrote: > > > > > > > > > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > > > > File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > > > > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = > > > [NSApp > > > > beginModalSessionForWindow:[self window]]; > > > > On 2015/08/06 19:49:25, groby wrote: > > > > > On 2015/08/06 19:08:04, anthonyvd wrote: > > > > > > On 2015/08/05 21:16:38, groby wrote: > > > > > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > > > > > (Specifically, via ConstrainedWindow) > > > > > > > > > > > > > > Is there a specific reason why this is introducing an entirely new > UI > > > > mode? > > > > > > > > > > > > To be honest it's only because I'm very unfamiliar with the way we do > > > things > > > > > on > > > > > > mac. Because of this, I based this class on the User Manager itself > > (which > > > > > isn't > > > > > > a ConstrainedWindow) but wanted it to stay on top so that's what I > > figured > > > > > > worked. > > > > > > > > > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd > > > need > > > > a > > > > > > bit of guidance w.r.t doing so. I've tried using > > ShowConstrainedWebDialog: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > > > > > as well as creating the ConstrainedWindowMac manually with no success. > > In > > > > both > > > > > > cases, Chrome crashes when checking (if (delegate_)) in > > > > > > WebContentsModalDialogManager::ShowDialogWithManager. > > > > > > > > > > > > Do you have any guidance as to what I'm doing wrong? > > > > > > > > > > First, thank you for willing to make this change, even though it's a > large > > > > > modification to this CL > > > > > Second: Do you have a CL I can take a look at? Also, happy to have a > quick > > > VC > > > > > chat if you think that'll help > > > > > > > > I don't have a CL but I'll create one from what you described in your next > > > > comment and send it to you if I'm still having issues. :) > > > > > > > > > > > > > > https://codereview.chromium.org/1261433013/diff/60001/chrome/browser/ui/cocoa... > > > > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:163: modalSession_ = > > > [NSApp > > > > beginModalSessionForWindow:[self window]]; > > > > On 2015/08/06 19:49:25, groby wrote: > > > > > On 2015/08/06 19:08:04, anthonyvd wrote: > > > > > > On 2015/08/05 21:16:38, groby wrote: > > > > > > > IIRC, we usually run modal things as sheets, not as a modal session. > > > > > > > (Specifically, via ConstrainedWindow) > > > > > > > > > > > > > > Is there a specific reason why this is introducing an entirely new > UI > > > > mode? > > > > > > > > > > > > To be honest it's only because I'm very unfamiliar with the way we do > > > things > > > > > on > > > > > > mac. Because of this, I based this class on the User Manager itself > > (which > > > > > isn't > > > > > > a ConstrainedWindow) but wanted it to stay on top so that's what I > > figured > > > > > > worked. > > > > > > > > > > > > I'd be happy to change it to use sheets and ConstrainedWindows but I'd > > > need > > > > a > > > > > > bit of guidance w.r.t doing so. I've tried using > > ShowConstrainedWebDialog: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > > > > > as well as creating the ConstrainedWindowMac manually with no success. > > In > > > > both > > > > > > cases, Chrome crashes when checking (if (delegate_)) in > > > > > > WebContentsModalDialogManager::ShowDialogWithManager. > > > > > That seems to indicate a NULL WebContentsDialogManager, which is... odd. > > > > > > > > > > > > > I thought so too :) > > > > > > > > > > > > > > > > Do you have any guidance as to what I'm doing wrong? > > > > > > > > > > General guidance: > > > > > If you create ConstrainedWindowMac manually, AutofillDialogCocoa::Show > > would > > > > be > > > > > the example. (Mostly because I wrote it, and so am familiar with it :) > > > > > > > > > > You'll need a (subclassed) NSWindowController to drive window behavior > and > > > > that > > > > > allocates a ConstrainedWindowCustomWindowMac, you need a > > > > > CustomConstrainedWIndowSheet that's bound to the window controller's > > window, > > > > and > > > > > you need a ConstrainedWindowMac that takes the sheet and the web > contents. > > > > It's > > > > > a bit of an elaborate dance. AutofillSignInContainer shows how to load > an > > > > actual > > > > > web page in there. > > > > > > > > > > Assuming you want to avoid that and stick with ShowConstrainedWebDialog, > > > that > > > > > does take care of this whole dance. As said above, your crash is likely > a > > > bad > > > > > manager, which - first guess - is caused by a bad BrowserContext. What > are > > > you > > > > > using as BrowserContext? > > > > > > > > Thanks a ton for this info, I'll try it out! > > > > > > > > The BrowserContext I'm using is the UserManager's web_ui's BrowserContext. > > If > > > I > > > > read everything correctly, that BrowserContext would actually be the > System > > > > Profile (which could be an issue since it's special in many ways). Maybe > it > > > > doesn't have a WebContentsDialogManager? > > > > > > That would be correct :) Only tabs, extensions windows, and app windows have > > > one, as far as I know. You could always add one with CreateForWebContents, > > > except that might have repercussions - I'm not that familiar with WebUI. > > > > Ah, that makes sense! Unfortunately creating one through CreateForWebContents > > doesn't seem to be enough. Something like: > > > > web_modal::WebContentsModalDialogManager::CreateForWebContents( > > webContents_.get()); > > ShowConstrainedWebDialog( > > profile, new ReauthDelegate(), webContents_.get()); > > > > stops the crashes but doesn't show any dialog. I'm passing the User Manager's > > webContent's to ShowConstrainedWebDialog, should I be creating the dialog's > web > > contents and pass that instead? > > No, it's the containing contents. I'm wondering if there's some special magic > you need to weave on web contents, but I'm not sure - never used this outside of > a tab's web contents. > > > > > I'll also try to do the entire ConstrainedWindowCustomWindowMac as you > > described, see what happens. > > That should be fairly straightforward. There's a skeleton implementation of a > dialog like that in https://codereview.chromium.org/904613006/, if you need a > starting point. > > > > > > > > > Question: Do you have any similar dialogs on the UserManager that are > already > > > implemented for Mac and that you can lean on? > > > > Unfortunately we don't. :( > > > > Again, thanks so much for your time, it's really appreciated! That last patchset should (hopefully) implement the modal dialog properly. groby@: Can you please take a look? (Thanks for helping me out with this by the way :) ) sky@: Can you please double check c/b/ui/* since it changed a little since your LGTM? Thanks!
LGTM
https://codereview.chromium.org/1261433013/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/1261433013/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:355: web_modal::WebContentsModalDialogManager::CreateForWebContents( I think I've explained things badly :( AIUI, you do not really want a modal dialog manager for the WebContents you display - unless you want a set of modal dialogs _on top_ of your web contents. Having a ConstrainedWindowCustomWindow together with the SheetDelegate should be enough to ensure a modal dialog. Or am I missing additional things you're trying to do? If you want to use the WebContentsModalDialogManager, I suggest pinging wittman@, who is more familiar with that particular part. To me, it seems like overkill?
https://codereview.chromium.org/1261433013/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/1261433013/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:355: web_modal::WebContentsModalDialogManager::CreateForWebContents( On 2015/08/11 21:24:50, groby wrote: > I think I've explained things badly :( > > AIUI, you do not really want a modal dialog manager for the WebContents you > display - unless you want a set of modal dialogs _on top_ of your web contents. > > Having a ConstrainedWindowCustomWindow together with the SheetDelegate should be > enough to ensure a modal dialog. > > Or am I missing additional things you're trying to do? > > If you want to use the WebContentsModalDialogManager, I suggest pinging > wittman@, who is more familiar with that particular part. To me, it seems like > overkill? > This Manager is attached to the UserManager's webContents (on top of which I *do* want a set of modal dialogs) though. The web contents I'm displaying inside that reauth modal don't have a Manager themselves. Since the UserManager doesn't have a proper manager, I was under the impression that it was the one I needed to attached a Dialog Manager to, allowing me to display dialogs over it. Isn't that how it's supposed to be?
On 2015/08/11 21:37:29, anthonyvd wrote: > https://codereview.chromium.org/1261433013/diff/160001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): > > https://codereview.chromium.org/1261433013/diff/160001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:355: > web_modal::WebContentsModalDialogManager::CreateForWebContents( > On 2015/08/11 21:24:50, groby wrote: > > I think I've explained things badly :( > > > > AIUI, you do not really want a modal dialog manager for the WebContents you > > display - unless you want a set of modal dialogs _on top_ of your web > contents. > > > > Having a ConstrainedWindowCustomWindow together with the SheetDelegate should > be > > enough to ensure a modal dialog. > > > > Or am I missing additional things you're trying to do? > > > > If you want to use the WebContentsModalDialogManager, I suggest pinging > > wittman@, who is more familiar with that particular part. To me, it seems like > > overkill? > > > > This Manager is attached to the UserManager's webContents (on top of which I > *do* want a set of modal dialogs) though. The web contents I'm displaying inside > that reauth modal don't have a Manager themselves. Ah, I misunderstood that - thank you for clarifying. > > Since the UserManager doesn't have a proper manager, I was under the impression > that it was the one I needed to attached a Dialog Manager to, allowing me to > display dialogs over it. > > Isn't that how it's supposed to be? Yes, it is - I confused things. LGTM
The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261433013/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261433013/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/1261433013/#ps160001 (title: "Use ConstrainedWindow.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261433013/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261433013/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b857f9db847bf0e56eefbf00869bfb98ceb61d2a Cr-Commit-Position: refs/heads/master@{#343110} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
