Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
327853002:
Move the CrashKeysWin class to a pair of files as first step. (Closed)
|
Created:
6 years, 6 months ago by Sigurður Ásgeirsson Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMove CrashKeysWin to a separate file and add first tests.
R=rsesek@chromium.org
BUG=378916
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276854
Patch Set 1 : CrashKeysWin class re-potting #Patch Set 2 : Merge refactor and testing back in. #
Total comments: 11
Patch Set 3 : Address Robert's comments. #
Total comments: 12
Patch Set 4 : Address Erik's comments. #
Total comments: 2
Patch Set 5 : Make breakpad_component target type conditional for IOS. #Patch Set 6 : Fix logic reversal :/. #
Messages
Total messages: 37 (0 generated)
Hey Robert, please take a look. This should be a net zero change in functionality, I've merely transplanted the CrashKeysWin class to a header/impl file and added an initial test. Siggi
Hey Erik, Robert, this should be a functional no-op, with just that little bit of initial testing to get the ball rolling. Siggi
Yay tests! https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... File components/breakpad/app/crash_keys_win.h (right): https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... components/breakpad/app/crash_keys_win.h:3: // found in the LICENSE file. nit: blank line after https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... components/breakpad/app/crash_keys_win.h:13: // Fwd. Delete line. https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... File components/breakpad/app/crash_keys_win_unittest.cc (right): https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... components/breakpad/app/crash_keys_win_unittest.cc:20: class TestingBreakpadClient : public BreakpadClient { It may be worth moving this into mock_breakpad_client.{h,cc} at some point, if we want to test other platforms. https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... components/breakpad/app/crash_keys_win_unittest.cc:81: ASSERT_EQ(static_cast<CrashKeysWin*>(NULL), CrashKeysWin::keeper()); You can use ASSERT_TRUE(CrashKeysWin::keeper()), if you'd like. https://codereview.chromium.org/327853002/diff/40001/components/components_te... File components/components_tests.gyp (left): https://codereview.chromium.org/327853002/diff/40001/components/components_te... components/components_tests.gyp:70: 'bookmarks/browser/bookmark_utils_unittest.cc', Intentional?
Thanks, PTAL? https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... File components/breakpad/app/crash_keys_win.h (right): https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... components/breakpad/app/crash_keys_win.h:3: // found in the LICENSE file. On 2014/06/10 21:14:14, rsesek wrote: > nit: blank line after Done. https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... components/breakpad/app/crash_keys_win.h:13: // Fwd. On 2014/06/10 21:14:14, rsesek wrote: > Delete line. Done. https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... File components/breakpad/app/crash_keys_win_unittest.cc (right): https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... components/breakpad/app/crash_keys_win_unittest.cc:20: class TestingBreakpadClient : public BreakpadClient { On 2014/06/10 21:14:14, rsesek wrote: > It may be worth moving this into mock_breakpad_client.{h,cc} at some point, if > we want to test other platforms. That SGTM - is there a naming convention in Chrome for mock classes like this? https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... components/breakpad/app/crash_keys_win_unittest.cc:81: ASSERT_EQ(static_cast<CrashKeysWin*>(NULL), CrashKeysWin::keeper()); On 2014/06/10 21:14:14, rsesek wrote: > You can use ASSERT_TRUE(CrashKeysWin::keeper()), if you'd like. I generally don't mind the extra typing if it helps describe intent or improve failure messages. https://codereview.chromium.org/327853002/diff/40001/components/components_te... File components/components_tests.gyp (left): https://codereview.chromium.org/327853002/diff/40001/components/components_te... components/components_tests.gyp:70: 'bookmarks/browser/bookmark_utils_unittest.cc', On 2014/06/10 21:14:14, rsesek wrote: > Intentional? Ooops - merge messup. Thanks!
https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... File components/breakpad/app/breakpad_win.cc (right): https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/breakpad_win.cc:92: const size_t kMaxDynamicEntries = 256; Are these two constants still needed here? If so, is it essential that they be the same as in the other file? If so, should they go somewhere else? https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... File components/breakpad/app/crash_keys_win.h (right): https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:8: #include "base/command_line.h" forward decl? https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:33: google_breakpad::CustomClientInfo* GetCustomInfo( forward-decl or IWYU? https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:34: const std::wstring& exe_path, IWYU https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:51: std::vector<google_breakpad::CustomInfoEntry> custom_entries_; IWYU? https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:64: }; DISALLOW_COPY_AND_ASSIGN?
Thanks, hit me again? :) https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... File components/breakpad/app/breakpad_win.cc (right): https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/breakpad_win.cc:92: const size_t kMaxDynamicEntries = 256; On 2014/06/11 13:27:47, erikwright wrote: > Are these two constants still needed here? If so, is it essential that they be > the same as in the other file? If so, should they go somewhere else? Thanks, removed. https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... File components/breakpad/app/crash_keys_win.h (right): https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:8: #include "base/command_line.h" On 2014/06/11 13:27:48, erikwright wrote: > forward decl? Done. https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:33: google_breakpad::CustomClientInfo* GetCustomInfo( On 2014/06/11 13:27:48, erikwright wrote: > forward-decl or IWYU? Done. https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:34: const std::wstring& exe_path, On 2014/06/11 13:27:47, erikwright wrote: > IWYU Done. https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:51: std::vector<google_breakpad::CustomInfoEntry> custom_entries_; On 2014/06/11 13:27:47, erikwright wrote: > IWYU? Done. https://codereview.chromium.org/327853002/diff/100001/components/breakpad/app... components/breakpad/app/crash_keys_win.h:64: }; On 2014/06/11 13:27:48, erikwright wrote: > DISALLOW_COPY_AND_ASSIGN? Done.
https://codereview.chromium.org/327853002/diff/120001/components/breakpad/app... File components/breakpad/app/crash_keys_win_unittest.cc (right): https://codereview.chromium.org/327853002/diff/120001/components/breakpad/app... components/breakpad/app/crash_keys_win_unittest.cc:21: class MockBreakpadClient : public BreakpadClient { GMock seems to be discouraged, if not deprecated, for use in Chromium. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/-KH_IP0rIWQ While there does not appear to be a formal edict, I know that project leaders seem to reject patches that use GMock. So consider whether it's worth going against the grain here vs. just implementing a simple manual mock.
I dunno - this seems a reasonable use for GMock here to mock out an interface. Robert, your call? On Wed, Jun 11, 2014 at 10:00 AM, <erikwright@chromium.org> wrote: > > https://codereview.chromium.org/327853002/diff/120001/ > components/breakpad/app/crash_keys_win_unittest.cc > File components/breakpad/app/crash_keys_win_unittest.cc (right): > > https://codereview.chromium.org/327853002/diff/120001/ > components/breakpad/app/crash_keys_win_unittest.cc#newcode21 > components/breakpad/app/crash_keys_win_unittest.cc:21: class > MockBreakpadClient : public BreakpadClient { > GMock seems to be discouraged, if not deprecated, for use in Chromium. > > https://groups.google.com/a/chromium.org/forum/#!topic/ > chromium-dev/-KH_IP0rIWQ > > While there does not appear to be a formal edict, I know that project > leaders seem to reject patches that use GMock. > > So consider whether it's worth going against the grain here vs. just > implementing a simple manual mock. > > https://codereview.chromium.org/327853002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... File components/breakpad/app/crash_keys_win_unittest.cc (right): https://codereview.chromium.org/327853002/diff/40001/components/breakpad/app/... components/breakpad/app/crash_keys_win_unittest.cc:20: class TestingBreakpadClient : public BreakpadClient { On 2014/06/11 13:19:39, Sigurður Ásgeirsson wrote: > On 2014/06/10 21:14:14, rsesek wrote: > > It may be worth moving this into mock_breakpad_client.{h,cc} at some point, if > > we want to test other platforms. > > That SGTM - is there a naming convention in Chrome for mock classes like this? Yeah, MockBreakpadClient would be typical.
I don't have any problems with GMock. On 2014/06/11 14:02:56, Sigurður Ásgeirsson wrote: > I dunno - this seems a reasonable use for GMock here to mock out an > interface. > Robert, your call? > > > On Wed, Jun 11, 2014 at 10:00 AM, <mailto:erikwright@chromium.org> wrote: > > > > > https://codereview.chromium.org/327853002/diff/120001/ > > components/breakpad/app/crash_keys_win_unittest.cc > > File components/breakpad/app/crash_keys_win_unittest.cc (right): > > > > https://codereview.chromium.org/327853002/diff/120001/ > > components/breakpad/app/crash_keys_win_unittest.cc#newcode21 > > components/breakpad/app/crash_keys_win_unittest.cc:21: class > > MockBreakpadClient : public BreakpadClient { > > GMock seems to be discouraged, if not deprecated, for use in Chromium. > > > > https://groups.google.com/a/chromium.org/forum/#!topic/ > > chromium-dev/-KH_IP0rIWQ > > > > While there does not appear to be a formal edict, I know that project > > leaders seem to reject patches that use GMock. > > > > So consider whether it's worth going against the grain here vs. just > > implementing a simple manual mock. > > > > https://codereview.chromium.org/327853002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Thanks - looking good enough yet? https://codereview.chromium.org/327853002/diff/120001/components/breakpad/app... File components/breakpad/app/crash_keys_win_unittest.cc (right): https://codereview.chromium.org/327853002/diff/120001/components/breakpad/app... components/breakpad/app/crash_keys_win_unittest.cc:21: class MockBreakpadClient : public BreakpadClient { On 2014/06/11 14:00:00, erikwright wrote: > GMock seems to be discouraged, if not deprecated, for use in Chromium. > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/-KH_IP0rIWQ > > While there does not appear to be a formal edict, I know that project leaders > seem to reject patches that use GMock. > > So consider whether it's worth going against the grain here vs. just > implementing a simple manual mock. Since Robert doesn't mind GMock, and since this is essentially mocking a clean interface here (regrettably we don't declare interfaces pure virtual), I'd like to leave it be.
Yes, LGTM. Didn't know if you were going to move the Mock into its own file or not.
Oh, sorry - I hate reusing code until it's been used at least once :). On Wed, Jun 11, 2014 at 11:33 AM, <rsesek@chromium.org> wrote: > Yes, LGTM. Didn't know if you were going to move the Mock into its own > file or > not. > > https://codereview.chromium.org/327853002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/327853002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/327853002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/327853002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/327853002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
latest GYP changes LGTM
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/327853002/200001
Message was sent while issue was closed.
Change committed as 276854 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
