Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2261793002:
Bring the feedback button to the Mac sad tab (Closed)
|
Created:
4 years, 4 months ago by Sidney San Martín Modified:
4 years, 3 months ago CC:
chromium-reviews, tfarina, asvitkine+watch_chromium.org, Will Harris Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBring the feedback button to the Mac sad tab
The Views implementation of SadTab supports two big things that were
missing in the Mac implementation:
1. The reload button turns into a feedback button on repeated crashes.
2. Histograms for tracking different kinds of crashes.
This moves that logic up into the base SadTab, so that both
implementations can use it.
It also adds new histograms to count when a sad tab with a reload button
or feedback button is displayed, and clicks on the button or help link.
Other changes:
- HISTOGRAM_ENUMERATION_WITH_FLAG uses a new base::underlying_value()
function to cast scoped enums to their underlying integral values.
(Unlike classic enums, they don't implicitly convert).
- The Show() and Close() methods of SadTab are gone. Show() was always
called right after construction, and Close() was always called right
before destruction, so I merged them with the ctor+dtor.
- SadTabController is gone.
- SadTabView no longer gets a reference to the WebContents; SadTabCocoa
is now responsible for sizing it and adding it to the view hierarchy.
- The help link text no longer selects when you right click it.
BUG=623690
TEST=Trigger sad tabs on each platform either with chrome://crash or by
killing processes in the task manager. Check that the first sad tab
shows a reload button and subsequent sad tabs show a feedback button.
Otherwise, the look and behavior of the sad tab should be the same.
Committed: https://crrev.com/26cabf35396e65533b30062b89aee46cbf55b862
Cr-Commit-Position: refs/heads/master@{#417606}
Patch Set 1 : Patch Set 1 (Needs histogram descriptions) #Patch Set 2 : Fix some Views build issues #Patch Set 3 : Refactor to elide SadTabController, tidy SadTabView #Patch Set 4 : Add real summaries for the new histograms #Patch Set 5 : Tweak CL similarity threshold to get rid of some false renames. #Patch Set 6 : CL cleanup #
Total comments: 12
Patch Set 7 : Make a cast which only takes enums and numbers, fix nits #
Total comments: 2
Patch Set 8 : underlying_number -> underlying_value for better error messages #
Total comments: 4
Patch Set 9 : Rebase, make switches exhaustive #Patch Set 10 : clang considers this unreachable, but not gcc #Patch Set 11 : Trybots caught this, but not the local presubmit check. Will investigate. #
Total comments: 7
Patch Set 12 : Ditch a commented-out ctor, CHECK(false) -> NOTREACHED(); return #Patch Set 13 : Move this #undef back where it belongs. #Patch Set 14 : Clean up some comments, replace DLOG_ASSERT with DCHECK, move an #include to the right spot. #
Total comments: 4
Patch Set 15 : Ditch a pointless |explicit|, add a comment on SadTabView ownership. #
Total comments: 43
Patch Set 16 : Rebase #Patch Set 17 : Address some comments #Patch Set 18 : No new needed #Patch Set 19 : Address second wave of comments, turn off feedback in non-Chrome builds #Messages
Total messages: 76 (45 generated)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Minor changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - Mac SadTabView no longer gets a reference to the WebContents; SadTabController is now responsible for sizing it and adding it to the view hierarchy, as it should be. - Refactoring. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ========== to ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Minor changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - Mac SadTabView no longer gets a reference to the WebContents; SadTabController is now responsible for sizing it and adding it to the view hierarchy, as it should be. - Refactoring. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ==========
sdy@chromium.org changed reviewers: + isherman@chromium.org, sky@chromium.org
sdy@chromium.org changed required reviewers: + isherman@chromium.org, sky@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Minor changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - Mac SadTabView no longer gets a reference to the WebContents; SadTabController is now responsible for sizing it and adding it to the view hierarchy, as it should be. - Refactoring. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ========== to ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy, as it should be. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ==========
Description was changed from ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy, as it should be. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ========== to ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ==========
Description was changed from ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ========== to ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ==========
Description was changed from ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ========== to ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. - The help link text no longer selects when you right click it. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ==========
Have at it, and thanks! I'm curious how you think removing SadTabController worked out. https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:174: chrome::ShowFeedbackPage( ShowFeedbackPage looks like a no-op on Chromium builds. What should I do about that — only enable the feedback state for Chrome? It looks like there's no Help menu in Chromium builds, either, I'll look at how that's done.
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2261793002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2261793002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:156: static_cast<uint32_t>(boundary) + 1, flag)) I'm not comfortable with adding a blanket static_cast to the macro, as it might hide legitimate compilation warnings/errors as well as just the enum class ones. Is there a way to more specifically target just enum classes? https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:5: #include "base/metrics/histogram.h" nit: histogram_macros https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:36: UMA_HISTOGRAM_COUNTS_1000("Tabs.SadTab." NAME, ++count); \ nit: Please move the increment statement to a separate line, so that readers don't need to think about pre- vs post-increment semantics. https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:39: enum class SadTabEvent { Please document that this enum is used to back an UMA histogram, and should therefore be treated as append-only. https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:47: if (feedback) { What determines whether the feedback style or reload style of sad tab is shown? If it's a field trial (aka Finch experiment), then I'd recommend having a single histogram rather than two separate ones.
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2261793002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2261793002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:156: static_cast<uint32_t>(boundary) + 1, flag)) On 2016/08/22 20:52:12, Ilya Sherman wrote: > I'm not comfortable with adding a blanket static_cast to the macro, as it might > hide legitimate compilation warnings/errors as well as just the enum class ones. > Is there a way to more specifically target just enum classes? Thanks, in retrospect that was careless. I created a function, base::underlying_number, that returns enums as their integral types and passes other arithmetic values through. Let me know what you think. https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:5: #include "base/metrics/histogram.h" On 2016/08/22 20:52:13, Ilya Sherman wrote: > nit: histogram_macros Done. https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:39: enum class SadTabEvent { On 2016/08/22 20:52:12, Ilya Sherman wrote: > Please document that this enum is used to back an UMA histogram, and should > therefore be treated as append-only. Done — borrowed an existing comment. https://codereview.chromium.org/2261793002/diff/100001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:47: if (feedback) { On 2016/08/22 20:52:13, Ilya Sherman wrote: > What determines whether the feedback style or reload style of sad tab is shown? > If it's a field trial (aka Finch experiment), then I'd recommend having a single > histogram rather than two separate ones. The first sad tab you see in a session will be reload style, subsequent ones are feedback style. The rule might change, but the idea is that we ask for feedback when you get multiple sad tabs in a short period of time.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2261793002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2261793002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:156: static_cast<uint32_t>(boundary) + 1, flag)) On 2016/08/25 00:55:11, Sidney San Martín wrote: > On 2016/08/22 20:52:12, Ilya Sherman wrote: > > I'm not comfortable with adding a blanket static_cast to the macro, as it > might > > hide legitimate compilation warnings/errors as well as just the enum class > ones. > > Is there a way to more specifically target just enum classes? > > Thanks, in retrospect that was careless. I created a function, > base::underlying_number, that returns enums as their integral types and passes > other arithmetic values through. Let me know what you think. Very cool! What does a compile error look like if someone attempts to use this macro with something of non-integral type? Say, a double, or a string? (And, what did the compile error look like before? I'm not sure, but I'm hoping it's sane prior to this change ... and hopefully after it as well =D) https://codereview.chromium.org/2261793002/diff/120001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/120001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:36: count++; \ Optional nit: Preincrement syntax is usually preferred, though it doesn't matter a whole bunch for an int.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2261793002/diff/140001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/140001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:81: default: remove default, if possible https://codereview.chromium.org/2261793002/diff/140001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:125: default: can you remove the default clause so additions to the enum will cause a compile error?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2261793002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2261793002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:156: static_cast<uint32_t>(boundary) + 1, flag)) On 2016/08/25 01:12:56, Ilya Sherman wrote: > Very cool! What does a compile error look like if someone attempts to use this > macro with something of non-integral type? Say, a double, or a string? (And, > what did the compile error look like before? I'm not sure, but I'm hoping it's > sane prior to this change ... and hopefully after it as well =D) It passed doubles through, since existing code already passes doubles to these macros — is that intentional? In the patchset you saw, wrong types caused compile errors of this flavor: example.cc:39:39: error: no matching function for call to 'underlying_number' template_util.h:138:35: note: candidate template ignored: disabled by 'enable_if' [with T = nullptr_t] template_util.h:150:35: note: candidate template ignored: disabled by 'enable_if' [with T = char const[4]] I was unhappy with that because I originally wanted the function to be more transparent. So in the new patchset it's transparent to all non-enum types, and you get errors like this: example.cc:50:5: error: cannot initialize a parameter of type 'Sample' (aka 'int') with an lvalue of type 'char const[4]' example.cc:53:5: error: cannot initialize a parameter of type 'Sample' (aka 'int') with an rvalue of type 'nullptr_t' It's now called `base::underlying_value` to match `std::underlying_type` (but, unlike `std::underlying_type`, it's a no-op instead of UB to use with a non-enum). My solution is "interesting", but maybe it would be smarter to handle this at the Histogram level (e.g. an overload that takes enum types)? https://codereview.chromium.org/2261793002/diff/120001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/120001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:36: count++; \ On 2016/08/25 01:12:56, Ilya Sherman wrote: > Optional nit: Preincrement syntax is usually preferred, though it doesn't matter > a whole bunch for an int. np, ++consistency
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2261793002/diff/140001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/140001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:81: default: On 2016/08/26 16:44:50, Will Harris wrote: > remove default, if possible Done. https://codereview.chromium.org/2261793002/diff/140001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:125: default: On 2016/08/26 16:44:50, Will Harris wrote: > can you remove the default clause so additions to the enum will cause a compile > error? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sdy@chromium.org changed reviewers: + thakis@chromium.org - sky@chromium.org
sdy@chromium.org changed required reviewers: + thakis@chromium.org - sky@chromium.org
Thanks! The details of the template magic are somewhat over my head, but I like the apparent results =) (And, yes, I forgot that we don't warn on implicit conversions from floating point to integer precision... unfortunately there are apparently a lot of places that don't explicitly cast, so it would be some significant effort to move everything over.) https://codereview.chromium.org/2261793002/diff/200001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2261793002/diff/200001/base/template_util.h#n... base/template_util.h:129: #undef CR_USE_FALLBACKS_FOR_OLD_GLIBCXX nit: Why did you move this? If you don't need to move it, it would be best to move it back, so that it kind of follows a "normal" scoping pattern. https://codereview.chromium.org/2261793002/diff/200001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/200001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:135: CHECK(false); nit: Both here and above: NOTREACHED(); return 0;
Sorry, meant to say: LGTM % nits.
Rerouting to thakis@ because the set of files I'm touching changed.
https://codereview.chromium.org/2261793002/diff/200001/base/template_util_uni... File base/template_util_unittest.cc (right): https://codereview.chromium.org/2261793002/diff/200001/base/template_util_uni... base/template_util_unittest.cc:38: // Uncopyable() = default; ?
https://codereview.chromium.org/2261793002/diff/200001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2261793002/diff/200001/base/template_util.h#n... base/template_util.h:129: #undef CR_USE_FALLBACKS_FOR_OLD_GLIBCXX On 2016/08/30 21:06:39, Ilya Sherman wrote: > nit: Why did you move this? If you don't need to move it, it would be best to > move it back, so that it kind of follows a "normal" scoping pattern. I did have to move it, sadly. It needs the underlying_type implementation a few lines above because we're on an old libstdc++ on Linux (crbug.com/593874). https://codereview.chromium.org/2261793002/diff/200001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/200001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:135: CHECK(false); On 2016/08/30 21:06:39, Ilya Sherman wrote: > nit: Both here and above: > > NOTREACHED(); > return 0; OK. I chose CHECK(false) because NOTREACHED() falls through in release builds. Is it better to return 0 instead of crashing? I would assume (but I'm not sure) that trying to get string ID 0 from l10n is also a crash.
On 2016/08/30 21:06:39, Ilya Sherman wrote: > Thanks! The details of the template magic are somewhat over my head, but I like > the apparent results =) Yeah, that's why I wonder whether there's a better way to fix it in the callees, instead of making this bridge. Maybe not, other code could turn up that expects enums to implicitly cast.
https://codereview.chromium.org/2261793002/diff/200001/base/template_util_uni... File base/template_util_unittest.cc (right): https://codereview.chromium.org/2261793002/diff/200001/base/template_util_uni... base/template_util_unittest.cc:38: // Uncopyable() = default; On 2016/08/30 21:11:11, Nico wrote: > ? Thanks, missed that. Done.
On 2016/08/30 21:20:07, Sidney San Martín wrote: > On 2016/08/30 21:06:39, Ilya Sherman wrote: > > Thanks! The details of the template magic are somewhat over my head, but I > like > > the apparent results =) > > Yeah, that's why I wonder whether there's a better way to fix it in the callees, > instead of making this bridge. Maybe not, other code could turn up that expects > enums to implicitly cast. Mmm, the template magic is definitely a bit magical, but it honestly looks mostly straightforward. I think if someone more familiar with template programming is happy with it, then it seems like a good utility to have in base. I just don't know what all of the subtle gotchas for template specialization are.
https://codereview.chromium.org/2261793002/diff/200001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2261793002/diff/200001/base/template_util.h#n... base/template_util.h:129: #undef CR_USE_FALLBACKS_FOR_OLD_GLIBCXX On 2016/08/30 21:18:13, Sidney San Martín wrote: > On 2016/08/30 21:06:39, Ilya Sherman wrote: > > nit: Why did you move this? If you don't need to move it, it would be best to > > move it back, so that it kind of follows a "normal" scoping pattern. > > I did have to move it, sadly. It needs the underlying_type implementation a few > lines above because we're on an old libstdc++ on Linux (crbug.com/593874). Sorry, just figured out what you meant. Fixed!
Patchset #14 (id:260001) has been deleted
[thakis@: I just made a couple of small changes to sad_tab.cc, sorry if you were already mid-review.]
Description was changed from ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG now explicitly casts the sample and boundary so that it can take enum class values (which don't implicitly convert to integers). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. - The help link text no longer selects when you right click it. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ========== to ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG uses a new base::underlying_value() function to get cast scoped enums to their underlying integral values. (Unlike classic enums, they don't implicitly convert). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. - The help link text no longer selects when you right click it. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ==========
Description was changed from ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG uses a new base::underlying_value() function to get cast scoped enums to their underlying integral values. (Unlike classic enums, they don't implicitly convert). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. - The help link text no longer selects when you right click it. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ========== to ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG uses a new base::underlying_value() function to cast scoped enums to their underlying integral values. (Unlike classic enums, they don't implicitly convert). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. - The help link text no longer selects when you right click it. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ==========
(Which files did Ilya already look at and which am I supposed to look at?) https://codereview.chromium.org/2261793002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/tab_contents/sad_tab_mac.mm (right): https://codereview.chromium.org/2261793002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_contents/sad_tab_mac.mm:13: explicit SadTabCocoa(content::WebContents* web_contents, we don't usually use explicit on ctors with != 1 arg (it means something now in c++11, but just like the standard library we never adapted to that) https://codereview.chromium.org/2261793002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_contents/sad_tab_mac.mm:26: SadTabView* sad_tab_view_; add "// Owned by web_contents", comment why that's guaranteed to out-live |this|. …actually, you never use sad_tab_view_ for anything. Why even have this as a field?
On 2016/08/30 22:49:11, Nico wrote: > (Which files did Ilya already look at and which am I supposed to look at?) I believe that Ilya covered histogram_macros.h, histograms.xml, and the metrics aspects of sad_tab.cc. Everything else still needs review. https://codereview.chromium.org/2261793002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/tab_contents/sad_tab_mac.mm (right): https://codereview.chromium.org/2261793002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_contents/sad_tab_mac.mm:13: explicit SadTabCocoa(content::WebContents* web_contents, On 2016/08/30 22:49:10, Nico wrote: > we don't usually use explicit on ctors with != 1 arg (it means something now in > c++11, but just like the standard library we never adapted to that) I think the explicit may have been a holdover from an earlier version of this change, I'll ditch it. https://codereview.chromium.org/2261793002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_contents/sad_tab_mac.mm:26: SadTabView* sad_tab_view_; On 2016/08/30 22:49:10, Nico wrote: > add "// Owned by web_contents", comment why that's guaranteed to out-live > |this|. > > …actually, you never use sad_tab_view_ for anything. Why even have this as a > field? I'll add a comment on ownership. The destructor uses sad_tab_view_.
On 2016/08/30 23:06:47, Sidney San Martín wrote: > I think the explicit may have been a holdover from an earlier version of this > change, I'll ditch it. On 2016/08/30 23:06:47, Sidney San Martín wrote: > I'll add a comment on ownership. The destructor uses sad_tab_view_. I just actually changed these things, instead of leaving them in limbo.
thakis@chromium.org changed reviewers: + jamescook@chromium.org
This looks generally like a great change to me. I'm not super familiar with this code -- it looks like jamescook touched it some, so adding him in case he has comments, but if not I'm comfortable approving this. Just one comment below: (sorry, I wrote this Tue night, but I had changed my name from "Nico (away until Tue)" to "Nico" in a different tab, and switched tabs after hitting "publish all my drafts" here, and when I came back to this tab I just saw that it hadn't sent my mail and instead showed "unknown user Nico (away until Tue)" :-( ) https://codereview.chromium.org/2261793002/diff/300001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2261793002/diff/300001/base/template_util.h#n... base/template_util.h:137: constexpr typename underlying_type<T>::type underlying_value(T val) { This should be called UnderlyingType. This file is a mix of two things: 1. template utils we write 2. backfill for c++ standard library functions that don't exist on libstdc++4.6 yet (on linux) This is of type 1, so it should use chrome style naming. However, it looks like this is only used to pass enum class types to histograms -- can't you just use plain enums in those two places and not need all this machinery? And if you _really_ want to do this, maybe we could discuss it in a separate CL as changing these enums to enum classes seems kind of unrelated to this change https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:87: NSMutableParagraphStyle* titleStyle = nit: I think we slightly prefer local scoped_nsobjects over autoreleasing https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:87: return false; Nice change!
I like this change. Apologies in advance for a bunch of nits from a reviewer you didn't ask for. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:28: // TODO(jamescook): Maybe track time between sad tabs? Feel free to nuke this -- it's not going to happen. :-) https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:32: #define UMA_SAD_TAB_COUNTER(NAME) \ I wouldn't do this with a macro and string concatenation. It's important to me to be able to grep the codebase for UMA statistic names like Tabs.SadTab.KillDisplayed.OOM. Sure, the old code is a bit repetitive, but it's not that much longer. And in general we discourage macros: https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:49: UMA_HISTOGRAM_ENUMERATION("Tabs.SadTab.StyleFeedback", event, I would call these either CreatedWithFeedback or DisplayedWithFeedback to match the existing histograms. (It's not clear from this name whether it's created or shown.) https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:57: static const int kCrashesBeforeFeedbackIsDisplayed = 1; nit: static not needed in anonymous namespace https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:62: return ++total_crashes > kCrashesBeforeFeedbackIsDisplayed; Aside: I had no idea we had this feature. It seems odd that a single crash in any tab results in the feedback button being shown for all subsequent crashes -- I would expect it if a particular tab crashed repeatedly. But if that's what we do on other platforms, I'm OK with it. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:93: SadTab::SadTab(content::WebContents* web_contents, SadTabKind kind) nit: make function order in .cc file match declaration order in .h https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:112: // Fall through nit: Indent (unless this is the output of clang-format, in which case leave it alone) https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:114: case chrome::SAD_TAB_KIND_KILLED: I would keep the LOG(WARNING) about kills. Kills are very interesting -- generally they mean that the renderer sent a malformed IPC to the browser. Historically these have been due to subtle bugs and/or only occur in the field. Having something in the logs is really helpful. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:170: // Fallthrough nit: ditto https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.h (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:33: int GetTitle(); nit: Document these functions, especially what the returned int means. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:49: bool want_feedback_; nit: Document and/or rename. Who wants feedback about what? Is this really |show_feedback_link_|? https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:51: #if DCHECK_IS_ON() Does this need #if guards? Is it for unused variable warnings? If not, just record it all the time -- skipping a single assignment isn't worth the decrease in readability.
No worries, thanks for your reviews! By the way, there's still an open question of what to do in Chromium (displaying a feedback dialog seems to be a no-op). I'm thinking of just disabling the feedback state, but wondering if there's a better option. https://codereview.chromium.org/2261793002/diff/300001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2261793002/diff/300001/base/template_util.h#n... base/template_util.h:137: constexpr typename underlying_type<T>::type underlying_value(T val) { On 2016/09/08 17:12:59, Nico wrote: > …it looks like this is only used to pass enum class types to histograms > -- can't you just use plain enums in those two places and not need all this > machinery? > > And if you _really_ want to do this, maybe we could discuss it in a separate CL > as changing these enums to enum classes seems kind of unrelated to this change Done. You're right that it should be left to a separate CL, and that there isn't sufficient evidence that it's worth supporting scoped enums here. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:87: NSMutableParagraphStyle* titleStyle = On 2016/09/08 17:12:59, Nico wrote: > nit: I think we slightly prefer local scoped_nsobjects over autoreleasing Yeah, you caught me. I avoided scoped_nsobject because it handles properties poorly: ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:35:7: error: no member named 'autoresizingMask' in 'base::scoped_nsobject<NSTextField>' ret.autoresizingMask = NSViewWidthSizable; ~~~ ^ ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:36:7: error: no member named 'editable' in 'base::scoped_nsobject<NSTextField>' ret.editable = NO; ~~~ ^ ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:37:7: error: no member named 'drawsBackground' in 'base::scoped_nsobject<NSTextField>' ret.drawsBackground = NO; ~~~ ^ ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:38:7: error: no member named 'bezeled' in 'base::scoped_nsobject<NSTextField>' ret.bezeled = NO; The workarounds I can think of are: 1. ret.get().editable = NO; 2. [ret setEditable:NO]; 3. Having a scoped_nsobject just to manage ownership *and* a raw pointer. Nico/James: What seems best? (#2 seems least nasty to me, but I'd be sad to just give up on properties.) https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:28: // TODO(jamescook): Maybe track time between sad tabs? On 2016/09/08 20:15:50, James Cook wrote: > Feel free to nuke this -- it's not going to happen. :-) Done :). https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:32: #define UMA_SAD_TAB_COUNTER(NAME) \ On 2016/09/08 20:15:50, James Cook wrote: > I wouldn't do this with a macro and string concatenation. It's important to me > to be able to grep the codebase for UMA statistic names like > Tabs.SadTab.KillDisplayed.OOM. Sure, the old code is a bit repetitive, but it's > not that much longer. And in general we discourage macros: > > https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros The macro deals with creating a block with a static counter. Is there a nice middle ground between it and repeating this pattern 8x? For now, I got rid of the concatenation. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:49: UMA_HISTOGRAM_ENUMERATION("Tabs.SadTab.StyleFeedback", event, On 2016/09/08 20:15:50, James Cook wrote: > I would call these either CreatedWithFeedback or DisplayedWithFeedback to match > the existing histograms. (It's not clear from this name whether it's created or > shown.) These histograms are for other events (BUTTON_CLICKED, HELP_LINK_CLICKED, and, in the future, TAB_CLOSED, NAVIGATED_AWAY…). I'm open to other names or ways of organizing this info, though. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:57: static const int kCrashesBeforeFeedbackIsDisplayed = 1; On 2016/09/08 20:15:50, James Cook wrote: > nit: static not needed in anonymous namespace Good catch, removed. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:62: return ++total_crashes > kCrashesBeforeFeedbackIsDisplayed; On 2016/09/08 20:15:50, James Cook wrote: > Aside: I had no idea we had this feature. It seems odd that a single crash in > any tab results in the feedback button being shown for all subsequent crashes -- > I would expect it if a particular tab crashed repeatedly. But if that's what we > do on other platforms, I'm OK with it. It's what we do on other platforms, but I bet it was supposed to be a placeholder for better behavior. Think it's worth making a bug to improve it? https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:93: SadTab::SadTab(content::WebContents* web_contents, SadTabKind kind) On 2016/09/08 20:15:50, James Cook wrote: > nit: make function order in .cc file match declaration order in .h Done. I wonder how hard it would be to make `git cl format` fix things like this… https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:112: // Fall through On 2016/09/08 20:15:50, James Cook wrote: > nit: Indent (unless this is the output of clang-format, in which case leave it > alone) It is from clang-format, sadly. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:114: case chrome::SAD_TAB_KIND_KILLED: On 2016/09/08 20:15:50, James Cook wrote: > I would keep the LOG(WARNING) about kills. Kills are very interesting -- > generally they mean that the renderer sent a malformed IPC to the browser. > Historically these have been due to subtle bugs and/or only occur in the field. > Having something in the logs is really helpful. I didn't know that! Thanks, done. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.h (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:33: int GetTitle(); On 2016/09/08 20:15:50, James Cook wrote: > nit: Document these functions, especially what the returned int means. Done, let me know how these comments look. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:49: bool want_feedback_; On 2016/09/08 20:15:50, James Cook wrote: > nit: Document and/or rename. Who wants feedback about what? Is this really > |show_feedback_link_|? We, the developers, want feedback about what caused the sad tab. I don't love want_feedback_, but I avoided names like show_feedback_link_ because there are other differences (the help link changes, too). Looking at it now, I agree that this name is extra confusing, so I changed it to show_feedback_button_. Can you think of something better? https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:51: #if DCHECK_IS_ON() On 2016/09/08 20:15:50, James Cook wrote: > Does this need #if guards? Is it for unused variable warnings? If not, just > record it all the time -- skipping a single assignment isn't worth the decrease > in readability. Huh, I thought it improved readability: you're allowed to assume that anything inside the guard doesn't affect Chrome's behavior (but may offer hints about how it *should* behave). If that's off base, I can nuke it.
lgtm once James is happy. Thanks! https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:87: NSMutableParagraphStyle* titleStyle = On 2016/09/09 01:36:00, Sidney San Martín wrote: > On 2016/09/08 17:12:59, Nico wrote: > > nit: I think we slightly prefer local scoped_nsobjects over autoreleasing > > Yeah, you caught me. I avoided scoped_nsobject because it handles properties > poorly: > > ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:35:7: > error: no member named 'autoresizingMask' in > 'base::scoped_nsobject<NSTextField>' > ret.autoresizingMask = NSViewWidthSizable; > ~~~ ^ > ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:36:7: > error: no member named 'editable' in 'base::scoped_nsobject<NSTextField>' > ret.editable = NO; > ~~~ ^ > ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:37:7: > error: no member named 'drawsBackground' in 'base::scoped_nsobject<NSTextField>' > ret.drawsBackground = NO; > ~~~ ^ > ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:38:7: > error: no member named 'bezeled' in 'base::scoped_nsobject<NSTextField>' > ret.bezeled = NO; > > The workarounds I can think of are: > > 1. ret.get().editable = NO; > 2. [ret setEditable:NO]; > 3. Having a scoped_nsobject just to manage ownership *and* a raw pointer. > > Nico/James: What seems best? (#2 seems least nasty to me, but I'd be sad to just > give up on properties.) Oh hm, that's a good point. Maybe just add `// Not a scoped_nsobject for easy propetry access` – or bring it up on the chrome-mac list and see if anyone has a great suggestion. But I'd be fine with keeping what you have. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:112: // Fall through On 2016/09/09 01:36:00, Sidney San Martín wrote: > On 2016/09/08 20:15:50, James Cook wrote: > > nit: Indent (unless this is the output of clang-format, in which case leave it > > alone) > > It is from clang-format, sadly. (I think if you switch the #endif and the comment line, clang-format might indent it.)
LGTM with nits https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:32: #define UMA_SAD_TAB_COUNTER(NAME) \ On 2016/09/09 01:36:00, Sidney San Martín wrote: > On 2016/09/08 20:15:50, James Cook wrote: > > I wouldn't do this with a macro and string concatenation. It's important to me > > to be able to grep the codebase for UMA statistic names like > > Tabs.SadTab.KillDisplayed.OOM. Sure, the old code is a bit repetitive, but > it's > > not that much longer. And in general we discourage macros: > > > > https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros > > The macro deals with creating a block with a static counter. Is there a nice > middle ground between it and repeating this pattern 8x? For now, I got rid of > the concatenation. This is fine. However, please change NAME to |histogram_name| or similar. Most of our code seems to use lower-case for macro parameters. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:49: UMA_HISTOGRAM_ENUMERATION("Tabs.SadTab.StyleFeedback", event, On 2016/09/09 01:36:00, Sidney San Martín wrote: > On 2016/09/08 20:15:50, James Cook wrote: > > I would call these either CreatedWithFeedback or DisplayedWithFeedback to > match > > the existing histograms. (It's not clear from this name whether it's created > or > > shown.) > > These histograms are for other events (BUTTON_CLICKED, HELP_LINK_CLICKED, and, > in the future, TAB_CLOSED, NAVIGATED_AWAY…). I'm open to other names or ways of > organizing this info, though. Hrm, I missed that distinction when reading this code. Tabs.SadTab.Feedback.Event? Tabs.SadTab.Feedback.Action? https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:93: SadTab::SadTab(content::WebContents* web_contents, SadTabKind kind) On 2016/09/09 01:36:00, Sidney San Martín wrote: > On 2016/09/08 20:15:50, James Cook wrote: > > nit: make function order in .cc file match declaration order in .h > > Done. I wonder how hard it would be to make `git cl format` fix things like > this… Yeah, it's kind of a judgement call for me. If you're touching most of the lines anyway, I think it's worth doing. If it's longstanding code that just happens to be out of order, I don't think it's worth the churn in git blame. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.h (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:33: int GetTitle(); On 2016/09/09 01:36:00, Sidney San Martín wrote: > On 2016/09/08 20:15:50, James Cook wrote: > > nit: Document these functions, especially what the returned int means. > > Done, let me know how these comments look. Very helpful, especially the note that they vary. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:51: #if DCHECK_IS_ON() On 2016/09/09 01:36:00, Sidney San Martín wrote: > On 2016/09/08 20:15:50, James Cook wrote: > > Does this need #if guards? Is it for unused variable warnings? If not, just > > record it all the time -- skipping a single assignment isn't worth the > decrease > > in readability. > > Huh, I thought it improved readability: you're allowed to assume that anything > inside the guard doesn't affect Chrome's behavior (but may offer hints about how > it *should* behave). If that's off base, I can nuke it. I found it harder to read. Frankly, I've been in the chrome code for 5 years and I didn't know this macro existed. It's weird because looks like a method call, as opposed to all our other #if defined(CONDITION) macros. It's obvious what it means, but it makes me think you're skipping some big block of validation code, but here it's just setting a bool. It's also a little odd that recorded_paint_ is initialized here, but show_feedback_button_ is not.
Thanks again! CQ time. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:87: NSMutableParagraphStyle* titleStyle = On 2016/09/09 01:48:42, Nico wrote: > Oh hm, that's a good point. Maybe just add `// Not a scoped_nsobject for easy > propetry access` – or bring it up on the chrome-mac list and see if anyone has a > great suggestion. But I'd be fine with keeping what you have. OK. I threw in a comment for now and ask around/ask the list. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.cc (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:32: #define UMA_SAD_TAB_COUNTER(NAME) \ On 2016/09/09 04:37:08, James Cook wrote: > On 2016/09/09 01:36:00, Sidney San Martín wrote: > > On 2016/09/08 20:15:50, James Cook wrote: > > > I wouldn't do this with a macro and string concatenation. It's important to > me > > > to be able to grep the codebase for UMA statistic names like > > > Tabs.SadTab.KillDisplayed.OOM. Sure, the old code is a bit repetitive, but > > it's > > > not that much longer. And in general we discourage macros: > > > > > > https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros > > > > The macro deals with creating a block with a static counter. Is there a nice > > middle ground between it and repeating this pattern 8x? For now, I got rid of > > the concatenation. > > This is fine. However, please change NAME to |histogram_name| or similar. Most > of our code seems to use lower-case for macro parameters. Done. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:49: UMA_HISTOGRAM_ENUMERATION("Tabs.SadTab.StyleFeedback", event, On 2016/09/09 04:37:08, James Cook wrote: > > These histograms are for other events (BUTTON_CLICKED, HELP_LINK_CLICKED, and, > > in the future, TAB_CLOSED, NAVIGATED_AWAY…). I'm open to other names or ways > of > > organizing this info, though. > > Hrm, I missed that distinction when reading this code. > Tabs.SadTab.Feedback.Event? Tabs.SadTab.Feedback.Action? Done. I like the idea of splitting it up. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.cc:112: // Fall through On 2016/09/09 01:48:42, Nico wrote: > On 2016/09/09 01:36:00, Sidney San Martín wrote: > > On 2016/09/08 20:15:50, James Cook wrote: > > > nit: Indent (unless this is the output of clang-format, in which case leave > it > > > alone) > > > > It is from clang-format, sadly. > > (I think if you switch the #endif and the comment line, clang-format might > indent it.) Done. I originally avoided doing this… but yeah, it looks better :). https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.h (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:51: #if DCHECK_IS_ON() On 2016/09/09 04:37:09, James Cook wrote: > It's also a little odd that recorded_paint_ is initialized here, but > show_feedback_button_ is not. Done. recorded_paint_ always starts false, and show_feedback_button_ (and everything else in this class) starts with a dynamic value… but I buy that it feels odd. https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:51: #if DCHECK_IS_ON() On 2016/09/09 04:37:09, James Cook wrote: > I found it harder to read. Frankly, I've been in the chrome code for 5 years and > I didn't know this macro existed. It's weird because looks like a method call, > as opposed to all our other #if defined(CONDITION) macros. It's obvious what it > means, but it makes me think you're skipping some big block of validation code, > but here it's just setting a bool. Done. I also find it strange that this is a macro function.
The CQ bit was checked by sdy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, jamescook@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2261793002/#ps380001 (title: "Address second wave of comments, turn off feedback in non-Chrome builds")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.h (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:51: #if DCHECK_IS_ON() On 2016/09/09 15:39:44, Sidney San Martín wrote: > On 2016/09/09 04:37:09, James Cook wrote: > > I found it harder to read. Frankly, I've been in the chrome code for 5 years > and > > I didn't know this macro existed. It's weird because looks like a method call, > > as opposed to all our other #if defined(CONDITION) macros. It's obvious what > it > > means, but it makes me think you're skipping some big block of validation > code, > > but here it's just setting a bool. > > Done. I also find it strange that this is a macro function. FWIW this is because if it were a macro and you forgot to include logging.h, then `#if DCHECK_IS_ON` will silently do the wrong thing.
https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... File chrome/browser/ui/sad_tab.h (right): https://codereview.chromium.org/2261793002/diff/300001/chrome/browser/ui/sad_... chrome/browser/ui/sad_tab.h:51: #if DCHECK_IS_ON() On 2016/09/09 15:45:30, Nico wrote: > On 2016/09/09 15:39:44, Sidney San Martín wrote: > > On 2016/09/09 04:37:09, James Cook wrote: > > > I found it harder to read. Frankly, I've been in the chrome code for 5 years > > and > > > I didn't know this macro existed. It's weird because looks like a method > call, > > > as opposed to all our other #if defined(CONDITION) macros. It's obvious what > > it > > > means, but it makes me think you're skipping some big block of validation > > code, > > > but here it's just setting a bool. > > > > Done. I also find it strange that this is a macro function. > > FWIW this is because if it were a macro and you forgot to include logging.h, > then `#if DCHECK_IS_ON` will silently do the wrong thing. Ah, thanks. That makes sense.
Message was sent while issue was closed.
Description was changed from ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG uses a new base::underlying_value() function to cast scoped enums to their underlying integral values. (Unlike classic enums, they don't implicitly convert). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. - The help link text no longer selects when you right click it. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ========== to ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG uses a new base::underlying_value() function to cast scoped enums to their underlying integral values. (Unlike classic enums, they don't implicitly convert). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. - The help link text no longer selects when you right click it. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ==========
Message was sent while issue was closed.
Committed patchset #19 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG uses a new base::underlying_value() function to cast scoped enums to their underlying integral values. (Unlike classic enums, they don't implicitly convert). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. - The help link text no longer selects when you right click it. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. ========== to ========== Bring the feedback button to the Mac sad tab The Views implementation of SadTab supports two big things that were missing in the Mac implementation: 1. The reload button turns into a feedback button on repeated crashes. 2. Histograms for tracking different kinds of crashes. This moves that logic up into the base SadTab, so that both implementations can use it. It also adds new histograms to count when a sad tab with a reload button or feedback button is displayed, and clicks on the button or help link. Other changes: - HISTOGRAM_ENUMERATION_WITH_FLAG uses a new base::underlying_value() function to cast scoped enums to their underlying integral values. (Unlike classic enums, they don't implicitly convert). - The Show() and Close() methods of SadTab are gone. Show() was always called right after construction, and Close() was always called right before destruction, so I merged them with the ctor+dtor. - SadTabController is gone. - SadTabView no longer gets a reference to the WebContents; SadTabCocoa is now responsible for sizing it and adding it to the view hierarchy. - The help link text no longer selects when you right click it. BUG=623690 TEST=Trigger sad tabs on each platform either with chrome://crash or by killing processes in the task manager. Check that the first sad tab shows a reload button and subsequent sad tabs show a feedback button. Otherwise, the look and behavior of the sad tab should be the same. Committed: https://crrev.com/26cabf35396e65533b30062b89aee46cbf55b862 Cr-Commit-Position: refs/heads/master@{#417606} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/26cabf35396e65533b30062b89aee46cbf55b862 Cr-Commit-Position: refs/heads/master@{#417606} |
