Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(14)

Issue 7189019: Fix even more crashes. To help identify remaining crashes now and in the future, I have made the ... (Closed)

Created:
8 years, 1 month ago by Ben Goodger (Google)
Modified:
8 years, 1 month ago
Reviewers:
Nico, sky
CC:
chromium-reviews, Paweł Hajdan Jr., dhollowa
Visibility:
Public.

Description

Fix even more crashes. To help identify remaining crashes now and in the future, I have made the GetWidget methods on WidgetDelegate pure virtual. This will cause classes that don't define them to fail compile instead of crashing at run time. http://crbug.com/86119 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89409

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+123 lines, -48 lines) Patch
M chrome/browser/chromeos/login/user_controller.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/login/user_controller.cc View 1 2 2 chunks +15 lines, -0 lines 0 comments Download
M chrome/browser/ui/login/login_prompt_win.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/constrained_html_delegate_win.cc View 2 chunks +14 lines, -8 lines 0 comments Download
M chrome/browser/ui/views/default_search_view.h View 1 chunk +10 lines, -7 lines 1 comment Download
M chrome/browser/ui/views/default_search_view.cc View 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/repost_form_warning_view.h View 1 chunk +9 lines, -7 lines 0 comments Download
M chrome/browser/ui/views/repost_form_warning_view.cc View 1 chunk +8 lines, -0 lines 0 comments Download
M views/controls/tabbed_pane/tabbed_pane_unittest.cc View 1 chunk +9 lines, -3 lines 0 comments Download
M views/controls/table/table_view_unittest.cc View 2 chunks +16 lines, -4 lines 0 comments Download
M views/desktop/desktop_window.h View 1 chunk +1 line, -2 lines 0 comments Download
M views/desktop/desktop_window.cc View 1 chunk +1 line, -2 lines 0 comments Download
M views/focus/focus_manager_unittest.cc View 2 chunks +9 lines, -3 lines 0 comments Download
M views/view_unittest.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M views/widget/widget.cc View 1 2 chunks +10 lines, -2 lines 0 comments Download
M views/widget/widget_delegate.h View 1 chunk +2 lines, -2 lines 0 comments Download
M views/widget/widget_delegate.cc View 1 chunk +0 lines, -8 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
Ben Goodger (Google)
8 years, 1 month ago (2011-06-16 21:38:47 UTC) #1
sky
LGTM
8 years, 1 month ago (2011-06-16 21:42:07 UTC) #2
Nico
http://codereview.chromium.org/7189019/diff/21/chrome/browser/ui/views/default_search_view.h File chrome/browser/ui/views/default_search_view.h (right): http://codereview.chromium.org/7189019/diff/21/chrome/browser/ui/views/default_search_view.h#newcode65 chrome/browser/ui/views/default_search_view.h:65: virtual const views::Widget* GetWidget() const OVERRIDE; Hi Ben, you ...
8 years, 1 month ago (2011-06-17 03:48:15 UTC) #3
Ben Goodger (Google)
8 years, 1 month ago (2011-06-17 04:07:38 UTC) #4
I figured it was related to the typedef, thanks for confirming my suspicion.
It's on my list to clear all this up at some point.

-Ben

On Thu, Jun 16, 2011 at 8:48 PM, <thakis@chromium.org> wrote:

>
> http://codereview.chromium.**org/7189019/diff/21/chrome/**
>
browser/ui/views/default_**search_view.h<http://codereview.chromium.org/7189019/diff/21/chrome/browser/ui/views/default_search_view.h>
> File chrome/browser/ui/views/**default_search_view.h (right):
>
> http://codereview.chromium.**org/7189019/diff/21/chrome/**
>
browser/ui/views/default_**search_view.h#newcode65<http://codereview.chromium.org/7189019/diff/21/chrome/browser/ui/views/default_search_view.h#newcode65>
> chrome/browser/ui/views/**default_search_view.h:65: virtual const
> views::Widget* GetWidget() const OVERRIDE;
> Hi Ben,
>
> you added a "TODO: Find out why clang doesn't like these OVERRIDEs in
> http://src.chromium.org/**viewvc/chrome/trunk/src/**
> chrome/browser/ui/views/**default_search_view.h?r1=**
>
89411&r2=89410&pathrev=89411<http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/default_search_view.h?r1=89411&r2=89410&pathrev=89411>
> ".
>
> The reason is that ConstrainedDialogDelegate is only a typef for
> views::DialogDelegate – on CrOS, it's a typedef for
> ConstrainedWindowGtkDelegate instead (see
> http://codesearch.google.com/**codesearch#OAMlx_jo-ck/src/**
> content/browser/tab_contents/**constrained_window.h&exact_**
>
package=chromium&ct=rc&cd=1&q=**ConstrainedDialogDelegate<http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/tab_contents/constrained_window.h&exact_package=chromium&ct=rc&cd=1&q=ConstrainedDialogDelegate>
> ). That seems at best confusing and at worst bug-introducing, so it
> should maybe be changed. I don't know this code well enough to know what
> the right thing is.
>
> (ConstrainedWindowGtkDelegate is here:
> http://codesearch.google.com/**codesearch#OAMlx_jo-ck/src/**
>
chrome/browser/ui/gtk/**constrained_window_gtk.h&l=27<http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/gtk/constrained_window_gtk.h&l=27>
> I'm not sure why this compiles as is; I guess on ChromeOS nothing tries
> to create an instance of DefaultSearchView.)
>
>
>
http://codereview.chromium.**org/7189019/<http://codereview.chromium.org/7189...
>

Powered by Google App Engine
This is Rietveld 408576698