Tab-modal dialog improvements:...

Tab-modal dialog improvements:
- treat constrained dialogs as tab-modal - only one shows at the time
- added visual indication (tab pulsing) to the tab strip when a tab is blocked by a tab-modal dialog
- blocked all UI activity from rendrer host and forced refocusing on constrained (tab-modal) dialogs

This CL reverts http://codereview.chromium.org/384113 and instead incorporates the changes from http://codereview.chromium.org/392018.


BUG=456, 27585, 27620
TEST=Go to http://www/~thakis/cgi-bin/test.html, hit esc.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=36415

[Nico, 2010-01-14 17:51:18]

A few questions below. The most interesting one is the one about "delayed
sheets".

Elliot suggested that ConstrainedWindow should be renamed to something else
(TabDialog?), given that it does something very different from what it used to
do when it was written. (Don't do this in this CL though.)

Once JS dialogs use per-tab dialogs, what's the plan if one render process
renders the contents of several tabs and one of the tabs opens a dialog while
another of these tabs is active? How will you show to the user that his current
tab is blocked on a dialog in another tab? Will the blocking tab autofocus? What
if the user manually switches back to their original tab without dismissing the
dialog?

Also, please at least add a todo to the mac code that it needs to glow when it's
blocked.


Elliot: Maybe you want to have a look too.
Trung: FYI.

http://codereview.chromium.org/541056/diff/5038/4069
File chrome/browser/cocoa/browser_window_controller.h (left):

http://codereview.chromium.org/541056/diff/5038/4069#oldcode215
chrome/browser/cocoa/browser_window_controller.h:215: // disappears. The window
should then call |attachConstrainedWindow:| again.
Keep the part of the comment that's still true.

http://codereview.chromium.org/541056/diff/5038/4069#oldcode219
chrome/browser/cocoa/browser_window_controller.h:219: // tab's sheet queue.
likewise

http://codereview.chromium.org/541056/diff/5038/4071
File chrome/browser/cocoa/constrained_window_mac.mm (right):

http://codereview.chromium.org/541056/diff/5038/4071#newcode94
chrome/browser/cocoa/constrained_window_mac.mm:94:
delegate_->set_sheet_open(true);
Say you have a server that sleeps for 4 seconds or so before sending back an
http auth header. If you navigate to that server and then switch tabs before the
auth response arrives, does this work? The sheet should only appear when you
switch back to that tab. That's why the "SetVisible()" callback was here iirc.

http://codereview.chromium.org/541056/diff/5038/4048
File chrome/browser/login_prompt_win.cc (left):

http://codereview.chromium.org/541056/diff/5038/4048#oldcode52
chrome/browser/login_prompt_win.cc:52: // views::DialogDelegate methods:
what's wrong with this comment?

http://codereview.chromium.org/541056/diff/5038/4076
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/541056/diff/5038/4076#newcode1665
chrome/browser/renderer_host/render_view_host.cc:1665: }
what about key events?

http://codereview.chromium.org/541056/diff/5038/4075
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/541056/diff/5038/4075#newcode540
chrome/browser/renderer_host/render_view_host_delegate.h:540: // Notification
from the renderer host that blocked UI event occured.
"This happens for example if ... and is needed because ...".

http://codereview.chromium.org/541056/diff/5038/4053
File chrome/browser/tab_contents/constrained_window.h (right):

http://codereview.chromium.org/541056/diff/5038/4053#newcode49
chrome/browser/tab_contents/constrained_window.h:49: virtual void
FocusConstrainedWindow() {}
needs comment. does it make the tab that contains the constrained window active,
or does it only move the focus to the window if it's tab is already active,
or...?

http://codereview.chromium.org/541056/diff/5038/4050
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/541056/diff/5038/4050#newcode1565
chrome/browser/tab_contents/tab_contents.cc:1565: 
remove newline

http://codereview.chromium.org/541056/diff/5038/4040
File views/view.cc (right):

http://codereview.chromium.org/541056/diff/5038/4040#newcode700
views/view.cc:700: UnregisterAccelerators(true);
I'm not familiar at all with the views code, but why did you have to change
this? Maybe add a comment to what the bool means and why it has to be the way it
is.

[Ben Goodger (Google), 2010-01-14 18:48:26]

Adding erg for GTK bits
Adding jcampan for focus bits in view.cc.

http://codereview.chromium.org/541056/diff/5038/4066
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/541056/diff/5038/4066#newcode354
chrome/browser/renderer_host/render_widget_host.h:354: bool
ignore_input_events() {
const

http://codereview.chromium.org/541056/diff/5038/4066#newcode561
chrome/browser/renderer_host/render_widget_host.h:561: bool
ignore_input_events_;
Note what this is for.

http://codereview.chromium.org/541056/diff/5038/4053
File chrome/browser/tab_contents/constrained_window.h (right):

http://codereview.chromium.org/541056/diff/5038/4053#newcode43
chrome/browser/tab_contents/constrained_window.h:43: // at a time per tab.
I think this could be a weakness down the line. It's not always a bad thing to
show multiple windows - what if one constrained window spawns another (e.g.
sub-dialogs)? I don't think we necessarily need to solve this now given that we
don't have UI to do this, but you might call out that this is a limitation we
should resolve somehow.

http://codereview.chromium.org/541056/diff/5038/4050
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/541056/diff/5038/4050#newcode335
chrome/browser/tab_contents/tab_contents.cc:335: BlockTabContent(false);
Yeah, I'm uneasy with this sort of blanket change. I think it works well enough
for now, but I'd rather we only unilaterally block if we know the constrained
dialog wants to be modal. I think you could easily wire up a "IsModal" method on
the ConstrainedWindow interface... on Windows the implementation of this method
would query the underlying ConstrainedWindow's WindowDelegate::IsModal method.
That way constrained dialogs would implement WindowDelegate::IsModal like any
other top level window and receive the appropriate "modality" given how they're
instantiated.

http://codereview.chromium.org/541056/diff/5038/4049
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/541056/diff/5038/4049#newcode186
chrome/browser/tab_contents/tab_contents_delegate.h:186: // Changes the blocked
state of the tab at |index|.
You might note what "blocked" means, since this appears to be the canonical
header for this function.

http://codereview.chromium.org/541056/diff/5038/4047
File chrome/browser/views/constrained_window_win.cc (right):

http://codereview.chromium.org/541056/diff/5038/4047#newcode596
chrome/browser/views/constrained_window_win.cc:596:
focused_view_->RequestFocus();
be sure to ask jcampan if there are any focus/activation related tests
(automated or manual) you should run.

http://codereview.chromium.org/541056/diff/5038/4042
File chrome/browser/views/constrained_window_win.h (right):

http://codereview.chromium.org/541056/diff/5038/4042#newcode73
chrome/browser/views/constrained_window_win.h:73: views::View* focused_view_;
???

http://codereview.chromium.org/541056/diff/5038/4074
File chrome/browser/views/login_view.cc (right):

http://codereview.chromium.org/541056/diff/5038/4074#newcode110
chrome/browser/views/login_view.cc:110: void LoginView::RequestFocus() {
OCD nit: make sure the order in CC file matches .h

http://codereview.chromium.org/541056/diff/5038/4044
File chrome/browser/views/tabs/tab_renderer.cc (right):

http://codereview.chromium.org/541056/diff/5038/4044#newcode897
chrome/browser/views/tabs/tab_renderer.cc:897: void
TabRenderer::set_blocked(bool blocked) {
You can't name a function like this and have it do this much work. Use
SetBlocked instead.

http://codereview.chromium.org/541056/diff/5038/4040
File views/view.cc (right):

http://codereview.chromium.org/541056/diff/5038/4040#newcode700
views/view.cc:700: UnregisterAccelerators(true);
jcampan should review this.

[zel, 2010-01-14 23:05:16]

http://codereview.chromium.org/541056/diff/5038/4069
File chrome/browser/cocoa/browser_window_controller.h (left):

http://codereview.chromium.org/541056/diff/5038/4069#oldcode215
chrome/browser/cocoa/browser_window_controller.h:215: // disappears. The window
should then call |attachConstrainedWindow:| again.
On 2010/01/14 17:51:19, Nico wrote:
> Keep the part of the comment that's still true.

Done.

http://codereview.chromium.org/541056/diff/5038/4069#oldcode219
chrome/browser/cocoa/browser_window_controller.h:219: // tab's sheet queue.
On 2010/01/14 17:51:19, Nico wrote:
> likewise

Done.

http://codereview.chromium.org/541056/diff/5038/4071
File chrome/browser/cocoa/constrained_window_mac.mm (right):

http://codereview.chromium.org/541056/diff/5038/4071#newcode94
chrome/browser/cocoa/constrained_window_mac.mm:94:
delegate_->set_sheet_open(true);
On 2010/01/14 17:51:19, Nico wrote:
> Say you have a server that sleeps for 4 seconds or so before sending back an
> http auth header. If you navigate to that server and then switch tabs before
the
> auth response arrives, does this work? The sheet should only appear when you
> switch back to that tab. That's why the "SetVisible()" callback was here iirc.

These change is done as a part of reverting (per your suggestion) CL
http://codereview.chromium.org/384113. Let me know if something else is more
appropriate here.

http://codereview.chromium.org/541056/diff/5038/4048
File chrome/browser/login_prompt_win.cc (left):

http://codereview.chromium.org/541056/diff/5038/4048#oldcode52
chrome/browser/login_prompt_win.cc:52: // views::DialogDelegate methods:
On 2010/01/14 17:51:19, Nico wrote:
> what's wrong with this comment?

Nuked by accident I guess. It's back now.

http://codereview.chromium.org/541056/diff/5038/4076
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/541056/diff/5038/4076#newcode1665
chrome/browser/renderer_host/render_view_host.cc:1665: }
On 2010/01/14 17:51:19, Nico wrote:
> what about key events?

Done.

http://codereview.chromium.org/541056/diff/5038/4075
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/541056/diff/5038/4075#newcode540
chrome/browser/renderer_host/render_view_host_delegate.h:540: // Notification
from the renderer host that blocked UI event occured.
On 2010/01/14 17:51:19, Nico wrote:
> "This happens for example if ... and is needed because ...".

Done.

http://codereview.chromium.org/541056/diff/5038/4066
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/541056/diff/5038/4066#newcode561
chrome/browser/renderer_host/render_widget_host.h:561: bool
ignore_input_events_;
On 2010/01/14 18:48:26, Ben Goodger wrote:
> Note what this is for.

Done.

http://codereview.chromium.org/541056/diff/5038/4053
File chrome/browser/tab_contents/constrained_window.h (right):

http://codereview.chromium.org/541056/diff/5038/4053#newcode43
chrome/browser/tab_contents/constrained_window.h:43: // at a time per tab.
On 2010/01/14 18:48:26, Ben Goodger wrote:
> I think this could be a weakness down the line. It's not always a bad thing to
> show multiple windows - what if one constrained window spawns another (e.g.
> sub-dialogs)? I don't think we necessarily need to solve this now given that
we
> don't have UI to do this, but you might call out that this is a limitation we
> should resolve somehow.

Based on our discussion, this will be addressed in a future CL. We also need to
see how that will look on GTK and Mac since one can't move around such dialogs
now.

http://codereview.chromium.org/541056/diff/5038/4053#newcode49
chrome/browser/tab_contents/constrained_window.h:49: virtual void
FocusConstrainedWindow() {}
On 2010/01/14 17:51:19, Nico wrote:
> needs comment. does it make the tab that contains the constrained window
active,
> or does it only move the focus to the window if it's tab is already active,
> or...?

Done.

http://codereview.chromium.org/541056/diff/5038/4049
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/541056/diff/5038/4049#newcode186
chrome/browser/tab_contents/tab_contents_delegate.h:186: // Changes the blocked
state of the tab at |index|.
On 2010/01/14 18:48:26, Ben Goodger wrote:
> You might note what "blocked" means, since this appears to be the canonical
> header for this function.

Done.

http://codereview.chromium.org/541056/diff/5038/4042
File chrome/browser/views/constrained_window_win.h (right):

http://codereview.chromium.org/541056/diff/5038/4042#newcode73
chrome/browser/views/constrained_window_win.h:73: views::View* focused_view_;
On 2010/01/14 18:48:26, Ben Goodger wrote:
> ???

We need to know on which view needs to run RequestFocus() when we want to focus
the ConstrainedWindow again. See .cc part of this change for details.

http://codereview.chromium.org/541056/diff/5038/4074
File chrome/browser/views/login_view.cc (right):

http://codereview.chromium.org/541056/diff/5038/4074#newcode110
chrome/browser/views/login_view.cc:110: void LoginView::RequestFocus() {
On 2010/01/14 18:48:26, Ben Goodger wrote:
> OCD nit: make sure the order in CC file matches .h

Done.

http://codereview.chromium.org/541056/diff/5038/4044
File chrome/browser/views/tabs/tab_renderer.cc (right):

http://codereview.chromium.org/541056/diff/5038/4044#newcode897
chrome/browser/views/tabs/tab_renderer.cc:897: void
TabRenderer::set_blocked(bool blocked) {
On 2010/01/14 18:48:26, Ben Goodger wrote:
> You can't name a function like this and have it do this much work. Use
> SetBlocked instead.

Done.

http://codereview.chromium.org/541056/diff/5038/4040
File views/view.cc (right):

http://codereview.chromium.org/541056/diff/5038/4040#newcode700
views/view.cc:700: UnregisterAccelerators(true);
On 2010/01/14 17:51:19, Nico wrote:
> I'm not familiar at all with the views code, but why did you have to change
> this? Maybe add a comment to what the bool means and why it has to be the way
it
> is.

Nico, this change took care for the crash that you have seen on Windows in your
original CL. I added comments why this change is there to begin with.

[Ben Goodger (Google), 2010-01-14 23:23:19]

On Thu, Jan 14, 2010 at 3:05 PM,  <zelidrag@chromium.org> wrote:
> Based on our discussion, this will be addressed in a future CL.

Can you file a bug and link it somewhere central, like the
tab_contents.h next to the method that shows the dialogs?

-Ben

[Ben Goodger (Google), 2010-01-14 23:23:42]

LGTM with the bug URL added to the comment btw.

On Thu, Jan 14, 2010 at 3:23 PM, Ben Goodger (Google) <ben@chromium.org> wrote:
> On Thu, Jan 14, 2010 at 3:05 PM,  <zelidrag@chromium.org> wrote:
>> Based on our discussion, this will be addressed in a future CL.
>
> Can you file a bug and link it somewhere central, like the
> tab_contents.h next to the method that shows the dialogs?
>
> -Ben
>

[jcampan, 2010-01-14 23:34:20]

http://codereview.chromium.org/541056/diff/5038/4040
File views/view.cc (right):

http://codereview.chromium.org/541056/diff/5038/4040#newcode700
views/view.cc:700: UnregisterAccelerators(true);
On 2010/01/14 23:05:16, zel wrote:
> On 2010/01/14 17:51:19, Nico wrote:
> > I'm not familiar at all with the views code, but why did you have to change
> > this? Maybe add a comment to what the bool means and why it has to be the
way
> it
> > is.
> 
> Nico, this change took care for the crash that you have seen on Windows in
your
> original CL. I added comments why this change is there to begin with.
> 
I saw your comment but I don't understand how this was causing a crasher, nor
exactly what the comment exactly means.

[Elliot Glaysher, 2010-01-14 23:38:44]

OK on the GTK parts

[Nico, 2010-01-15 01:03:17]

LG from me with comments addressed.

Please mention in the CL description that this reverts
http://codereview.chromium.org/384113 instead incorporates the changes from
http://codereview.chromium.org/392018 (and then some).

TEST= shouldn't be "N/A" but how one would manually check that stuff still works
as far as I understand. Copy what the two CLs above have and / or put the repro
steps from that linux esc dialog.

File a bug each to add the glowing for linux and os x, add the linux bug nr to
the linux TODO for this and add a os x TODO with the mac bug to the right place
(somewhere in chrome/browser/cocoa/tab_view.mm , search for "glow").

And wait for what jcampan has to say too :-)

http://codereview.chromium.org/541056/diff/10001/9029
File chrome/browser/cocoa/tab_strip_controller.h (left):

http://codereview.chromium.org/541056/diff/10001/9029#oldcode196
chrome/browser/cocoa/tab_strip_controller.h:196: // functions.
keep this comment

http://codereview.chromium.org/541056/diff/10001/9038
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/541056/diff/10001/9038#newcode541
chrome/browser/renderer_host/render_view_host_delegate.h:541: // This happens
when are displaying tab-modal dialogs. In this case, the
s/when are/when there are/

http://codereview.chromium.org/541056/diff/10001/9012
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/541056/diff/10001/9012#newcode1565
chrome/browser/tab_contents/tab_contents.cc:1565: 
remove newline

[zelidrag1, 2010-01-15 01:39:35]

Jay,

Without this change in views.cc, I have reached following asset there (see
the stack dump below).

Basically, View::ViewHierarchyChangedImpl() would nuke accelerators with
UnregisterAccelerators(false) call and then the rest of the execution flow
would still expect that they exist what results in assert in
View::RemoveAccelerator():1008.




 chrome.dll!views::View::RemoveAccelerator(const views::Accelerator &
accelerator={...})  Line 1008C++
 chrome.dll!views::NativeButton::SetIsDefault(bool is_default=false)  Line
87 + 0x23 bytesC++
 chrome.dll!views::DialogClientView::SetDefaultButton(views::NativeButton *
new_default_button=0x05a93360)  Line 176C++
 chrome.dll!views::DialogClientView::FocusWillChange(views::View *
focused_before=0x05a93480, views::View * focused_now=0x00000000)  Line
202C++
 chrome.dll!views::FocusManager::SetFocusedView(views::View *
view=0x00000000)  Line 241 + 0x23 bytesC++
 chrome.dll!views::RootView::FocusView(views::View * view=0x00000000)  Line
526C++
 chrome.dll!views::RootView::ViewHierarchyChanged(bool is_add=false,
views::View * parent=0x00d93e00, views::View * child=0x05a93480)  Line
267C++
 chrome.dll!views::View::ViewHierarchyChangedImpl(bool
register_accelerators=true, bool is_add=false, views::View *
parent=0x00d93e00, views::View * child=0x05a93480)  Line 704 + 0x1f bytesC++
 chrome.dll!views::View::PropagateRemoveNotifications(views::View *
parent=0x00d93e00)  Line 641 + 0x14 bytesC++
 chrome.dll!views::View::PropagateRemoveNotifications(views::View *
parent=0x00d93e00)  Line 638 + 0x17 bytesC++
 chrome.dll!views::View::PropagateRemoveNotifications(views::View *
parent=0x00d93e00)  Line 638 + 0x17 bytesC++
 chrome.dll!views::View::DoRemoveChildView(views::View * a_view=0x04fce780,
bool update_focus_cycle=false, bool update_tool_tip=false, bool
delete_removed_view=true)  Line 621C++
 chrome.dll!views::View::RemoveAllChildViews(bool delete_views=true)  Line
575C++
 chrome.dll!views::RootView::~RootView()  Line 87C++
 chrome.dll!views::RootView::`scalar deleting destructor'()  + 0x16 bytesC++
 chrome.dll!scoped_ptr<views::RootView>::~scoped_ptr<views::RootView>()
 Line 72 + 0x25 bytesC++
 chrome.dll!views::WidgetWin::~WidgetWin()  Line 63 + 0x37 bytesC++
 chrome.dll!views::WindowWin::~WindowWin()  Line 103 + 0x48 bytesC++
 chrome.dll!ConstrainedWindowWin::~ConstrainedWindowWin()  Line 586 + 0x27
bytesC++
 chrome.dll!ConstrainedWindowWin::`scalar deleting destructor'()  + 0x16
bytesC++
 chrome.dll!views::WidgetWin::OnFinalMessage(HWND__ * window=0x001604e6)
 Line 834 + 0x23 bytesC++
 chrome.dll!views::WindowWin::OnFinalMessage(HWND__ * window=0x001604e6)
 Line 658C++
 chrome.dll!ConstrainedWindowWin::OnFinalMessage(HWND__ * window=0x001604e6)
 Line 671C++
 chrome.dll!views::WidgetWin::OnWndProc(unsigned int message=130, unsigned
int w_param=0, long l_param=0)  Line 1126 + 0x16 bytesC++
 chrome.dll!app::WindowImpl::WndProc(HWND__ * hwnd=0x001604e6, unsigned int
message=130, unsigned int w_param=0, long l_param=0)  Line 183 + 0x1b
bytesC++





On Thu, Jan 14, 2010 at 3:34 PM, <jcampan@chromium.org> wrote:

>
> http://codereview.chromium.org/541056/diff/5038/4040
> File views/view.cc (right):
>
> http://codereview.chromium.org/541056/diff/5038/4040#newcode700
> views/view.cc:700: UnregisterAccelerators(true);
> On 2010/01/14 23:05:16, zel wrote:
>
>> On 2010/01/14 17:51:19, Nico wrote:
>> > I'm not familiar at all with the views code, but why did you have to
>>
> change
>
>> > this? Maybe add a comment to what the bool means and why it has to
>>
> be the way
>
>> it
>> > is.
>>
>
>  Nico, this change took care for the crash that you have seen on
>>
> Windows in your
>
>> original CL. I added comments why this change is there to begin with.
>>
>
> I saw your comment but I don't understand how this was causing a
> crasher, nor exactly what the comment exactly means.
>
>
> http://codereview.chromium.org/541056
>

[zel, 2010-01-15 03:25:04]

http://codereview.chromium.org/541056/diff/10001/9029
File chrome/browser/cocoa/tab_strip_controller.h (left):

http://codereview.chromium.org/541056/diff/10001/9029#oldcode196
chrome/browser/cocoa/tab_strip_controller.h:196: // functions.
On 2010/01/15 01:03:18, Nico wrote:
> keep this comment

Done.

http://codereview.chromium.org/541056/diff/10001/9038
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/541056/diff/10001/9038#newcode541
chrome/browser/renderer_host/render_view_host_delegate.h:541: // This happens
when are displaying tab-modal dialogs. In this case, the
On 2010/01/15 01:03:18, Nico wrote:
> s/when are/when there are/

Done.

http://codereview.chromium.org/541056/diff/10001/9012
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/541056/diff/10001/9012#newcode1565
chrome/browser/tab_contents/tab_contents.cc:1565: 
On 2010/01/15 01:03:18, Nico wrote:
> remove newline

Done.

[jcampan, 2010-01-15 17:18:40]

OK, makes sense. Thanks for the explanation.

LGTM

On Thu, Jan 14, 2010 at 5:39 PM, Zelidrag Hornung <zelidrag@google.com> wrote:
> Jay,
> Without this change in views.cc, I have reached following asset there (see
> the stack dump below).
> Basically, View::ViewHierarchyChangedImpl() would nuke accelerators with
> UnregisterAccelerators(false) call and then the rest of the execution flow
> would still expect that they exist what results in assert in
> View::RemoveAccelerator():1008.
>
>
>
>  chrome.dll!views::View::RemoveAccelerator(const views::Accelerator &
> accelerator={...})  Line 1008C++
>  chrome.dll!views::NativeButton::SetIsDefault(bool is_default=false)  Line
> 87 + 0x23 bytesC++
>  chrome.dll!views::DialogClientView::SetDefaultButton(views::NativeButton *
> new_default_button=0x05a93360)  Line 176C++
>  chrome.dll!views::DialogClientView::FocusWillChange(views::View *
> focused_before=0x05a93480, views::View * focused_now=0x00000000)  Line
> 202C++
>  chrome.dll!views::FocusManager::SetFocusedView(views::View *
> view=0x00000000)  Line 241 + 0x23 bytesC++
>  chrome.dll!views::RootView::FocusView(views::View * view=0x00000000)  Line
> 526C++
>  chrome.dll!views::RootView::ViewHierarchyChanged(bool is_add=false,
> views::View * parent=0x00d93e00, views::View * child=0x05a93480)  Line
> 267C++
>  chrome.dll!views::View::ViewHierarchyChangedImpl(bool
> register_accelerators=true, bool is_add=false, views::View *
> parent=0x00d93e00, views::View * child=0x05a93480)  Line 704 + 0x1f bytesC++
>  chrome.dll!views::View::PropagateRemoveNotifications(views::View *
> parent=0x00d93e00)  Line 641 + 0x14 bytesC++
>  chrome.dll!views::View::PropagateRemoveNotifications(views::View *
> parent=0x00d93e00)  Line 638 + 0x17 bytesC++
>  chrome.dll!views::View::PropagateRemoveNotifications(views::View *
> parent=0x00d93e00)  Line 638 + 0x17 bytesC++
>  chrome.dll!views::View::DoRemoveChildView(views::View * a_view=0x04fce780,
> bool update_focus_cycle=false, bool update_tool_tip=false, bool
> delete_removed_view=true)  Line 621C++
>  chrome.dll!views::View::RemoveAllChildViews(bool delete_views=true)  Line
> 575C++
>  chrome.dll!views::RootView::~RootView()  Line 87C++
>  chrome.dll!views::RootView::`scalar deleting destructor'()  + 0x16 bytesC++
>  chrome.dll!scoped_ptr<views::RootView>::~scoped_ptr<views::RootView>()
>  Line 72 + 0x25 bytesC++
>  chrome.dll!views::WidgetWin::~WidgetWin()  Line 63 + 0x37 bytesC++
>  chrome.dll!views::WindowWin::~WindowWin()  Line 103 + 0x48 bytesC++
>  chrome.dll!ConstrainedWindowWin::~ConstrainedWindowWin()  Line 586 + 0x27
> bytesC++
>  chrome.dll!ConstrainedWindowWin::`scalar deleting destructor'()  + 0x16
> bytesC++
>  chrome.dll!views::WidgetWin::OnFinalMessage(HWND__ * window=0x001604e6)
>  Line 834 + 0x23 bytesC++
>  chrome.dll!views::WindowWin::OnFinalMessage(HWND__ * window=0x001604e6)
>  Line 658C++
>  chrome.dll!ConstrainedWindowWin::OnFinalMessage(HWND__ * window=0x001604e6)
>  Line 671C++
>  chrome.dll!views::WidgetWin::OnWndProc(unsigned int message=130, unsigned
> int w_param=0, long l_param=0)  Line 1126 + 0x16 bytesC++
>  chrome.dll!app::WindowImpl::WndProc(HWND__ * hwnd=0x001604e6, unsigned int
> message=130, unsigned int w_param=0, long l_param=0)  Line 183 + 0x1b
> bytesC++
>
>
>
>
> On Thu, Jan 14, 2010 at 3:34 PM, <jcampan@chromium.org> wrote:
>>
>> http://codereview.chromium.org/541056/diff/5038/4040
>> File views/view.cc (right):
>>
>> http://codereview.chromium.org/541056/diff/5038/4040#newcode700
>> views/view.cc:700: UnregisterAccelerators(true);
>> On 2010/01/14 23:05:16, zel wrote:
>>>
>>> On 2010/01/14 17:51:19, Nico wrote:
>>> > I'm not familiar at all with the views code, but why did you have to
>>
>> change
>>>
>>> > this? Maybe add a comment to what the bool means and why it has to
>>
>> be the way
>>>
>>> it
>>> > is.
>>
>>> Nico, this change took care for the crash that you have seen on
>>
>> Windows in your
>>>
>>> original CL. I added comments why this change is there to begin with.
>>
>> I saw your comment but I don't understand how this was causing a
>> crasher, nor exactly what the comment exactly means.
>>
>> http://codereview.chromium.org/541056
>
>